[PATCH 4/5] tracing: eprobe-event: Allocate string buffers from heap

Masami Hiramatsu (Google) posted 5 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH 4/5] tracing: eprobe-event: Allocate string buffers from heap
Posted by Masami Hiramatsu (Google) 2 months, 2 weeks ago
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Allocate temporary string buffers for parsing eprobe-events
from heap instead of stack.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_eprobe.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 1e18a8619b40..75d8208cd859 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -9,6 +9,7 @@
  * Copyright (C) 2021, VMware Inc, Tzvetomir Stoyanov tz.stoyanov@gmail.com>
  *
  */
+#include <linux/cleanup.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/ftrace.h>
@@ -871,10 +872,10 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 	const char *event = NULL, *group = EPROBE_EVENT_SYSTEM;
 	const char *sys_event = NULL, *sys_name = NULL;
 	struct trace_event_call *event_call;
+	char *buf1 __free(kfree) = NULL;
+	char *buf2 __free(kfree) = NULL;
+	char *gbuf __free(kfree) = NULL;
 	struct trace_eprobe *ep = NULL;
-	char buf1[MAX_EVENT_NAME_LEN];
-	char buf2[MAX_EVENT_NAME_LEN];
-	char gbuf[MAX_EVENT_NAME_LEN];
 	int ret = 0, filter_idx = 0;
 	int i, filter_cnt;
 
@@ -885,6 +886,11 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 
 	event = strchr(&argv[0][1], ':');
 	if (event) {
+		gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
+		if (!gbuf) {
+			ret = -ENOMEM;
+			goto parse_error;
+		}
 		event++;
 		ret = traceprobe_parse_event_name(&event, &group, gbuf,
 						  event - argv[0]);
@@ -894,6 +900,12 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 
 	trace_probe_log_set_index(1);
 	sys_event = argv[1];
+
+	buf2 = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
+	if (!buf2) {
+		ret = -ENOMEM;
+		goto parse_error;
+	}
 	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
 	if (ret || !sys_event || !sys_name) {
 		trace_probe_log_err(0, NO_EVENT_INFO);
@@ -901,7 +913,11 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 	}
 
 	if (!event) {
-		strscpy(buf1, sys_event, MAX_EVENT_NAME_LEN);
+		buf1 = kstrdup(sys_event, GFP_KERNEL);
+		if (!buf1) {
+			ret = -ENOMEM;
+			goto error;
+		}
 		event = buf1;
 	}
Re: [PATCH 4/5] tracing: eprobe-event: Allocate string buffers from heap
Posted by Steven Rostedt 2 months, 2 weeks ago
On Fri, 18 Jul 2025 20:34:40 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Allocate temporary string buffers for parsing eprobe-events
> from heap instead of stack.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_eprobe.c |   24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 1e18a8619b40..75d8208cd859 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -9,6 +9,7 @@
>   * Copyright (C) 2021, VMware Inc, Tzvetomir Stoyanov tz.stoyanov@gmail.com>
>   *
>   */
> +#include <linux/cleanup.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/ftrace.h>
> @@ -871,10 +872,10 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  	const char *event = NULL, *group = EPROBE_EVENT_SYSTEM;
>  	const char *sys_event = NULL, *sys_name = NULL;
>  	struct trace_event_call *event_call;
> +	char *buf1 __free(kfree) = NULL;
> +	char *buf2 __free(kfree) = NULL;
> +	char *gbuf __free(kfree) = NULL;
>  	struct trace_eprobe *ep = NULL;
> -	char buf1[MAX_EVENT_NAME_LEN];
> -	char buf2[MAX_EVENT_NAME_LEN];
> -	char gbuf[MAX_EVENT_NAME_LEN];
>  	int ret = 0, filter_idx = 0;
>  	int i, filter_cnt;
>  
> @@ -885,6 +886,11 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  
>  	event = strchr(&argv[0][1], ':');
>  	if (event) {
> +		gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> +		if (!gbuf) {
> +			ret = -ENOMEM;
> +			goto parse_error;
> +		}
>  		event++;
>  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
>  						  event - argv[0]);
> @@ -894,6 +900,12 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  
>  	trace_probe_log_set_index(1);
>  	sys_event = argv[1];
> +
> +	buf2 = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> +	if (!buf2) {
> +		ret = -ENOMEM;
> +		goto parse_error;
> +	}
>  	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
>  	if (ret || !sys_event || !sys_name) {
>  		trace_probe_log_err(0, NO_EVENT_INFO);
> @@ -901,7 +913,11 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  	}
>  
>  	if (!event) {
> -		strscpy(buf1, sys_event, MAX_EVENT_NAME_LEN);
> +		buf1 = kstrdup(sys_event, GFP_KERNEL);
> +		if (!buf1) {
> +			ret = -ENOMEM;
> +			goto error;
> +		}

I kinda hate all these updating of "ret" before jumping to error.

>  		event = buf1;
>  	}
>  

What about this:

-- Steve

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 916555f0de81..48cc1079a1dd 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -9,6 +9,7 @@
  * Copyright (C) 2021, VMware Inc, Tzvetomir Stoyanov tz.stoyanov@gmail.com>
  *
  */
+#include <linux/cleanup.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/ftrace.h>
@@ -869,10 +870,10 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 	const char *event = NULL, *group = EPROBE_EVENT_SYSTEM;
 	const char *sys_event = NULL, *sys_name = NULL;
 	struct trace_event_call *event_call;
+	char *buf1 __free(kfree) = NULL;
+	char *buf2 __free(kfree) = NULL;
+	char *gbuf __free(kfree) = NULL;
 	struct trace_eprobe *ep = NULL;
-	char buf1[MAX_EVENT_NAME_LEN];
-	char buf2[MAX_EVENT_NAME_LEN];
-	char gbuf[MAX_EVENT_NAME_LEN];
 	int ret = 0, filter_idx = 0;
 	int i, filter_cnt;
 
@@ -883,6 +884,9 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 
 	event = strchr(&argv[0][1], ':');
 	if (event) {
+		gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
+		if (!gbuf)
+			goto mem_error;
 		event++;
 		ret = traceprobe_parse_event_name(&event, &group, gbuf,
 						  event - argv[0]);
@@ -892,6 +896,10 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 
 	trace_probe_log_set_index(1);
 	sys_event = argv[1];
+
+	buf2 = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
+	if (!buf2)
+		goto mem_error;
 	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
 	if (ret || !sys_event || !sys_name) {
 		trace_probe_log_err(0, NO_EVENT_INFO);
@@ -899,7 +907,9 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 	}
 
 	if (!event) {
-		strscpy(buf1, sys_event, MAX_EVENT_NAME_LEN);
+		buf1 = kstrdup(sys_event, GFP_KERNEL);
+		if (!buf1)
+			goto mem_error;
 		event = buf1;
 	}
 
@@ -972,6 +982,9 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 	trace_probe_log_clear();
 	return ret;
 
+mem_error:
+	ret = -ENOMEM;
+	goto error;
 parse_error:
 	ret = -EINVAL;
 error:
Re: [PATCH 4/5] tracing: eprobe-event: Allocate string buffers from heap
Posted by Masami Hiramatsu (Google) 2 months, 2 weeks ago
On Fri, 18 Jul 2025 13:55:49 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 18 Jul 2025 20:34:40 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Allocate temporary string buffers for parsing eprobe-events
> > from heap instead of stack.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  kernel/trace/trace_eprobe.c |   24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> > index 1e18a8619b40..75d8208cd859 100644
> > --- a/kernel/trace/trace_eprobe.c
> > +++ b/kernel/trace/trace_eprobe.c
> > @@ -9,6 +9,7 @@
> >   * Copyright (C) 2021, VMware Inc, Tzvetomir Stoyanov tz.stoyanov@gmail.com>
> >   *
> >   */
> > +#include <linux/cleanup.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/ftrace.h>
> > @@ -871,10 +872,10 @@ static int __trace_eprobe_create(int argc, const char *argv[])
> >  	const char *event = NULL, *group = EPROBE_EVENT_SYSTEM;
> >  	const char *sys_event = NULL, *sys_name = NULL;
> >  	struct trace_event_call *event_call;
> > +	char *buf1 __free(kfree) = NULL;
> > +	char *buf2 __free(kfree) = NULL;
> > +	char *gbuf __free(kfree) = NULL;
> >  	struct trace_eprobe *ep = NULL;
> > -	char buf1[MAX_EVENT_NAME_LEN];
> > -	char buf2[MAX_EVENT_NAME_LEN];
> > -	char gbuf[MAX_EVENT_NAME_LEN];
> >  	int ret = 0, filter_idx = 0;
> >  	int i, filter_cnt;
> >  
> > @@ -885,6 +886,11 @@ static int __trace_eprobe_create(int argc, const char *argv[])
> >  
> >  	event = strchr(&argv[0][1], ':');
> >  	if (event) {
> > +		gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> > +		if (!gbuf) {
> > +			ret = -ENOMEM;
> > +			goto parse_error;
> > +		}
> >  		event++;
> >  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> >  						  event - argv[0]);
> > @@ -894,6 +900,12 @@ static int __trace_eprobe_create(int argc, const char *argv[])
> >  
> >  	trace_probe_log_set_index(1);
> >  	sys_event = argv[1];
> > +
> > +	buf2 = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> > +	if (!buf2) {
> > +		ret = -ENOMEM;
> > +		goto parse_error;
> > +	}
> >  	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
> >  	if (ret || !sys_event || !sys_name) {
> >  		trace_probe_log_err(0, NO_EVENT_INFO);
> > @@ -901,7 +913,11 @@ static int __trace_eprobe_create(int argc, const char *argv[])
> >  	}
> >  
> >  	if (!event) {
> > -		strscpy(buf1, sys_event, MAX_EVENT_NAME_LEN);
> > +		buf1 = kstrdup(sys_event, GFP_KERNEL);
> > +		if (!buf1) {
> > +			ret = -ENOMEM;
> > +			goto error;
> > +		}
> 
> I kinda hate all these updating of "ret" before jumping to error.

Agreed.

> 
> >  		event = buf1;
> >  	}
> >  
> 
> What about this:

Looks good to me. Note that eventually I will use cleanup.h to
remove gotos from this function as same as other probes too.

Anyway, in this series I will use the below code.

Thank you!

> 
> -- Steve
> 
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 916555f0de81..48cc1079a1dd 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -9,6 +9,7 @@
>   * Copyright (C) 2021, VMware Inc, Tzvetomir Stoyanov tz.stoyanov@gmail.com>
>   *
>   */
> +#include <linux/cleanup.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/ftrace.h>
> @@ -869,10 +870,10 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  	const char *event = NULL, *group = EPROBE_EVENT_SYSTEM;
>  	const char *sys_event = NULL, *sys_name = NULL;
>  	struct trace_event_call *event_call;
> +	char *buf1 __free(kfree) = NULL;
> +	char *buf2 __free(kfree) = NULL;
> +	char *gbuf __free(kfree) = NULL;
>  	struct trace_eprobe *ep = NULL;
> -	char buf1[MAX_EVENT_NAME_LEN];
> -	char buf2[MAX_EVENT_NAME_LEN];
> -	char gbuf[MAX_EVENT_NAME_LEN];
>  	int ret = 0, filter_idx = 0;
>  	int i, filter_cnt;
>  
> @@ -883,6 +884,9 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  
>  	event = strchr(&argv[0][1], ':');
>  	if (event) {
> +		gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> +		if (!gbuf)
> +			goto mem_error;
>  		event++;
>  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
>  						  event - argv[0]);
> @@ -892,6 +896,10 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  
>  	trace_probe_log_set_index(1);
>  	sys_event = argv[1];
> +
> +	buf2 = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> +	if (!buf2)
> +		goto mem_error;
>  	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
>  	if (ret || !sys_event || !sys_name) {
>  		trace_probe_log_err(0, NO_EVENT_INFO);
> @@ -899,7 +907,9 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  	}
>  
>  	if (!event) {
> -		strscpy(buf1, sys_event, MAX_EVENT_NAME_LEN);
> +		buf1 = kstrdup(sys_event, GFP_KERNEL);
> +		if (!buf1)
> +			goto mem_error;
>  		event = buf1;
>  	}
>  
> @@ -972,6 +982,9 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  	trace_probe_log_clear();
>  	return ret;
>  
> +mem_error:
> +	ret = -ENOMEM;
> +	goto error;
>  parse_error:
>  	ret = -EINVAL;
>  error:


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