Hi Fernando,
I found the patch from Mikel solves the same issue and his v2 patch fixes
wider situation ("$arg*" BTF argument case). So I decided to pick his patch.
https://lore.kernel.org/all/[email protected]/
Let me know if there is any case the above fix does not cover.
Thank you,
On Tue, 22 Oct 2024 14:40:45 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:
> Hi,
>
> On Thu, 5 Sep 2024 00:19:16 +0200
> "Fernando F. Mancera" <[email protected]> wrote:
>
> > Hi,
> >
> > On 26/08/2024 01:56, Masami Hiramatsu (Google) wrote:
> > > On Sun, 25 Aug 2024 19:06:22 +0200
> > > "Fernando F. Mancera" <[email protected]> wrote:
> > >
> > >> Hi,
> > >>
> > >> On 25/08/2024 09:41, Masami Hiramatsu (Google) wrote:
> > >>> Hi,
> > >>>
> > >>> On Tue, 13 Aug 2024 13:25:40 -0400
> > >>> Fernando Fernandez Mancera <[email protected]> wrote:
> > >>>
> > >>>> When initializing trace_probes::nr_args, make sure the maximum number
> > >>>> of
> > >>>> probe arguments is honored. Oherwise, we can hit a NULL pointer
> > >>>> dereferences in multiple situations like on traceprobe_set_print_fmt().
> > >>>>
> > >>>> Link: https://bugzilla.redhat.com/2303876
> > >>>
> > >>> Sorry for replying later. I'm not sure why but I did not found this in
> > >>> my mbox...
> > >>>
> > >>
> > >> No worries!
> > >>
> > >>> Anyway, trace_probe_init() should return -E2BIG in this case because
> > >>> it is actuall wrong value.
> > >>>
> > >>> Can you update your patch?
> > >>>
> > >>
> > >> I agree this is the right solution but it would mean a change in
> > >> behavior. Is that fine? Before merging 035ba76014c0, it was writing up
> > >> to the maximum number of arguments and ignoring the rest. If you confirm
> > >> it is fine to change the behavior, I will update the patch.
> > >
> > > Yes, I think this is the better behavior for users too, rather than
> > > silently droping some arguments.
> > > We also need to handle this error code in callsite and log an error,
> > > and add a testcase in the syntax error testcase. But that should be done
> > > in another series. We should fix this first.
> > >
> >
> > Just in case this fell through the cracks, I've sent a V2 patch. In
> > addition I also have a series ready for the logging and syntax error
> > testcase in kselftest but I didn't send it because it requires this
> > patch to be applied.
>
> I think that v2 was lost in my mailbox, but I found that in patchwork.
> Let me pick it.
>
> Thanks,
>
> >
> > Thank you,
> > Fernando.
> >
> > > Thank you,
> > >
> > >
> > >>
> > >> Thank you!
> > >>
> > >>> Thank you,
> > >>>
> > >>>
> > >>>>
> > >>>> Fixes: 035ba76014c0 ("tracing/probes: cleanup: Set
> > >>>> trace_probe::nr_args at trace_probe_init")
> > >>>> Signed-off-by: Fernando Fernandez Mancera <[email protected]>
> > >>>> ---
> > >>>> kernel/trace/trace_probe.c | 8 ++++++--
> > >>>> 1 file changed, 6 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > >>>> index 39877c80d6cb..f577b5e71026 100644
> > >>>> --- a/kernel/trace/trace_probe.c
> > >>>> +++ b/kernel/trace/trace_probe.c
> > >>>> @@ -2043,10 +2043,14 @@ int trace_probe_init(struct trace_probe *tp,
> > >>>> const char *event,
> > >>>> goto error;
> > >>>> }
> > >>>>
> > >>>> - tp->nr_args = nargs;
> > >>>> + if (nargs > MAX_TRACE_ARGS)
> > >>>> + tp->nr_args = MAX_TRACE_ARGS;
> > >>>> + else
> > >>>> + tp->nr_args = nargs;
> > >>>> +
> > >>>> /* Make sure pointers in args[] are NULL */
> > >>>> if (nargs)
> > >>>> - memset(tp->args, 0, sizeof(tp->args[0]) * nargs);
> > >>>> + memset(tp->args, 0, sizeof(tp->args[0]) * tp->nr_args);
> > >>>>
> > >>>> return 0;
> > >>>>
> > >>>
> > >>>
> > >
> > >
>
>
> --
> Masami Hiramatsu (Google) <[email protected]>
--
Masami Hiramatsu (Google) <[email protected]>
