[PATCH 0/8] tracing: probes: Fixes and enhancing error logs

[PATCH 0/8] tracing: probes: Fixes and enhancing error logs

[Masami Hiramatsu (Google)]

The following series fixes some bugs and adding some error messages
which are not handled.
This also add some selftests which tests the new error messages.

Thank you,

---

Masami Hiramatsu (Google) (8):
      tracing: tprobe-events: Fix a memory leak when tprobe with $retval
      tracing: tprobe-events: Reject invalid tracepoint name
      tracing: fprobe-events: Log error for exceeding the number of entry args
      tracing: probe-events: Log errro for exceeding the number of arguments
      tracing: probe-events: Remove unused MAX_ARG_BUF_LEN macro
      selftests/ftrace: Expand the tprobe event test to check wrong format
      selftests/ftrace: Add new syntax error test
      selftests/ftrace: Add dynamic events argument limitation test case


 kernel/trace/trace_eprobe.c                        |    2 +
 kernel/trace/trace_fprobe.c                        |   25 +++++++++++-
 kernel/trace/trace_kprobe.c                        |    5 ++
 kernel/trace/trace_probe.h                         |    6 ++-
 kernel/trace/trace_uprobe.c                        |    9 +++-
 .../ftrace/test.d/dynevent/add_remove_tprobe.tc    |   14 +++++++
 .../ftrace/test.d/dynevent/dynevent_limitations.tc |   42 ++++++++++++++++++++
 .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    1 
 8 files changed, 98 insertions(+), 6 deletions(-)
 create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/dynevent_limitations.tc

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

[PATCH 1/8] tracing: tprobe-events: Fix a memory leak when tprobe with $retval

[Masami Hiramatsu (Google)]

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

Fix a memory leak when a tprobe is defined with $retval. This
combination is not allowed, but the parse_symbol_and_return() does
not free the *symbol which should not be used if it returns the error.
Thus, it leaks the *symbol memory in that error path.

Fixes: ce51e6153f77 ("tracing: fprobe-event: Fix to check tracepoint event and return")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 kernel/trace/trace_fprobe.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index b8f3c4ba309b..8826f44f69a4 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -1056,6 +1056,8 @@ static int parse_symbol_and_return(int argc, const char *argv[],
 			if (is_tracepoint) {
 				trace_probe_log_set_index(i);
 				trace_probe_log_err(tmp - argv[i], RETVAL_ON_PROBE);
+				kfree(*symbol);
+				*symbol = NULL;
 				return -EINVAL;
 			}
 			*is_return = true;

[PATCH 2/8] tracing: tprobe-events: Reject invalid tracepoint name

[Masami Hiramatsu (Google)]

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

Commit 57a7e6de9e30 ("tracing/fprobe: Support raw tracepoints on
future loaded modules") allows user to set a tprobe on non-exist
tracepoint but it does not check the tracepoint name is acceptable.
So it leads tprobe has a wrong character for events (e.g. with
subsystem prefix). In this case, the event is not shown in the
events directory.

Reject such invalid tracepoint name.

The tracepoint name must consist of alphabet or digit or '_'.

Fixes: 57a7e6de9e30 ("tracing/fprobe: Support raw tracepoints on future loaded modules")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 kernel/trace/trace_fprobe.c |   13 +++++++++++++
 kernel/trace/trace_probe.h  |    1 +
 2 files changed, 14 insertions(+)

diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index 8826f44f69a4..85f037dc1462 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -1049,6 +1049,19 @@ static int parse_symbol_and_return(int argc, const char *argv[],
 	if (*is_return)
 		return 0;
 
+	if (is_tracepoint) {
+		tmp = *symbol;
+		while (*tmp && (isalnum(*tmp) || *tmp == '_'))
+			tmp++;
+		if (*tmp) {
+			/* find a wrong character. */
+			trace_probe_log_err(tmp - *symbol, BAD_TP_NAME);
+			kfree(*symbol);
+			*symbol = NULL;
+			return -EINVAL;
+		}
+	}
+
 	/* If there is $retval, this should be a return fprobe. */
 	for (i = 2; i < argc; i++) {
 		tmp = strstr(argv[i], "$retval");
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 5803e6a41570..fba3ede87054 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -481,6 +481,7 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
 	C(NON_UNIQ_SYMBOL,	"The symbol is not unique"),		\
 	C(BAD_RETPROBE,		"Retprobe address must be an function entry"), \
 	C(NO_TRACEPOINT,	"Tracepoint is not found"),		\
+	C(BAD_TP_NAME,		"Invalid character in tracepoint name"),\
 	C(BAD_ADDR_SUFFIX,	"Invalid probed address suffix"), \
 	C(NO_GROUP_NAME,	"Group name is not specified"),		\
 	C(GROUP_TOO_LONG,	"Group name is too long"),		\

[PATCH 3/8] tracing: fprobe-events: Log error for exceeding the number of entry args

[Masami Hiramatsu (Google)]

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

Add error message when the number of entry argument exceeds the
maximum size of entry data.
This is currently checked when registering fprobe, but in this case
no error message is shown in the error_log file.

Fixes: 25f00e40ce79 ("tracing/probes: Support $argN in return probe (kprobe and fprobe)")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_fprobe.c |    5 +++++
 kernel/trace/trace_probe.h  |    3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index 85f037dc1462..e27305d31fc5 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -1230,6 +1230,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 	if (is_return && tf->tp.entry_arg) {
 		tf->fp.entry_handler = trace_fprobe_entry_handler;
 		tf->fp.entry_data_size = traceprobe_get_entry_data_size(&tf->tp);
+		if (ALIGN(tf->fp.entry_data_size, sizeof(long)) > MAX_FPROBE_DATA_SIZE) {
+			trace_probe_log_set_index(2);
+			trace_probe_log_err(0, TOO_MANY_EARGS);
+			return -E2BIG;
+		}
 	}
 
 	ret = traceprobe_set_print_fmt(&tf->tp,
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index fba3ede87054..c47ca002347a 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -545,7 +545,8 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
 	C(NO_BTF_FIELD,		"This field is not found."),	\
 	C(BAD_BTF_TID,		"Failed to get BTF type info."),\
 	C(BAD_TYPE4STR,		"This type does not fit for string."),\
-	C(NEED_STRING_TYPE,	"$comm and immediate-string only accepts string type"),
+	C(NEED_STRING_TYPE,	"$comm and immediate-string only accepts string type"),\
+	C(TOO_MANY_EARGS,	"Too many entry arguments specified"),
 
 #undef C
 #define C(a, b)		TP_ERR_##a

[PATCH 4/8] tracing: probe-events: Log errro for exceeding the number of arguments

[Masami Hiramatsu (Google)]

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

Add error message when the number of arguments exceeeds the limitation.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_eprobe.c |    2 ++
 kernel/trace/trace_fprobe.c |    5 ++++-
 kernel/trace/trace_kprobe.c |    5 ++++-
 kernel/trace/trace_probe.h  |    1 +
 kernel/trace/trace_uprobe.c |    9 +++++++--
 5 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 82fd637cfc19..af9fa0632b57 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -913,6 +913,8 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 	}
 
 	if (argc - 2 > MAX_TRACE_ARGS) {
+		trace_probe_log_set_index(2);
+		trace_probe_log_err(0, TOO_MANY_ARGS);
 		ret = -E2BIG;
 		goto error;
 	}
diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index e27305d31fc5..372936163c21 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -1201,8 +1201,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 		argc = new_argc;
 		argv = new_argv;
 	}
-	if (argc > MAX_TRACE_ARGS)
+	if (argc > MAX_TRACE_ARGS) {
+		trace_probe_log_set_index(2);
+		trace_probe_log_err(0, TOO_MANY_ARGS);
 		return -E2BIG;
+	}
 
 	ret = traceprobe_expand_dentry_args(argc, argv, &dbuf);
 	if (ret)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index d8d5f18a141a..8287b175667f 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1007,8 +1007,11 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
 		argc = new_argc;
 		argv = new_argv;
 	}
-	if (argc > MAX_TRACE_ARGS)
+	if (argc > MAX_TRACE_ARGS) {
+		trace_probe_log_set_index(2);
+		trace_probe_log_err(0, TOO_MANY_ARGS);
 		return -E2BIG;
+	}
 
 	ret = traceprobe_expand_dentry_args(argc, argv, &dbuf);
 	if (ret)
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index c47ca002347a..6075afc8ea67 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -546,6 +546,7 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
 	C(BAD_BTF_TID,		"Failed to get BTF type info."),\
 	C(BAD_TYPE4STR,		"This type does not fit for string."),\
 	C(NEED_STRING_TYPE,	"$comm and immediate-string only accepts string type"),\
+	C(TOO_MANY_ARGS,	"Too many arguments are specified"),	\
 	C(TOO_MANY_EARGS,	"Too many entry arguments specified"),
 
 #undef C
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index ccc762fbb69c..3386439ec9f6 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -562,8 +562,14 @@ static int __trace_uprobe_create(int argc, const char **argv)
 
 	if (argc < 2)
 		return -ECANCELED;
-	if (argc - 2 > MAX_TRACE_ARGS)
+
+	trace_probe_log_init("trace_uprobe", argc, argv);
+
+	if (argc - 2 > MAX_TRACE_ARGS) {
+		trace_probe_log_set_index(2);
+		trace_probe_log_err(0, TOO_MANY_ARGS);
 		return -E2BIG;
+	}
 
 	if (argv[0][1] == ':')
 		event = &argv[0][2];
@@ -582,7 +588,6 @@ static int __trace_uprobe_create(int argc, const char **argv)
 		return -ECANCELED;
 	}
 
-	trace_probe_log_init("trace_uprobe", argc, argv);
 	trace_probe_log_set_index(1);	/* filename is the 2nd argument */
 
 	*arg++ = '\0';

[PATCH 5/8] tracing: probe-events: Remove unused MAX_ARG_BUF_LEN macro

[Masami Hiramatsu (Google)]

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

Commit 18b1e870a496 ("tracing/probes: Add $arg* meta argument for all
function args") introduced MAX_ARG_BUF_LEN but it is not used.
Remove it.

Fixes: 18b1e870a496 ("tracing/probes: Add $arg* meta argument for all function args")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_probe.h |    1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 6075afc8ea67..854e5668f5ee 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -36,7 +36,6 @@
 #define MAX_BTF_ARGS_LEN	128
 #define MAX_DENTRY_ARGS_LEN	256
 #define MAX_STRING_SIZE		PATH_MAX
-#define MAX_ARG_BUF_LEN		(MAX_TRACE_ARGS * MAX_ARG_NAME_LEN)
 
 /* Reserved field names */
 #define FIELD_STRING_IP		"__probe_ip"

[PATCH 6/8] selftests/ftrace: Expand the tprobe event test to check wrong format

[Masami Hiramatsu (Google)]

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

Expand the tprobe event test case to check wrong tracepoint
format.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 .../ftrace/test.d/dynevent/add_remove_tprobe.tc    |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_tprobe.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_tprobe.tc
index 155792eaeee5..f271c4238b72 100644
--- a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_tprobe.tc
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_tprobe.tc
@@ -6,6 +6,7 @@
 echo 0 > events/enable
 echo > dynamic_events
 
+SUBSYSTEM=kmem
 TRACEPOINT1=kmem_cache_alloc
 TRACEPOINT2=kmem_cache_free
 
@@ -24,4 +25,17 @@ grep -q myevent1 dynamic_events
 
 echo > dynamic_events
 
+# auto naming check
+echo "t $TRACEPOINT1" >> dynamic_events
+
+test -d events/tracepoints/$TRACEPOINT1
+
+echo > dynamic_events
+
+# SUBSYSTEM is not supported
+echo "t $SUBSYSTEM/$TRACEPOINT1" >> dynamic_events && exit_fail ||:
+echo "t $SUBSYSTEM:$TRACEPOINT1" >> dynamic_events && exit_fail ||:
+echo "t:myevent3 $SUBSYSTEM/$TRACEPOINT1" >> dynamic_events && exit_fail ||:
+echo "t:myevent3 $SUBSYSTEM:$TRACEPOINT1" >> dynamic_events && exit_fail ||:
+
 clear_trace

[PATCH 7/8] selftests/ftrace: Add new syntax error test

[Masami Hiramatsu (Google)]

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

Add BAD_TP_NAME syntax error message check.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
index c9425a34fae3..fee479295e2f 100644
--- a/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
@@ -27,6 +27,7 @@ check_error 'f:^foo.1/bar vfs_read'	# BAD_GROUP_NAME
 check_error 'f:^ vfs_read'		# NO_EVENT_NAME
 check_error 'f:foo/^12345678901234567890123456789012345678901234567890123456789012345 vfs_read'	# EVENT_TOO_LONG
 check_error 'f:foo/^bar.1 vfs_read'	# BAD_EVENT_NAME
+check_error 't kmem^/kfree'       # BAD_TP_NAME
 
 check_error 'f vfs_read ^$stack10000'	# BAD_STACK_NUM
 

[PATCH 8/8] selftests/ftrace: Add dynamic events argument limitation test case

[Masami Hiramatsu (Google)]

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

Add argument limitation test case for dynamic events.
This is a boudary check for the maximum number of the probe
event arguments.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 .../ftrace/test.d/dynevent/dynevent_limitations.tc |   42 ++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/dynevent_limitations.tc

diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/dynevent_limitations.tc b/tools/testing/selftests/ftrace/test.d/dynevent/dynevent_limitations.tc
new file mode 100644
index 000000000000..6b94b678741a
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/dynevent_limitations.tc
@@ -0,0 +1,42 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: Checking dynamic events limitations
+# requires: dynamic_events "imm-value":README
+
+# Max arguments limitation
+MAX_ARGS=128
+EXCEED_ARGS=$((MAX_ARGS + 1))
+
+check_max_args() { # event_header
+  TEST_STRING=$1
+  # Acceptable
+  for i in `seq 1 $MAX_ARGS`; do
+    TEST_STRING="$TEST_STRING \\$i"
+  done
+  echo "$TEST_STRING" >> dynamic_events
+  echo > dynamic_events
+  # Error
+  TEST_STRING="$TEST_STRING \\$EXCEED_ARGS"
+  ! echo "$TEST_STRING" >> dynamic_events
+  return 0
+}
+
+# Kprobe max args limitation
+if grep -q "kprobe_events" README; then
+  check_max_args "p vfs_read"
+fi
+
+# Fprobe max args limitation
+if grep -q "f[:[<group>/][<event>]] <func-name>[%return] [<args>]" README; then
+  check_max_args "f vfs_read"
+fi
+
+# Tprobe max args limitation
+if grep -q "t[:[<group>/][<event>]] <tracepoint> [<args>]" README; then
+  check_max_args "t kfree"
+fi
+
+# Uprobe max args limitation
+if grep -q "uprobe_events" README; then
+  check_max_args "p /bin/sh:10"
+fi

Re: [PATCH 1/8] tracing: tprobe-events: Fix a memory leak when tprobe with $retval

[Steven Rostedt]

On Wed, 26 Feb 2025 15:18:46 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Fix a memory leak when a tprobe is defined with $retval. This
> combination is not allowed, but the parse_symbol_and_return() does
> not free the *symbol which should not be used if it returns the error.
> Thus, it leaks the *symbol memory in that error path.
> 
> Fixes: ce51e6153f77 ("tracing: fprobe-event: Fix to check tracepoint event and return")
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Cc: stable@vger.kernel.org

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

-- Steve

> ---
>  kernel/trace/trace_fprobe.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> index b8f3c4ba309b..8826f44f69a4 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -1056,6 +1056,8 @@ static int parse_symbol_and_return(int argc, const char *argv[],
>  			if (is_tracepoint) {
>  				trace_probe_log_set_index(i);
>  				trace_probe_log_err(tmp - argv[i], RETVAL_ON_PROBE);
> +				kfree(*symbol);
> +				*symbol = NULL;
>  				return -EINVAL;
>  			}
>  			*is_return = true;

Re: [PATCH 2/8] tracing: tprobe-events: Reject invalid tracepoint name

[Steven Rostedt]

On Wed, 26 Feb 2025 15:18:54 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Commit 57a7e6de9e30 ("tracing/fprobe: Support raw tracepoints on
> future loaded modules") allows user to set a tprobe on non-exist
> tracepoint but it does not check the tracepoint name is acceptable.
> So it leads tprobe has a wrong character for events (e.g. with
> subsystem prefix). In this case, the event is not shown in the
> events directory.
> 
> Reject such invalid tracepoint name.
> 
> The tracepoint name must consist of alphabet or digit or '_'.
> 
> Fixes: 57a7e6de9e30 ("tracing/fprobe: Support raw tracepoints on future loaded modules")
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Cc: stable@vger.kernel.org

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

-- Steve


> ---
>  kernel/trace/trace_fprobe.c |   13 +++++++++++++
>  kernel/trace/trace_probe.h  |    1 +
>  2 files changed, 14 insertions(+)

Re: [PATCH 3/8] tracing: fprobe-events: Log error for exceeding the number of entry args

[Steven Rostedt]

On Wed, 26 Feb 2025 15:19:02 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> index 85f037dc1462..e27305d31fc5 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -1230,6 +1230,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  	if (is_return && tf->tp.entry_arg) {
>  		tf->fp.entry_handler = trace_fprobe_entry_handler;
>  		tf->fp.entry_data_size = traceprobe_get_entry_data_size(&tf->tp);

Looking at traceprobe_get_entry_data_size(), the setting of the offset and
what it returns is a bit of magic. It's not intuitive and really could use
some comments. This isn't against this patch, but it does make it hard to
review this patch.

> +		if (ALIGN(tf->fp.entry_data_size, sizeof(long)) > MAX_FPROBE_DATA_SIZE) {
> +			trace_probe_log_set_index(2);
> +			trace_probe_log_err(0, TOO_MANY_EARGS);
> +			return -E2BIG;
> +		}
>  	}
>  
>  	ret = traceprobe_set_print_fmt(&tf->tp,


We have this in trace_probe.c:

static int __store_entry_arg(struct trace_probe *tp, int argnum)
{
	struct probe_entry_arg *earg = tp->entry_arg;
	bool match = false;
	int i, offset;

	if (!earg) {
		earg = kzalloc(sizeof(*tp->entry_arg), GFP_KERNEL);
		if (!earg)
			return -ENOMEM;
		earg->size = 2 * tp->nr_args + 1;
		earg->code = kcalloc(earg->size, sizeof(struct fetch_insn),
				     GFP_KERNEL);
		if (!earg->code) {
			kfree(earg);
			return -ENOMEM;
		}
		/* Fill the code buffer with 'end' to simplify it */
		for (i = 0; i < earg->size; i++)
			earg->code[i].op = FETCH_OP_END;
		tp->entry_arg = earg;
	}

	offset = 0;
	for (i = 0; i < earg->size - 1; i++) {
		switch (earg->code[i].op) {
		case FETCH_OP_END:
			earg->code[i].op = FETCH_OP_ARG;
			earg->code[i].param = argnum;
			earg->code[i + 1].op = FETCH_OP_ST_EDATA;
			earg->code[i + 1].offset = offset;

// There's definitely some dependency between FETCH_OP_END, FETCH_OP_ARG
// and FETCH_OP_ST_EDATA that isn't documented. At least not in this file.

			return offset;
		case FETCH_OP_ARG:
			match = (earg->code[i].param == argnum);
			break;
		case FETCH_OP_ST_EDATA:
			offset = earg->code[i].offset;
			if (match)
				return offset;
			offset += sizeof(unsigned long);
			break;
		default:
			break;
		}
	}
	return -ENOSPC;
}

int traceprobe_get_entry_data_size(struct trace_probe *tp)
{
	struct probe_entry_arg *earg = tp->entry_arg;
	int i, size = 0;

	if (!earg)
		return 0;

	for (i = 0; i < earg->size; i++) {
		switch (earg->code[i].op) {
		case FETCH_OP_END:
			goto out;
		case FETCH_OP_ST_EDATA:
			size = earg->code[i].offset + sizeof(unsigned long);

// What makes earg->code[i].offset so special?
// What is the guarantee that code[i + 1].offset is greater than code[i].offset?
// I guess the above function guarantees that, but it's not obvious here.

			break;
		default:
			break;
		}
	}
out:
	return size;
}

Assuming that traceprobe_get_entry_data_size() does properly return the max size.

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

-- Steve

Re: [PATCH 4/8] tracing: probe-events: Log errro for exceeding the number of arguments

[Steven Rostedt]

On Wed, 26 Feb 2025 15:19:10 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Add error message when the number of arguments exceeeds the limitation.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

-- Steve

> ---
>  kernel/trace/trace_eprobe.c |    2 ++
>  kernel/trace/trace_fprobe.c |    5 ++++-
>  kernel/trace/trace_kprobe.c |    5 ++++-
>  kernel/trace/trace_probe.h  |    1 +
>  kernel/trace/trace_uprobe.c |    9 +++++++--
>  5 files changed, 18 insertions(+), 4 deletions(-)

Re: [PATCH 5/8] tracing: probe-events: Remove unused MAX_ARG_BUF_LEN macro

[Steven Rostedt]

On Wed, 26 Feb 2025 15:19:18 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Commit 18b1e870a496 ("tracing/probes: Add $arg* meta argument for all
> function args") introduced MAX_ARG_BUF_LEN but it is not used.
> Remove it.
> 
> Fixes: 18b1e870a496 ("tracing/probes: Add $arg* meta argument for all function args")
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---

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

-- Steve


>  kernel/trace/trace_probe.h |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 6075afc8ea67..854e5668f5ee 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -36,7 +36,6 @@
>  #define MAX_BTF_ARGS_LEN	128
>  #define MAX_DENTRY_ARGS_LEN	256
>  #define MAX_STRING_SIZE		PATH_MAX
> -#define MAX_ARG_BUF_LEN		(MAX_TRACE_ARGS * MAX_ARG_NAME_LEN)
>  
>  /* Reserved field names */
>  #define FIELD_STRING_IP		"__probe_ip"

Re: [PATCH 2/8] tracing: tprobe-events: Reject invalid tracepoint name

[Markus Elfring]

…
> +++ b/kernel/trace/trace_fprobe.c
> @@ -1049,6 +1049,19 @@ static int parse_symbol_and_return(int argc, const char *argv[],
>  	if (*is_return)
>  		return 0;
>
> +	if (is_tracepoint) {
…
> +			kfree(*symbol);
> +			*symbol = NULL;
> +			return -EINVAL;
> +		}
> +	}
…

May a bit of exception handling code be shared by an additional jump target?
Will another goto chain become helpful here?

Regards,
Markus

Re: [PATCH 4/8] tracing: probe-events: Log errro for exceeding the number of arguments

[Markus Elfring]

…
> +++ b/kernel/trace/trace_fprobe.c
> @@ -1201,8 +1201,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  		argc = new_argc;
>  		argv = new_argv;
>  	}
> -	if (argc > MAX_TRACE_ARGS)
> +	if (argc > MAX_TRACE_ARGS) {
> +		trace_probe_log_set_index(2);
> +		trace_probe_log_err(0, TOO_MANY_ARGS);
>  		return -E2BIG;
> +	}
>
>  	ret = traceprobe_expand_dentry_args(argc, argv, &dbuf);
…

May a bit of exception handling code be shared by an additional jump target?
Will another goto chain become helpful here?


How do you think about to avoid a typo in the summary phrase?

Regards,
Markus

Re: [PATCH 3/8] tracing: fprobe-events: Log error for exceeding the number of entry args

[Masami Hiramatsu]

On Wed, 26 Feb 2025 10:22:23 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 26 Feb 2025 15:19:02 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > index 85f037dc1462..e27305d31fc5 100644
> > --- a/kernel/trace/trace_fprobe.c
> > +++ b/kernel/trace/trace_fprobe.c
> > @@ -1230,6 +1230,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  	if (is_return && tf->tp.entry_arg) {
> >  		tf->fp.entry_handler = trace_fprobe_entry_handler;
> >  		tf->fp.entry_data_size = traceprobe_get_entry_data_size(&tf->tp);
> 
> Looking at traceprobe_get_entry_data_size(), the setting of the offset and
> what it returns is a bit of magic. It's not intuitive and really could use
> some comments. This isn't against this patch, but it does make it hard to
> review this patch.

Indeed. It is a bit tricky.

> 
> > +		if (ALIGN(tf->fp.entry_data_size, sizeof(long)) > MAX_FPROBE_DATA_SIZE) {
> > +			trace_probe_log_set_index(2);
> > +			trace_probe_log_err(0, TOO_MANY_EARGS);
> > +			return -E2BIG;
> > +		}
> >  	}
> >  
> >  	ret = traceprobe_set_print_fmt(&tf->tp,
> 
> 
> We have this in trace_probe.c:
> 
> static int __store_entry_arg(struct trace_probe *tp, int argnum)
> {
> 	struct probe_entry_arg *earg = tp->entry_arg;
> 	bool match = false;
> 	int i, offset;
> 
> 	if (!earg) {
> 		earg = kzalloc(sizeof(*tp->entry_arg), GFP_KERNEL);
> 		if (!earg)
> 			return -ENOMEM;
> 		earg->size = 2 * tp->nr_args + 1;
> 		earg->code = kcalloc(earg->size, sizeof(struct fetch_insn),
> 				     GFP_KERNEL);
> 		if (!earg->code) {
> 			kfree(earg);
> 			return -ENOMEM;
> 		}
> 		/* Fill the code buffer with 'end' to simplify it */
> 		for (i = 0; i < earg->size; i++)
> 			earg->code[i].op = FETCH_OP_END;
> 		tp->entry_arg = earg;
> 	}
> 
> 	offset = 0;
> 	for (i = 0; i < earg->size - 1; i++) {
> 		switch (earg->code[i].op) {
> 		case FETCH_OP_END:
> 			earg->code[i].op = FETCH_OP_ARG;
> 			earg->code[i].param = argnum;
> 			earg->code[i + 1].op = FETCH_OP_ST_EDATA;
> 			earg->code[i + 1].offset = offset;
> 
> // There's definitely some dependency between FETCH_OP_END, FETCH_OP_ARG
> // and FETCH_OP_ST_EDATA that isn't documented. At least not in this file.

This accumurates the fetching operation on the code. The problem is
limitation of the entry data size. So this scans the entry code and
checks whether the specified function argument is already stored, and
reuse it (return the offset where it is stored). If there isn't,
it stores a pair of FETCH_OP_ARG + FETCH_OP_ST_EDATA at the end of
the code array.

> 
> 			return offset;
> 		case FETCH_OP_ARG:
> 			match = (earg->code[i].param == argnum);
> 			break;
> 		case FETCH_OP_ST_EDATA:
> 			offset = earg->code[i].offset;
> 			if (match)
> 				return offset;
> 			offset += sizeof(unsigned long);
> 			break;
> 		default:
> 			break;
> 		}
> 	}
> 	return -ENOSPC;
> }
> 
> int traceprobe_get_entry_data_size(struct trace_probe *tp)
> {
> 	struct probe_entry_arg *earg = tp->entry_arg;
> 	int i, size = 0;
> 
> 	if (!earg)
> 		return 0;
> 
> 	for (i = 0; i < earg->size; i++) {
> 		switch (earg->code[i].op) {
> 		case FETCH_OP_END:
> 			goto out;
> 		case FETCH_OP_ST_EDATA:
> 			size = earg->code[i].offset + sizeof(unsigned long);
> 
> // What makes earg->code[i].offset so special?
> // What is the guarantee that code[i + 1].offset is greater than code[i].offset?
> // I guess the above function guarantees that, but it's not obvious here.

Yeah, let me explain.

	/*
	 * earg->code[] array has an operation sequence which is run in
	 * the entry handler.
	 * The sequence stopped by FETCH_OP_END and each data stored in
	 * the entry data buffer by FETCH_OP_ST_EDATA. The FETCH_OP_ST_EDATA
	 * stores the data at the data buffer + its offset, and all data are
	 * "unsigned long" size. The offset must be increased when a data is
	 * stored. Thus we need to find the last FETCH_OP_ST_EDATA in the
	 * code array.
	 */

> 
> 			break;
> 		default:
> 			break;
> 		}
> 	}
> out:
> 	return size;
> }
> 
> Assuming that traceprobe_get_entry_data_size() does properly return the max size.

Yes, maybe we can scan the code array from the end for optimization but it still
needs the explanation.


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

Thank you!

> 
> -- Steve


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

Re: [PATCH 4/8] tracing: probe-events: Log errro for exceeding the number of arguments

[Masami Hiramatsu]

On Wed, 26 Feb 2025 17:13:17 +0100
Markus Elfring <Markus.Elfring@web.de> wrote:

> …
> > +++ b/kernel/trace/trace_fprobe.c
> > @@ -1201,8 +1201,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  		argc = new_argc;
> >  		argv = new_argv;
> >  	}
> > -	if (argc > MAX_TRACE_ARGS)
> > +	if (argc > MAX_TRACE_ARGS) {
> > +		trace_probe_log_set_index(2);
> > +		trace_probe_log_err(0, TOO_MANY_ARGS);
> >  		return -E2BIG;
> > +	}
> >
> >  	ret = traceprobe_expand_dentry_args(argc, argv, &dbuf);
> …
> 
> May a bit of exception handling code be shared by an additional jump target?
> Will another goto chain become helpful here?

Honestly, I don't want to introduce jump any more.

> 
> 
> How do you think about to avoid a typo in the summary phrase?

Ah, I added too much 'e' :-D Will fix it.

Thanks,

> 
> Regards,
> Markus


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

Re: [4/8] tracing: probe-events: Log errro for exceeding the number of arguments

[Markus Elfring]

>> May a bit of exception handling code be shared by an additional jump target?
>> Will another goto chain become helpful here?
>
> Honestly, I don't want to introduce jump any more.

Would you like to avoid any duplicate source code (including error handling)?

Regards,
Markus

Re: [PATCH 4/8] tracing: probe-events: Log errro for exceeding the number of arguments

[Masami Hiramatsu]

On Wed, 26 Feb 2025 15:19:10 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Add error message when the number of arguments exceeeds the limitation.
> 

Hmm, this is not a fix patch so this should be handled as enhancement.
I'll drop this from probes/fixes.

Thanks,

> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_eprobe.c |    2 ++
>  kernel/trace/trace_fprobe.c |    5 ++++-
>  kernel/trace/trace_kprobe.c |    5 ++++-
>  kernel/trace/trace_probe.h  |    1 +
>  kernel/trace/trace_uprobe.c |    9 +++++++--
>  5 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 82fd637cfc19..af9fa0632b57 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -913,6 +913,8 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  	}
>  
>  	if (argc - 2 > MAX_TRACE_ARGS) {
> +		trace_probe_log_set_index(2);
> +		trace_probe_log_err(0, TOO_MANY_ARGS);
>  		ret = -E2BIG;
>  		goto error;
>  	}
> diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> index e27305d31fc5..372936163c21 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -1201,8 +1201,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  		argc = new_argc;
>  		argv = new_argv;
>  	}
> -	if (argc > MAX_TRACE_ARGS)
> +	if (argc > MAX_TRACE_ARGS) {
> +		trace_probe_log_set_index(2);
> +		trace_probe_log_err(0, TOO_MANY_ARGS);
>  		return -E2BIG;
> +	}
>  
>  	ret = traceprobe_expand_dentry_args(argc, argv, &dbuf);
>  	if (ret)
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index d8d5f18a141a..8287b175667f 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -1007,8 +1007,11 @@ static int trace_kprobe_create_internal(int argc, const char *argv[],
>  		argc = new_argc;
>  		argv = new_argv;
>  	}
> -	if (argc > MAX_TRACE_ARGS)
> +	if (argc > MAX_TRACE_ARGS) {
> +		trace_probe_log_set_index(2);
> +		trace_probe_log_err(0, TOO_MANY_ARGS);
>  		return -E2BIG;
> +	}
>  
>  	ret = traceprobe_expand_dentry_args(argc, argv, &dbuf);
>  	if (ret)
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index c47ca002347a..6075afc8ea67 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -546,6 +546,7 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
>  	C(BAD_BTF_TID,		"Failed to get BTF type info."),\
>  	C(BAD_TYPE4STR,		"This type does not fit for string."),\
>  	C(NEED_STRING_TYPE,	"$comm and immediate-string only accepts string type"),\
> +	C(TOO_MANY_ARGS,	"Too many arguments are specified"),	\
>  	C(TOO_MANY_EARGS,	"Too many entry arguments specified"),
>  
>  #undef C
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index ccc762fbb69c..3386439ec9f6 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -562,8 +562,14 @@ static int __trace_uprobe_create(int argc, const char **argv)
>  
>  	if (argc < 2)
>  		return -ECANCELED;
> -	if (argc - 2 > MAX_TRACE_ARGS)
> +
> +	trace_probe_log_init("trace_uprobe", argc, argv);
> +
> +	if (argc - 2 > MAX_TRACE_ARGS) {
> +		trace_probe_log_set_index(2);
> +		trace_probe_log_err(0, TOO_MANY_ARGS);
>  		return -E2BIG;
> +	}
>  
>  	if (argv[0][1] == ':')
>  		event = &argv[0][2];
> @@ -582,7 +588,6 @@ static int __trace_uprobe_create(int argc, const char **argv)
>  		return -ECANCELED;
>  	}
>  
> -	trace_probe_log_init("trace_uprobe", argc, argv);
>  	trace_probe_log_set_index(1);	/* filename is the 2nd argument */
>  
>  	*arg++ = '\0';
> 


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