Re: [PATCH] tracing/eprobe: Replace kzalloc with kmalloc

[Christophe JAILLET]

Le 07/01/2023 à 13:23, Quanfa Fu a écrit :
> On Sat, Jan 7, 2023 at 4:42 PM Christophe JAILLET
> <christophe.jaillet@xxxxxxxxxx> wrote:
> > Le 07/01/2023 à 04:45, Quanfa Fu a écrit :
> > > Since this memory will be filled soon below, I feel that there is
> > > no need for a memory of all zeros here. 'snprintf' does not return
> > > negative num according to ISO C99, so I feel that no judgment is
> > > needed here.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Quanfa Fu <quanfafu@xxxxxxxxx>
> > > ---
> > >    kernel/trace/trace_eprobe.c | 4 +---
> > >    1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> > > index 352b65e2b910..cd1d271a74e7 100644
> > > --- a/kernel/trace/trace_eprobe.c
> > > +++ b/kernel/trace/trace_eprobe.c
> > > @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch
> > >        for (i = 0; i < argc; i++)
> > >                len += strlen(argv[i]) + 1;
> > >
> > > -     ep->filter_str = kzalloc(len, GFP_KERNEL);
> > > +     ep->filter_str = kmalloc(len, GFP_KERNEL);
> > >        if (!ep->filter_str)
> > >                return -ENOMEM;
> > >
> > >        p = ep->filter_str;
> > >        for (i = 0; i < argc; i++) {
> > >                ret = snprintf(p, len, "%s ", argv[i]);
> > > -             if (ret < 0)
> > > -                     goto error;
> > >                if (ret > len) {
>
> I think that:
>
> for example, argc = 2, argv = {"a", "b"};
>
> Before the loop
> ===============
> len = (1 + 1) + (1 + 1) = 4;
> ep->filter_str = 0x00 0x00 0x00 0x00
>                         ^
>                          |__ p
>
> After the loop:
> ===============
> i = 1, snprintf write: 'a' and ' ', so ret = 2
> i = 2, snprintf write: 'b' and ' ', so ret = 2
> ===============

Ok, I missed the space after the %s.

Sorry for the noise.

>   Since the length of the last argv is not enough
> to write, the space is replaced by null
>
> ep->filter_str = 0x61 0x20 0x62 0x00
>                                                            ^
>                                                            |__ p
> p = ep->fiter_str + 2 (ret1) +2 (ret2) = ep->filter_str + 4
> ===============
> so After p[-1] = *(ep->filter_str + 3) = '\0';
> ep->filter_str = 0x61 0x20 0x62 0x00
>
> According to ISO C99: " If the output was truncated due to this limit
> then the return value is the number of characters (excluding the
> terminating null byte) which would have been written to the final
> string if enough  space  had been available"
>
> The last snprintf will end with 'NULL', so I think p[-1] = '\0' can
> also be deleted

Hmm, now that I see it, I think that it is there to remove the last 
space (even if there shouldn't be any because the last snprintf will be 
truncated).

Code LGTM as-is, even if puzzling.

CJ

>