[PATCH 0/5] tracing: probes: Use heap instead of stack for temporary buffers

[PATCH 0/5] tracing: probes: Use heap instead of stack for temporary buffers

[Masami Hiramatsu (Google)]

Hi,

Here is a series of cleanup patches which allocate temporary buffers
and objects on heap (slab) instead of the stack. This reduces a
possibility of stack overflow. This may reduce the performance but
the modified path is not a hot path.

---

Masami Hiramatsu (Google) (5):
      tracing: probe: Allocate traceprobe_parse_context from heap
      tracing: fprobe-event: Allocate string buffers from heap
      tracing: kprobe-event: Allocate string buffers from heap
      tracing: eprobe-event: Allocate string buffers from heap
      tracing: uprobe-event: Allocate string buffers from heap


 kernel/trace/trace_eprobe.c |   38 +++++++++++++++++++++++--------
 kernel/trace/trace_fprobe.c |   52 ++++++++++++++++++++++++++++---------------
 kernel/trace/trace_kprobe.c |   49 ++++++++++++++++++++++++++---------------
 kernel/trace/trace_probe.h  |    9 +++++++
 kernel/trace/trace_uprobe.c |   37 ++++++++++++++++++++++---------
 5 files changed, 129 insertions(+), 56 deletions(-)

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

[PATCH 1/5] tracing: probe: Allocate traceprobe_parse_context from heap

[Masami Hiramatsu (Google)]

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

Instead of allocating traceprobe_parse_context on stack, allocate it
dynamically from heap (slab).

This change is likely intended to prevent potential stack overflow
issues, which can be a concern in the kernel environment where stack
space is limited.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202506240416.nZIhDXoO-lkp@intel.com/
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_eprobe.c |   14 ++++++++------
 kernel/trace/trace_fprobe.c |   13 ++++++++-----
 kernel/trace/trace_kprobe.c |   10 +++++++---
 kernel/trace/trace_probe.h  |    9 +++++++++
 kernel/trace/trace_uprobe.c |   15 +++++++++------
 5 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 916555f0de81..1e18a8619b40 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -797,18 +797,20 @@ find_and_get_event(const char *system, const char *event_name)
 
 static int trace_eprobe_tp_update_arg(struct trace_eprobe *ep, const char *argv[], int i)
 {
-	struct traceprobe_parse_context ctx = {
-		.event = ep->event,
-		.flags = TPARG_FL_KERNEL | TPARG_FL_TEVENT,
-	};
+	struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
 	int ret;
 
-	ret = traceprobe_parse_probe_arg(&ep->tp, i, argv[i], &ctx);
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	ctx->event = ep->event;
+	ctx->flags = TPARG_FL_KERNEL | TPARG_FL_TEVENT;
+
+	ret = traceprobe_parse_probe_arg(&ep->tp, i, argv[i], ctx);
 	/* Handle symbols "@" */
 	if (!ret)
 		ret = traceprobe_update_arg(&ep->tp.args[i]);
 
-	traceprobe_finish_parse(&ctx);
 	return ret;
 }
 
diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index dbf9d413125a..264cf7fc9a1d 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -1383,14 +1383,17 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 
 static int trace_fprobe_create_cb(int argc, const char *argv[])
 {
-	struct traceprobe_parse_context ctx = {
-		.flags = TPARG_FL_KERNEL | TPARG_FL_FPROBE,
-	};
+	struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
 	int ret;
 
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->flags = TPARG_FL_KERNEL | TPARG_FL_FPROBE,
+
 	trace_probe_log_init("trace_fprobe", argc, argv);
-	ret = trace_fprobe_create_internal(argc, argv, &ctx);
-	traceprobe_finish_parse(&ctx);
+	ret = trace_fprobe_create_internal(argc, argv, ctx);
 	trace_probe_log_clear();
 	return ret;
 }
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 3e5c47b6d7b2..15d7a381a128 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1065,14 +1065,18 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
 
 static int trace_kprobe_create_cb(int argc, const char *argv[])
 {
-	struct traceprobe_parse_context ctx = { .flags = TPARG_FL_KERNEL };
+	struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
 	int ret;
 
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	ctx->flags = TPARG_FL_KERNEL;
+
 	trace_probe_log_init("trace_kprobe", argc, argv);
 
-	ret = trace_kprobe_create_internal(argc, argv, &ctx);
+	ret = trace_kprobe_create_internal(argc, argv, ctx);
 
-	traceprobe_finish_parse(&ctx);
 	trace_probe_log_clear();
 	return ret;
 }
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 854e5668f5ee..7bc4c84464e4 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -10,6 +10,7 @@
  * Author:     Srikar Dronamraju
  */
 
+#include <linux/cleanup.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/smp.h>
@@ -438,6 +439,14 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg);
  * this MUST be called for clean up the context and return a resource.
  */
 void traceprobe_finish_parse(struct traceprobe_parse_context *ctx);
+static inline void traceprobe_free_parse_ctx(struct traceprobe_parse_context *ctx)
+{
+	traceprobe_finish_parse(ctx);
+	kfree(ctx);
+}
+
+DEFINE_FREE(traceprobe_parse_context, struct traceprobe_parse_context *,
+		if (!IS_ERR_OR_NULL(_T)) traceprobe_free_parse_ctx(_T))
 
 extern int traceprobe_split_symbol_offset(char *symbol, long *offset);
 int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index f95a2c3d5b1b..1fd479718d03 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -537,6 +537,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
  */
 static int __trace_uprobe_create(int argc, const char **argv)
 {
+	struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
 	struct trace_uprobe *tu;
 	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
 	char *arg, *filename, *rctr, *rctr_end, *tmp;
@@ -693,15 +694,17 @@ static int __trace_uprobe_create(int argc, const char **argv)
 	tu->path = path;
 	tu->filename = filename;
 
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx) {
+		ret = -ENOMEM;
+		goto error;
+	}
+	ctx->flags = (is_return ? TPARG_FL_RETURN : 0) | TPARG_FL_USER;
+
 	/* parse arguments */
 	for (i = 0; i < argc; i++) {
-		struct traceprobe_parse_context ctx = {
-			.flags = (is_return ? TPARG_FL_RETURN : 0) | TPARG_FL_USER,
-		};
-
 		trace_probe_log_set_index(i + 2);
-		ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i], &ctx);
-		traceprobe_finish_parse(&ctx);
+		ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i], ctx);
 		if (ret)
 			goto error;
 	}

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

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.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@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202506240416.nZIhDXoO-lkp@intel.com/
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.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);
+		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);

[PATCH 3/5] tracing: kprobe-event: Allocate string buffers from heap

[Masami Hiramatsu (Google)]

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

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

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_kprobe.c |   39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 15d7a381a128..793af6000f16 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -861,20 +861,20 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
 	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
 	 */
 	struct trace_kprobe *tk __free(free_trace_kprobe) = NULL;
+	const char *event = NULL, *group = KPROBE_EVENT_SYSTEM;
+	const char **new_argv __free(kfree) = NULL;
 	int i, len, new_argc = 0, ret = 0;
-	bool is_return = false;
 	char *symbol __free(kfree) = NULL;
-	char *tmp = NULL;
-	const char **new_argv __free(kfree) = NULL;
-	const char *event = NULL, *group = KPROBE_EVENT_SYSTEM;
+	char *ebuf __free(kfree) = NULL;
+	char *gbuf __free(kfree) = NULL;
+	char *abuf __free(kfree) = NULL;
+	char *dbuf __free(kfree) = NULL;
 	enum probe_print_type ptype;
+	bool is_return = false;
 	int maxactive = 0;
-	long offset = 0;
 	void *addr = NULL;
-	char buf[MAX_EVENT_NAME_LEN];
-	char gbuf[MAX_EVENT_NAME_LEN];
-	char abuf[MAX_BTF_ARGS_LEN];
-	char *dbuf __free(kfree) = NULL;
+	char *tmp = NULL;
+	long offset = 0;
 
 	switch (argv[0][0]) {
 	case 'r':
@@ -893,6 +893,8 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
 		event++;
 
 	if (isdigit(argv[0][1])) {
+		char *buf __free(kfree) = NULL;
+
 		if (!is_return) {
 			trace_probe_log_err(1, BAD_MAXACT_TYPE);
 			return -EINVAL;
@@ -905,7 +907,7 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
 			trace_probe_log_err(1, BAD_MAXACT);
 			return -EINVAL;
 		}
-		memcpy(buf, &argv[0][1], len);
+		buf = kmemdup(&argv[0][1], len + 1, GFP_KERNEL);
 		buf[len] = '\0';
 		ret = kstrtouint(buf, 0, &maxactive);
 		if (ret || !maxactive) {
@@ -973,6 +975,9 @@ static int trace_kprobe_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)
@@ -981,16 +986,22 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
 
 	if (!event) {
 		/* Make a new event name */
+		ebuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
+		if (!ebuf)
+			return -ENOMEM;
 		if (symbol)
-			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
+			snprintf(ebuf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
 				 is_return ? 'r' : 'p', symbol, offset);
 		else
-			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_0x%p",
+			snprintf(ebuf, MAX_EVENT_NAME_LEN, "%c_0x%p",
 				 is_return ? 'r' : 'p', addr);
-		sanitize_event_name(buf);
-		event = buf;
+		sanitize_event_name(ebuf);
+		event = ebuf;
 	}
 
+	abuf = kmalloc(MAX_BTF_ARGS_LEN, GFP_KERNEL);
+	if (!abuf)
+		return -ENOMEM;
 	argc -= 2; argv += 2;
 	ctx->funcname = symbol;
 	new_argv = traceprobe_expand_meta_args(argc, argv, &new_argc,

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

[Masami Hiramatsu (Google)]

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;
 	}
 

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

[Masami Hiramatsu (Google)]

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

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

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_uprobe.c |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 1fd479718d03..17124769e254 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -7,6 +7,7 @@
  */
 #define pr_fmt(fmt)	"trace_uprobe: " fmt
 
+#include <linux/cleanup.h>
 #include <linux/bpf-cgroup.h>
 #include <linux/security.h>
 #include <linux/ctype.h>
@@ -19,6 +20,7 @@
 #include <linux/filter.h>
 #include <linux/percpu.h>
 
+#include "trace.h"
 #include "trace_dynevent.h"
 #include "trace_probe.h"
 #include "trace_probe_tmpl.h"
@@ -538,15 +540,15 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
 static int __trace_uprobe_create(int argc, const char **argv)
 {
 	struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
-	struct trace_uprobe *tu;
 	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
 	char *arg, *filename, *rctr, *rctr_end, *tmp;
-	char buf[MAX_EVENT_NAME_LEN];
-	char gbuf[MAX_EVENT_NAME_LEN];
-	enum probe_print_type ptype;
-	struct path path;
 	unsigned long offset, ref_ctr_offset;
+	char *gbuf __free(kfree) = NULL;
+	char *buf __free(kfree) = NULL;
+	enum probe_print_type ptype;
+	struct trace_uprobe *tu;
 	bool is_return = false;
+	struct path path;
 	int i, ret;
 
 	ref_ctr_offset = 0;
@@ -654,6 +656,11 @@ static int __trace_uprobe_create(int argc, const char **argv)
 	/* setup a probe */
 	trace_probe_log_set_index(0);
 	if (event) {
+		gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
+		if (!gbuf) {
+			ret = -ENOMEM;
+			goto fail_address_parse;
+		}
 		ret = traceprobe_parse_event_name(&event, &group, gbuf,
 						  event - argv[0]);
 		if (ret)
@@ -674,6 +681,11 @@ static int __trace_uprobe_create(int argc, const char **argv)
 		if (ptr)
 			*ptr = '\0';
 
+		buf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
+		if (!buf) {
+			ret = -ENOMEM;
+			goto fail_address_parse;
+		}
 		snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_0x%lx", 'p', tail, offset);
 		event = buf;
 		kfree(tail);

Re: [PATCH 1/5] tracing: probe: Allocate traceprobe_parse_context from heap

[Steven Rostedt]

On Fri, 18 Jul 2025 20:34:08 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 854e5668f5ee..7bc4c84464e4 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -10,6 +10,7 @@
>   * Author:     Srikar Dronamraju
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/smp.h>

Nit, but let's keep the "upside-down x-mas tree" format:

#include <linux/seq_file.h>
#include <linux/cleanup.h>
#include <linux/slab.h>
#include <linux/smp.h>


> @@ -438,6 +439,14 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg);
>   * this MUST be called for clean up the context and return a resource.
>   */
>  void traceprobe_finish_parse(struct traceprobe_parse_context *ctx);
> +static inline void traceprobe_free_parse_ctx(struct traceprobe_parse_context *ctx)
> +{
> +	traceprobe_finish_parse(ctx);
> +	kfree(ctx);
> +}
> +
> +DEFINE_FREE(traceprobe_parse_context, struct traceprobe_parse_context *,
> +		if (!IS_ERR_OR_NULL(_T)) traceprobe_free_parse_ctx(_T))

ctx will either be allocated or NULL, I think the above could be:

		if (_T) traceprobe_free_parse_ctx(_T))


>  
>  extern int traceprobe_split_symbol_offset(char *symbol, long *offset);
>  int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index f95a2c3d5b1b..1fd479718d03 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -537,6 +537,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
>   */
>  static int __trace_uprobe_create(int argc, const char **argv)
>  {
> +	struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
>  	struct trace_uprobe *tu;
>  	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
>  	char *arg, *filename, *rctr, *rctr_end, *tmp;
> @@ -693,15 +694,17 @@ static int __trace_uprobe_create(int argc, const char **argv)
>  	tu->path = path;
>  	tu->filename = filename;
>  
> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +	ctx->flags = (is_return ? TPARG_FL_RETURN : 0) | TPARG_FL_USER;
> +
>  	/* parse arguments */
>  	for (i = 0; i < argc; i++) {
> -		struct traceprobe_parse_context ctx = {
> -			.flags = (is_return ? TPARG_FL_RETURN : 0) | TPARG_FL_USER,
> -		};
> -
>  		trace_probe_log_set_index(i + 2);
> -		ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i], &ctx);
> -		traceprobe_finish_parse(&ctx);
> +		ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i], ctx);

Doesn't this change the semantics a bit?

Before this change, traceprobe_finish_parse(&ctx) is called for every
iteration of the loop. Now we only do it when it exits the function.

-- Steve


>  		if (ret)
>  			goto error;
>  	}

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

[Steven Rostedt]

On Fri, 18 Jul 2025 20:34:19 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.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@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202506240416.nZIhDXoO-lkp@intel.com/
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.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.

	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);

Re: [PATCH 3/5] tracing: kprobe-event: Allocate string buffers from heap

[Steven Rostedt]

On Fri, 18 Jul 2025 20:34:29 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Allocate temporary string buffers for parsing kprobe-events
> from heap instead of stack.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_kprobe.c |   39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 15d7a381a128..793af6000f16 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -861,20 +861,20 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
>  	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
>  	 */
>  	struct trace_kprobe *tk __free(free_trace_kprobe) = NULL;
> +	const char *event = NULL, *group = KPROBE_EVENT_SYSTEM;
> +	const char **new_argv __free(kfree) = NULL;
>  	int i, len, new_argc = 0, ret = 0;
> -	bool is_return = false;
>  	char *symbol __free(kfree) = NULL;
> -	char *tmp = NULL;
> -	const char **new_argv __free(kfree) = NULL;
> -	const char *event = NULL, *group = KPROBE_EVENT_SYSTEM;
> +	char *ebuf __free(kfree) = NULL;
> +	char *gbuf __free(kfree) = NULL;
> +	char *abuf __free(kfree) = NULL;
> +	char *dbuf __free(kfree) = NULL;
>  	enum probe_print_type ptype;
> +	bool is_return = false;
>  	int maxactive = 0;
> -	long offset = 0;
>  	void *addr = NULL;
> -	char buf[MAX_EVENT_NAME_LEN];
> -	char gbuf[MAX_EVENT_NAME_LEN];
> -	char abuf[MAX_BTF_ARGS_LEN];
> -	char *dbuf __free(kfree) = NULL;
> +	char *tmp = NULL;
> +	long offset = 0;
>  
>  	switch (argv[0][0]) {
>  	case 'r':
> @@ -893,6 +893,8 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
>  		event++;
>  
>  	if (isdigit(argv[0][1])) {
> +		char *buf __free(kfree) = NULL;

So this gets freed when this if block ends, right?

> +
>  		if (!is_return) {
>  			trace_probe_log_err(1, BAD_MAXACT_TYPE);
>  			return -EINVAL;
> @@ -905,7 +907,7 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
>  			trace_probe_log_err(1, BAD_MAXACT);
>  			return -EINVAL;
>  		}
> -		memcpy(buf, &argv[0][1], len);
> +		buf = kmemdup(&argv[0][1], len + 1, GFP_KERNEL);
>  		buf[len] = '\0';
>  		ret = kstrtouint(buf, 0, &maxactive);
>  		if (ret || !maxactive) {
> @@ -973,6 +975,9 @@ static int trace_kprobe_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]);

And you can't use the same trick here because
traceprobe_parse_event_name() assigns "group" to gbuf and is used
outside this if block.

I notice there's no comment that states this. At the very minimum,
traceprobe_parse_event_name() should have a kerneldoc comment above its
definition and state this. But that's not an issue with this patch
series. Just an observation. Thus...

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve


>  		if (ret)
> @@ -981,16 +986,22 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
>  
>  	if (!event) {
>  		/* Make a new event name */
> +		ebuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> +		if (!ebuf)
> +			return -ENOMEM;
>  		if (symbol)
> -			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
> +			snprintf(ebuf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
>  				 is_return ? 'r' : 'p', symbol, offset);
>  		else
> -			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_0x%p",
> +			snprintf(ebuf, MAX_EVENT_NAME_LEN, "%c_0x%p",
>  				 is_return ? 'r' : 'p', addr);
> -		sanitize_event_name(buf);
> -		event = buf;
> +		sanitize_event_name(ebuf);
> +		event = ebuf;
>  	}
>  
> +	abuf = kmalloc(MAX_BTF_ARGS_LEN, GFP_KERNEL);
> +	if (!abuf)
> +		return -ENOMEM;
>  	argc -= 2; argv += 2;
>  	ctx->funcname = symbol;
>  	new_argv = traceprobe_expand_meta_args(argc, argv, &new_argc,

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

[Steven Rostedt]

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 5/5] tracing: uprobe-event: Allocate string buffers from heap

[Steven Rostedt]

On Fri, 18 Jul 2025 20:34:51 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Allocate temporary string buffers for parsing uprobe-events
> from heap instead of stack.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_uprobe.c |   22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 1fd479718d03..17124769e254 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -7,6 +7,7 @@
>   */
>  #define pr_fmt(fmt)	"trace_uprobe: " fmt
>  
> +#include <linux/cleanup.h>
>  #include <linux/bpf-cgroup.h>
>  #include <linux/security.h>
>  #include <linux/ctype.h>
> @@ -19,6 +20,7 @@
>  #include <linux/filter.h>
>  #include <linux/percpu.h>
>  
> +#include "trace.h"
>  #include "trace_dynevent.h"
>  #include "trace_probe.h"
>  #include "trace_probe_tmpl.h"
> @@ -538,15 +540,15 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
>  static int __trace_uprobe_create(int argc, const char **argv)
>  {
>  	struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
> -	struct trace_uprobe *tu;
>  	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
>  	char *arg, *filename, *rctr, *rctr_end, *tmp;
> -	char buf[MAX_EVENT_NAME_LEN];
> -	char gbuf[MAX_EVENT_NAME_LEN];
> -	enum probe_print_type ptype;
> -	struct path path;
>  	unsigned long offset, ref_ctr_offset;
> +	char *gbuf __free(kfree) = NULL;
> +	char *buf __free(kfree) = NULL;
> +	enum probe_print_type ptype;
> +	struct trace_uprobe *tu;
>  	bool is_return = false;
> +	struct path path;
>  	int i, ret;
>  
>  	ref_ctr_offset = 0;
> @@ -654,6 +656,11 @@ static int __trace_uprobe_create(int argc, const char **argv)
>  	/* setup a probe */
>  	trace_probe_log_set_index(0);
>  	if (event) {
> +		gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> +		if (!gbuf) {
> +			ret = -ENOMEM;
> +			goto fail_address_parse;
> +		}
>  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
>  						  event - argv[0]);
>  		if (ret)
> @@ -674,6 +681,11 @@ static int __trace_uprobe_create(int argc, const char **argv)
>  		if (ptr)
>  			*ptr = '\0';
>  
> +		buf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> +		if (!buf) {
> +			ret = -ENOMEM;
> +			goto fail_address_parse;
> +		}
>  		snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_0x%lx", 'p', tail, offset);
>  		event = buf;
>  		kfree(tail);

You could easily do the same thing as I mentioned in my reply to patch 4:

		if (!buf)
			goto fail_mem;

error:
	free_trace_uprobe(tu);
out:
	trace_probe_log_clear();
	return ret;

fail_mem:
	ret = -ENOMEM;
fail_address_parse:
	trace_probe_log_clear();
	path_put(&path);
	kfree(filename);

	return ret;
}

-- Steve

Re: [PATCH 1/5] tracing: probe: Allocate traceprobe_parse_context from heap

[Masami Hiramatsu]

On Fri, 18 Jul 2025 12:58:20 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 18 Jul 2025 20:34:08 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > index 854e5668f5ee..7bc4c84464e4 100644
> > --- a/kernel/trace/trace_probe.h
> > +++ b/kernel/trace/trace_probe.h
> > @@ -10,6 +10,7 @@
> >   * Author:     Srikar Dronamraju
> >   */
> >  
> > +#include <linux/cleanup.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/slab.h>
> >  #include <linux/smp.h>
> 
> Nit, but let's keep the "upside-down x-mas tree" format:
> 
> #include <linux/seq_file.h>
> #include <linux/cleanup.h>
> #include <linux/slab.h>
> #include <linux/smp.h>

Isn't it for variable rules?
I saw some examples of sorting headers by A-Z.

> 
> 
> > @@ -438,6 +439,14 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg);
> >   * this MUST be called for clean up the context and return a resource.
> >   */
> >  void traceprobe_finish_parse(struct traceprobe_parse_context *ctx);
> > +static inline void traceprobe_free_parse_ctx(struct traceprobe_parse_context *ctx)
> > +{
> > +	traceprobe_finish_parse(ctx);
> > +	kfree(ctx);
> > +}
> > +
> > +DEFINE_FREE(traceprobe_parse_context, struct traceprobe_parse_context *,
> > +		if (!IS_ERR_OR_NULL(_T)) traceprobe_free_parse_ctx(_T))
> 
> ctx will either be allocated or NULL, I think the above could be:
> 
> 		if (_T) traceprobe_free_parse_ctx(_T))

OK.

> 
> 
> >  
> >  extern int traceprobe_split_symbol_offset(char *symbol, long *offset);
> >  int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index f95a2c3d5b1b..1fd479718d03 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -537,6 +537,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
> >   */
> >  static int __trace_uprobe_create(int argc, const char **argv)
> >  {
> > +	struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
> >  	struct trace_uprobe *tu;
> >  	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
> >  	char *arg, *filename, *rctr, *rctr_end, *tmp;
> > @@ -693,15 +694,17 @@ static int __trace_uprobe_create(int argc, const char **argv)
> >  	tu->path = path;
> >  	tu->filename = filename;
> >  
> > +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx) {
> > +		ret = -ENOMEM;
> > +		goto error;
> > +	}
> > +	ctx->flags = (is_return ? TPARG_FL_RETURN : 0) | TPARG_FL_USER;
> > +
> >  	/* parse arguments */
> >  	for (i = 0; i < argc; i++) {
> > -		struct traceprobe_parse_context ctx = {
> > -			.flags = (is_return ? TPARG_FL_RETURN : 0) | TPARG_FL_USER,
> > -		};
> > -
> >  		trace_probe_log_set_index(i + 2);
> > -		ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i], &ctx);
> > -		traceprobe_finish_parse(&ctx);
> > +		ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i], ctx);
> 
> Doesn't this change the semantics a bit?

Yes, and we don't need to allocate ctx each time because probe target
point is always same (not different for each field). In this case,
we don't need to allocate/free each time.

> 
> Before this change, traceprobe_finish_parse(&ctx) is called for every
> iteration of the loop. Now we only do it when it exits the function.

Yes, but that is not a good way to use the ctx. As same as kprobe and
fprobe events, it is designed to be the same through parsing one probe,
not each field.

For the uprobe case, this is just passing ctx->flags, others are mostly
unused or temporarily used in field parsing. So allocating from stack
frame, it is OK. But allocating from heap, it involves slab allocation
and free each time. I think it is just inefficient. 

Hmm, but eprobe seems doing the same mistake. Let me split that part
to fix to keep using the same ctx through parsing one probe.


Thank you,

> 
> -- Steve
> 
> 
> >  		if (ret)
> >  			goto error;
> >  	}
> 


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

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

[Masami Hiramatsu]

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

> On Fri, 18 Jul 2025 20:34:19 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.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@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202506240416.nZIhDXoO-lkp@intel.com/
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.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@kernel.org>

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

[Masami Hiramatsu]

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>

Re: [PATCH 5/5] tracing: uprobe-event: Allocate string buffers from heap

[Masami Hiramatsu]

On Fri, 18 Jul 2025 13:58:46 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > +		buf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> > +		if (!buf) {
> > +			ret = -ENOMEM;
> > +			goto fail_address_parse;
> > +		}
> >  		snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_0x%lx", 'p', tail, offset);
> >  		event = buf;
> >  		kfree(tail);
> 
> You could easily do the same thing as I mentioned in my reply to patch 4:
> 
> 		if (!buf)
> 			goto fail_mem;
> 
> error:
> 	free_trace_uprobe(tu);
> out:
> 	trace_probe_log_clear();
> 	return ret;
> 
> fail_mem:
> 	ret = -ENOMEM;
> fail_address_parse:
> 	trace_probe_log_clear();
> 	path_put(&path);
> 	kfree(filename);
> 
> 	return ret;
> }

OK, let me update it. Thanks!

> 
> -- Steve


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

Re: [PATCH 3/5] tracing: kprobe-event: Allocate string buffers from heap

[Masami Hiramatsu]

On Fri, 18 Jul 2025 13:46:27 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 18 Jul 2025 20:34:29 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Allocate temporary string buffers for parsing kprobe-events
> > from heap instead of stack.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  kernel/trace/trace_kprobe.c |   39 +++++++++++++++++++++++++--------------
> >  1 file changed, 25 insertions(+), 14 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 15d7a381a128..793af6000f16 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -861,20 +861,20 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
> >  	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
> >  	 */
> >  	struct trace_kprobe *tk __free(free_trace_kprobe) = NULL;
> > +	const char *event = NULL, *group = KPROBE_EVENT_SYSTEM;
> > +	const char **new_argv __free(kfree) = NULL;
> >  	int i, len, new_argc = 0, ret = 0;
> > -	bool is_return = false;
> >  	char *symbol __free(kfree) = NULL;
> > -	char *tmp = NULL;
> > -	const char **new_argv __free(kfree) = NULL;
> > -	const char *event = NULL, *group = KPROBE_EVENT_SYSTEM;
> > +	char *ebuf __free(kfree) = NULL;
> > +	char *gbuf __free(kfree) = NULL;
> > +	char *abuf __free(kfree) = NULL;
> > +	char *dbuf __free(kfree) = NULL;
> >  	enum probe_print_type ptype;
> > +	bool is_return = false;
> >  	int maxactive = 0;
> > -	long offset = 0;
> >  	void *addr = NULL;
> > -	char buf[MAX_EVENT_NAME_LEN];
> > -	char gbuf[MAX_EVENT_NAME_LEN];
> > -	char abuf[MAX_BTF_ARGS_LEN];
> > -	char *dbuf __free(kfree) = NULL;
> > +	char *tmp = NULL;
> > +	long offset = 0;
> >  
> >  	switch (argv[0][0]) {
> >  	case 'r':
> > @@ -893,6 +893,8 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
> >  		event++;
> >  
> >  	if (isdigit(argv[0][1])) {
> > +		char *buf __free(kfree) = NULL;
> 
> So this gets freed when this if block ends, right?

Yes, because in this block, "buf" is used as a really temporary
buffer.

> 
> > +
> >  		if (!is_return) {
> >  			trace_probe_log_err(1, BAD_MAXACT_TYPE);
> >  			return -EINVAL;
> > @@ -905,7 +907,7 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
> >  			trace_probe_log_err(1, BAD_MAXACT);
> >  			return -EINVAL;
> >  		}
> > -		memcpy(buf, &argv[0][1], len);
> > +		buf = kmemdup(&argv[0][1], len + 1, GFP_KERNEL);
> >  		buf[len] = '\0';
> >  		ret = kstrtouint(buf, 0, &maxactive);
> >  		if (ret || !maxactive) {
> > @@ -973,6 +975,9 @@ static int trace_kprobe_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]);
> 
> And you can't use the same trick here because
> traceprobe_parse_event_name() assigns "group" to gbuf and is used
> outside this if block.

Yes, that holds the group name used until parsing the probe.

> 
> I notice there's no comment that states this. At the very minimum,
> traceprobe_parse_event_name() should have a kerneldoc comment above its
> definition and state this.

Yeah, that function should be docummented, it is a bit complicated.

> But that's not an issue with this patch
> series. Just an observation. Thus...
> 
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Thank you!

> 
> -- Steve
> 
> 
> >  		if (ret)
> > @@ -981,16 +986,22 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
> >  
> >  	if (!event) {
> >  		/* Make a new event name */
> > +		ebuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> > +		if (!ebuf)
> > +			return -ENOMEM;
> >  		if (symbol)
> > -			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
> > +			snprintf(ebuf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
> >  				 is_return ? 'r' : 'p', symbol, offset);
> >  		else
> > -			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_0x%p",
> > +			snprintf(ebuf, MAX_EVENT_NAME_LEN, "%c_0x%p",
> >  				 is_return ? 'r' : 'p', addr);
> > -		sanitize_event_name(buf);
> > -		event = buf;
> > +		sanitize_event_name(ebuf);
> > +		event = ebuf;
> >  	}
> >  
> > +	abuf = kmalloc(MAX_BTF_ARGS_LEN, GFP_KERNEL);
> > +	if (!abuf)
> > +		return -ENOMEM;
> >  	argc -= 2; argv += 2;
> >  	ctx->funcname = symbol;
> >  	new_argv = traceprobe_expand_meta_args(argc, argv, &new_argc,
> 


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

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

[Masami Hiramatsu]

On Sat, 19 Jul 2025 09:57:33 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> 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.
> 

It is my misread. eprobe does it correctly.

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