Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230107034335.1154374-1-quanfafu@gmail.com> <20230108162222.146d136f@rorschach.local.home>
In-Reply-To: <20230108162222.146d136f@rorschach.local.home>
From:   Quanfa Fu <quanfafu@gmail.com>
Date:   Mon, 9 Jan 2023 11:44:46 +0800
Message-ID: <CAM4UoypG3c2TDKs7GDTkzP+uFVAzWSE2bdSFH5mLWud6tigCRg@mail.gmail.com>
Subject: Re: [PATCH] tracing/eprobe: Replace kzalloc with kmalloc
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     rostedt@goodmis.or, mhiramat@kernel.org,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thanks a lot. Learned a lot from here.

I replaced snprintf with memcpy in Patchv2


On Mon, Jan 9, 2023 at 5:22 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Sat,  7 Jan 2023 11:43:35 +0800
> Quanfa Fu <quanfafu@gmail.com> wrote:
>
> > Since this memory will be filled soon below, I feel that there is
>
> kzalloc() is also used as a safety measure to make sure nothing is
> accidentally exposed. I rather keep it for safety. Just because it
> doesn't need to be here doesn't mean it should be removed. There is no
> benefit to making this kmalloc(), as this is far from a fast path.
>
> > 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.
>
> Also, it's best to remove "feelings" from change logs. Code updates are
> not made due to how one feels about something (at least it shouldn't
> be), but about having technical reasons for doing so. I do agree
> there's no reason to check snprintf() from returning negative, as
> looking at its implementation, there is no negative return. Thus, the
> change log should be:
>
>  "No need to check for negative return value from snprintf() as the
>  code does not return negative values."
>
> >
> > No functional change intended.
>
> And this does have functional changes. If the output of a compiler is
> different for a function, then that's a functional change. What we
> consider non functional changes is if functions get moved around, or
> possibly code in a function is moved into a helper function where the
> compiler *should* end up with the same assembly.
>
> Changing what malloc is called is definitely a functional change.
>
> >
> > Signed-off-by: Quanfa Fu <quanfafu@gmail.com>
> > ---
> >  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]);
>
> I wonder if this should be a strncat() instead?
>
> > -             if (ret < 0)
> > -                     goto error;
> >               if (ret > len) {
> >                       ret = -E2BIG;
> >                       goto error;
>
>         for (i = 0; i < arcc, i++)
>                 strncat(ep->filter_str, argv[i], len);
>
> I mean, before this code we have that loop already determining what len
> is, do we really need to check if it is going to be -E2BIG?
>
> -- Steve