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]>
