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

From: Masami Hiramatsu
Date: Wed Oct 06 2021 - 21:19:57 EST


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>