[PATCH] tracing/synthetic_events: Fix use when created by dynamic_events file

[PATCH] tracing/synthetic_events: Fix use when created by dynamic_events file

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The dynamic_events file can create kprobe, uprobe, event probes as well as
synthetic events. New dynamic events will also be created by this file.
Each of these kinds of events register a "create" function, that gets
called, and if the prefix does not match the type of event, the create
function is to return -ECANCELED to tell the dynamic event code that the
command does not belong to it, and other events should be tried.

The synthetic event does some format checking before it determines that it
is the event that should be created, and if that format check does not
match, it will return an error, telling the dynamic event code that it was
the expected event to be created and that the input had an error. This
returns an error code back to the user. But unfortunately, because it does
the check before it determines that it is indeed the proper event to parse
the input, it may fail the call even though the input is a proper syntax
for another event type.

Have it confirm that the input is for the synthetic event before it
returns an error due to parsing failure.

Fixes: c9e759b1e8456 ("tracing: Rework synthetic event command parsing")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_synth.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index d54094b7a9d7..feb87e5817e9 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -2045,11 +2045,17 @@ static int create_synth_event(const char *raw_command)
 {
 	char *fields, *p;
 	const char *name;
-	int len, ret = 0;
+	int len, ret;
 
 	raw_command = skip_spaces(raw_command);
 	if (raw_command[0] == '\0')
-		return ret;
+		return -ECANCELED;
+
+	name = raw_command;
+
+	if (name[0] != 's' || name[1] != ':')
+		return -ECANCELED;
+	name += 2;
 
 	last_cmd_set(raw_command);
 
@@ -2061,12 +2067,6 @@ static int create_synth_event(const char *raw_command)
 
 	fields = skip_spaces(p);
 
-	name = raw_command;
-
-	if (name[0] != 's' || name[1] != ':')
-		return -ECANCELED;
-	name += 2;
-
 	/* This interface accepts group name prefix */
 	if (strchr(name, '/')) {
 		len = str_has_prefix(name, SYNTH_SYSTEM "/");
-- 
2.31.1

Re: [PATCH] tracing/synthetic_events: Fix use when created by dynamic_events file

[Masami Hiramatsu]

On Wed, 6 Oct 2021 11:53:17 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> The dynamic_events file can create kprobe, uprobe, event probes as well as
> synthetic events. New dynamic events will also be created by this file.
> Each of these kinds of events register a "create" function, that gets
> called, and if the prefix does not match the type of event, the create
> function is to return -ECANCELED to tell the dynamic event code that the
> command does not belong to it, and other events should be tried.
> 
> The synthetic event does some format checking before it determines that it
> is the event that should be created, and if that format check does not
> match, it will return an error, telling the dynamic event code that it was
> the expected event to be created and that the input had an error. This
> returns an error code back to the user. But unfortunately, because it does
> the check before it determines that it is indeed the proper event to parse
> the input, it may fail the call even though the input is a proper syntax
> for another event type.
> 
> Have it confirm that the input is for the synthetic event before it
> returns an error due to parsing failure.
> 
> Fixes: c9e759b1e8456 ("tracing: Rework synthetic event command parsing")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  kernel/trace/trace_events_synth.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
> index d54094b7a9d7..feb87e5817e9 100644
> --- a/kernel/trace/trace_events_synth.c
> +++ b/kernel/trace/trace_events_synth.c
> @@ -2045,11 +2045,17 @@ static int create_synth_event(const char *raw_command)
>  {
>  	char *fields, *p;
>  	const char *name;
> -	int len, ret = 0;
> +	int len, ret;
>  
>  	raw_command = skip_spaces(raw_command);
>  	if (raw_command[0] == '\0')
> -		return ret;
> +		return -ECANCELED;

Good catch! BTW, I thought trace_parse_run_command() skips such empty line.

Anyway,

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> +
> +	name = raw_command;
> +
> +	if (name[0] != 's' || name[1] != ':')
> +		return -ECANCELED;
> +	name += 2;
>  
>  	last_cmd_set(raw_command);
>  
> @@ -2061,12 +2067,6 @@ static int create_synth_event(const char *raw_command)
>  
>  	fields = skip_spaces(p);
>  
> -	name = raw_command;
> -
> -	if (name[0] != 's' || name[1] != ':')
> -		return -ECANCELED;
> -	name += 2;
> -
>  	/* This interface accepts group name prefix */
>  	if (strchr(name, '/')) {
>  		len = str_has_prefix(name, SYNTH_SYSTEM "/");
> -- 
> 2.31.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] tracing/synthetic_events: Fix use when created by dynamic_events file

[Steven Rostedt]

On Thu, 7 Oct 2021 10:19:52 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> >  	raw_command = skip_spaces(raw_command);
> >  	if (raw_command[0] == '\0')
> > -		return ret;
> > +		return -ECANCELED;  
> 
> Good catch! BTW, I thought trace_parse_run_command() skips such empty line.

I think it does, but I was paranoid to remove that part, as this is
going to stable. As a clean up, we could remove that for the next merge
window.

> 
> Anyway,
> 
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!

-- Steve