linux-kernel - Re: [PATCH 2/5] tracing: fprobe-event: Allocate string buffers from heap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20250719095733.0fd0cc95a41a2f0bde589c2a@kernel.org>
Date: Sat, 19 Jul 2025 09:57:33 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] tracing: fprobe-event: Allocate string buffers from
 heap

On Fri, 18 Jul 2025 13:39:07 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Fri, 18 Jul 2025 20:34:19 +0900
> "Masami Hiramatsu (Google)" <mhiramat@...nel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> > 
> > Allocate temporary string buffers for fprobe-event from heap
> > instead of stack. This fixes the stack frame exceed limit error.
> > 
> > Reported-by: kernel test robot <lkp@...el.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202506240416.nZIhDXoO-lkp@intel.com/
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> > ---
> >  kernel/trace/trace_fprobe.c |   39 ++++++++++++++++++++++++++-------------
> >  1 file changed, 26 insertions(+), 13 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> > index 264cf7fc9a1d..fd1036e27309 100644
> > --- a/kernel/trace/trace_fprobe.c
> > +++ b/kernel/trace/trace_fprobe.c
> > @@ -1234,18 +1234,18 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
> >  	 */
> >  	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
> > -	struct module *mod __free(module_put) = NULL;
> > -	int i, new_argc = 0, ret = 0;
> > -	bool is_return = false;
> > -	char *symbol __free(kfree) = NULL;
> >  	const char *event = NULL, *group = FPROBE_EVENT_SYSTEM;
> > +	struct module *mod __free(module_put) = NULL;
> >  	const char **new_argv __free(kfree) = NULL;
> > -	char buf[MAX_EVENT_NAME_LEN];
> > -	char gbuf[MAX_EVENT_NAME_LEN];
> > -	char sbuf[KSYM_NAME_LEN];
> > -	char abuf[MAX_BTF_ARGS_LEN];
> > +	char *symbol __free(kfree) = NULL;
> > +	char *ebuf __free(kfree) = NULL;
> > +	char *gbuf __free(kfree) = NULL;
> > +	char *sbuf __free(kfree) = NULL;
> > +	char *abuf __free(kfree) = NULL;
> >  	char *dbuf __free(kfree) = NULL;
> > +	int i, new_argc = 0, ret = 0;
> >  	bool is_tracepoint = false;
> > +	bool is_return = false;
> >  
> >  	if ((argv[0][0] != 'f' && argv[0][0] != 't') || argc < 2)
> >  		return -ECANCELED;
> > @@ -1273,6 +1273,9 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  
> >  	trace_probe_log_set_index(0);
> >  	if (event) {
> > +		gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> > +		if (!gbuf)
> > +			return -ENOMEM;
> >  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> >  						  event - argv[0]);
> >  		if (ret)
> > @@ -1280,15 +1283,18 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  	}
> >  
> >  	if (!event) {
> > +		ebuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> 
> ebuf and gbuf are used with the same length. Why not just keep them
> using the same buffer? It worked before this patch, it should work
> after too.

Actually, ebug and gbuf can be used the same time. The first "event" could
have only "group name" by commit 95c104c378dc ("tracing: Auto generate event
name when creating a group of events"). In this case, after calling
traceprobe_parse_event_name(), "event" can be NULL.
(see kernel/trace/trace_probe.c:283)

In this case, gbuf is storing splitted "group name" and
ebuf is storing auto generated "event name".

Oops, and eprobe does not handle this case. Let me fix that first.

Thank you,

> 
> 	buf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> 	if (!buf)
> 		return -ENOMEM;
> 
> 	if (event) {
> 		[..]
> 	}
> 
> 	if (!event) {
> 		[..]
> 	}
> 
> And not require two different variables that will add two exit codes
> when one would do.
> 
> -- Steve
> 
> 
> > +		if (!ebuf)
> > +			return -ENOMEM;
> >  		/* Make a new event name */
> >  		if (is_tracepoint)
> > -			snprintf(buf, MAX_EVENT_NAME_LEN, "%s%s",
> > +			snprintf(ebuf, MAX_EVENT_NAME_LEN, "%s%s",
> >  				 isdigit(*symbol) ? "_" : "", symbol);
> >  		else
> > -			snprintf(buf, MAX_EVENT_NAME_LEN, "%s__%s", symbol,
> > +			snprintf(ebuf, MAX_EVENT_NAME_LEN, "%s__%s", symbol,
> >  				 is_return ? "exit" : "entry");
> > -		sanitize_event_name(buf);
> > -		event = buf;
> > +		sanitize_event_name(ebuf);
> > +		event = ebuf;
> >  	}
> >  
> >  	if (is_return)
> > @@ -1304,13 +1310,20 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  		ctx->flags |= TPARG_FL_TPOINT;
> >  		mod = NULL;
> >  		tpoint = find_tracepoint(symbol, &mod);
> > -		if (tpoint)
> > +		if (tpoint) {
> > +			sbuf = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
> > +			if (!sbuf)
> > +				return -ENOMEM;
> >  			ctx->funcname = kallsyms_lookup((unsigned long)tpoint->probestub,
> >  							NULL, NULL, NULL, sbuf);
> > +		}
> >  	}
> >  	if (!ctx->funcname)
> >  		ctx->funcname = symbol;
> >  
> > +	abuf = kmalloc(MAX_BTF_ARGS_LEN, GFP_KERNEL);
> > +	if (!abuf)
> > +		return -ENOMEM;
> >  	argc -= 2; argv += 2;
> >  	new_argv = traceprobe_expand_meta_args(argc, argv, &new_argc,
> >  					       abuf, MAX_BTF_ARGS_LEN, ctx);
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>