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@xxxxxxxxxxx> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>

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@xxxxxxxxxx>