From: "Steven Rostedt (Google)" rostedt@goodmis.org
The variable $comm is hard coded as a string, which is true for both kprobes and uprobes, but for event probes (eprobes) it is a field name. In most cases the "comm" field would be a string, but there's no guarantee of that fact.
Do not assume that comm is a string. Not to mention, it currently forces comm fields to fault, as string processing for event probes is currently broken.
Cc: stable@vger.kernel.org Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events") Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org --- kernel/trace/trace_probe.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c index dec657af363c..23dcd52ad45c 100644 --- a/kernel/trace/trace_probe.c +++ b/kernel/trace/trace_probe.c @@ -622,9 +622,10 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
/* * Since $comm and immediate string can not be dereferenced, - * we can find those by strcmp. + * we can find those by strcmp. But ignore for eprobes. */ - if (strcmp(arg, "$comm") == 0 || strncmp(arg, "\"", 2) == 0) { + if (!(flags & TPARG_FL_TPOINT) && + strcmp(arg, "$comm") == 0 || strncmp(arg, "\"", 2) == 0) { /* The type of $comm must be "string", and not an array. */ if (parg->count || (t && strcmp(t, "string"))) goto out;
On Fri, 19 Aug 2022 21:40:37 -0400 Steven Rostedt rostedt@goodmis.org wrote:
+++ b/kernel/trace/trace_probe.c @@ -622,9 +622,10 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size, /* * Since $comm and immediate string can not be dereferenced,
* we can find those by strcmp.
* we can find those by strcmp. But ignore for eprobes.
- if (strcmp(arg, "$comm") == 0 || strncmp(arg, "\"", 2) == 0) {
- if (!(flags & TPARG_FL_TPOINT) &&
strcmp(arg, "$comm") == 0 || strncmp(arg, "\\\"", 2) == 0) {
And my tests fail shortly after I send this. It complains about a new warning. The above needs parenthesis around it.
Will send a v2 after my tests pass, in case it finds something else I missed.
-- Steve
/* The type of $comm must be "string", and not an array. */ if (parg->count || (t && strcmp(t, "string"))) goto out;--
On Fri, 19 Aug 2022 21:40:37 -0400 Steven Rostedt rostedt@goodmis.org wrote:
From: "Steven Rostedt (Google)" rostedt@goodmis.org
The variable $comm is hard coded as a string, which is true for both kprobes and uprobes, but for event probes (eprobes) it is a field name. In most cases the "comm" field would be a string, but there's no guarantee of that fact.
Do not assume that comm is a string. Not to mention, it currently forces comm fields to fault, as string processing for event probes is currently broken.
Indeed. There should be an event argument which names "comm". Eprobe might refer it. BTW, does eprobe use any special common fields? I originally introduced "$" variable for such special variables.
Thank you,
Cc: stable@vger.kernel.org Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events") Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org
kernel/trace/trace_probe.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c index dec657af363c..23dcd52ad45c 100644 --- a/kernel/trace/trace_probe.c +++ b/kernel/trace/trace_probe.c @@ -622,9 +622,10 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size, /* * Since $comm and immediate string can not be dereferenced,
* we can find those by strcmp.
* we can find those by strcmp. But ignore for eprobes.
- if (strcmp(arg, "$comm") == 0 || strncmp(arg, "\"", 2) == 0) {
- if (!(flags & TPARG_FL_TPOINT) &&
strcmp(arg, "$comm") == 0 || strncmp(arg, "\\\"", 2) == 0) {-- 2.35.1
On Sat, 20 Aug 2022 20:18:24 +0900 Masami Hiramatsu (Google) mhiramat@kernel.org wrote:
Do not assume that comm is a string. Not to mention, it currently forces comm fields to fault, as string processing for event probes is currently broken.
Indeed. There should be an event argument which names "comm". Eprobe might refer it. BTW, does eprobe use any special common fields? I originally introduced "$" variable for such special variables.
I used the '$' for denoting the fields, as it was the easiest way to integrate with trace_probe.c. There's no special variables, but this patch series now allows '@' as well as if $comm (or $COMM) is not a field, it acts the same as $comm for kprobes. Filtering and histograms do the same thing (use 'comm' as the event field, or has the current->comm if the event does not have 'comm' as a field). I should probably make "$common_comm" used too.
-- Steve
On Sat, 20 Aug 2022 08:48:37 -0400 Steven Rostedt rostedt@goodmis.org wrote:
I should probably make "$common_comm" used too.
My mistake. It was "common_cpu" not "common_comm". The filter and histogram just use "comm" or "COMM". I'll leave this as it.
-- Steve
On Sat, 20 Aug 2022 09:00:38 -0400 Steven Rostedt rostedt@goodmis.org wrote:
On Sat, 20 Aug 2022 08:48:37 -0400 Steven Rostedt rostedt@goodmis.org wrote:
I should probably make "$common_comm" used too.
My mistake. It was "common_cpu" not "common_comm". The filter and histogram just use "comm" or "COMM". I'll leave this as it.
Yeah, this is a bit confusing me. histogram allows common_cpu but filter allows only CPU. Shouldn't we unify those?
Thanks,
-- Steve
On Sat, 20 Aug 2022 22:09:20 +0900 Masami Hiramatsu (Google) mhiramat@kernel.org wrote:
My mistake. It was "common_cpu" not "common_comm". The filter and histogram just use "comm" or "COMM". I'll leave this as it.
Yeah, this is a bit confusing me. histogram allows common_cpu but filter allows only CPU. Shouldn't we unify those?
I can do that too.
-- Steve
linux-stable-mirror@lists.linaro.org
-
Masami Hiramatsu -
Steven Rostedt