Re: [PATCH RFC 7/7] tracing: Parse filter from event string

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `:mod:` suffix parsing using `strstr` is broken by the introduction of filter expressions, which can legitimately contain `:mod:` within string literals.
- [Medium] Unintended userspace ABI change for the debugfs `set_event` interface by exposing event string filter parsing.
--

--- Patch [7]: [PATCH RFC 7/7] tracing: Parse filter from event string ---
commit d99b1133b250a61ac85f36416d5a9f5e175d37f9
Author: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

tracing: Parse filter from event string

This commit makes it possible to specify a trace event filter from the kernel
command line by extracting it from the event string.

> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index c24929d003dd6..d4c46b24c3829 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1426,10 +1426,12 @@ static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
>  }
>  
>  VISIBLE_IF_KUNIT void
> -ftrace_parse_event_string(char *buf, char **match, char **sub, char **event, char **mod)
> +ftrace_parse_event_string(char *buf, char **match, char **sub, char **event, char **mod,
> +			  char **filter)
>  {
>  	*event = NULL;
>  	*sub = NULL;
> +	*filter = NULL;
>  
>  	/* Modules events can be appended with :mod:<module> */
>  	*mod = strstr(buf, ":mod:");

[Severity: High]
Does this code incorrectly match the module string inside the filter string?

If a filter expression is provided that legitimately contains the literal
string ":mod:" (for example, comm == "my:mod:app"), strstr() will match it
here before the buffer is tokenized. 

This could truncate the filter and falsely extract a module name, causing
event enablement to fail.

[ ... ]

> @@ -1471,7 +1476,7 @@ EXPORT_SYMBOL_IF_KUNIT(ftrace_parse_event_string);
>  
>  int ftrace_set_clr_event(struct trace_array *tr, const char *_buf, int set)
>  {
> -	char *event, *sub, *match, *mod;
> +	char *event, *sub, *match, *mod, *filter;
>  
>  	if (!tr)
>  		return -ENOENT;
> @@ -1480,9 +1485,9 @@ int ftrace_set_clr_event(struct trace_array *tr, const char *_buf, int set)
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	ftrace_parse_event_string(buf, &match, &sub, &event, &mod);
> +	ftrace_parse_event_string(buf, &match, &sub, &event, &mod, &filter);
>  
> -	return __ftrace_set_clr_event(tr, match, sub, event, set, mod, NULL);
> +	return __ftrace_set_clr_event(tr, match, sub, event, set, mod, filter);
>  }

[Severity: Medium]
Will this unintentionally alter the ABI for the set_event debugfs file?

Since ftrace_set_clr_event() is called by the debugfs set_event file write
handler, this change exposes the filter parsing to userspace writes. 

Writing strings with colons directly to set_event will now attempt to parse
them as filters, changing the expected ABI behavior and error semantics of
the interface. 

As mentioned in the patch notes, shouldn't this be limited to only apply to
the command line path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813-tracing-cli-event-filter-v1-0-57c4e8029c86@linutronix.de?part=7