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
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;
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;
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"), \
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(+)
…
+++ 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
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
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
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
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';
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(-)
…
+++ 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
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
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
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);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);- }
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);- }
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);- }
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';
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"
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"
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
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
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
linux-kselftest-mirror@lists.linaro.org
-
Markus Elfring -
Masami Hiramatsu -
Masami Hiramatsu (Google)
-
Steven Rostedt