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;
Date:   Sun, 8 Jan 2023 16:22:22 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Quanfa Fu <quanfafu@gmail.com>
Cc:     rostedt@goodmis.or, mhiramat@kernel.org,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
Subject: Re: [PATCH] tracing/eprobe: Replace kzalloc with kmalloc
Message-ID: <20230108162222.146d136f@rorschach.local.home>
In-Reply-To: <20230107034335.1154374-1-quanfafu@gmail.com>
References: <20230107034335.1154374-1-quanfafu@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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