On Wed, 28 Oct 2026 01:00:29 +0000, [email protected]
<[email protected]> wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [Medium] tracing/filters: in-place truncation on error in
> filter_check_newline()
> --
>
> commit 8f43e3c8d713c8d0669d239d3455b033bcd6f4f2
> Author: Masami Hiramatsu (Google) <[email protected]>
>
> tracing/filters: Check for trailing characters after newline in filter string
>
> This commit introduces filter_check_newline() to truncate the filter string
> at the first newline and reject inputs that contain non-whitespace
> characters after that newline with -EINVAL.
>
> > diff --git a/kernel/trace/trace_events_filter.c
> > b/kernel/trace/trace_events_filter.c
> > index 9d1446464058a..f3c18324b07a8 100644
> > --- a/kernel/trace/trace_events_filter.c
> > +++ b/kernel/trace/trace_events_filter.c
> > @@ -2441,6 +2441,31 @@ static int process_system_preds(struct
> > trace_subsystem_dir *dir,
> > return -ENOMEM;
> > }
> >
> > +/*
> > + * Convert the first newline to '\0' and check that the rest of the input
> > + * contains no non-whitespace characters.
> > + */
> > +static int filter_check_newline(char *str)
> > +{
> > + char *p;
> > +
> > + if (!str)
> > + return 0;
> > +
> > + p = strchr(str, '\n');
> > + if (!p)
> > + return 0;
> > +
> > + *p++ = '\0';
> > + while (*p) {
> > + if (!isspace(*p))
> > + return -EINVAL;
> > + p++;
> > + }
> > +
> > + return 0;
> > +}
>
> [Severity: Medium]
> Should filter_check_newline() verify that all trailing characters are
> whitespace before writing '\0' to str?
>
> Because *p++ = '\0' mutates str in-place before the loop checks the remaining
> characters, returning -EINVAL leaves str truncated at the newline.
>
> When __ftrace_set_clr_event_nolock() in kernel/trace/trace_events.c iterates
> over matching events and passes the same filter buffer to
> apply_event_filter():
Oops, indeed...
>
> kernel/trace/trace_events.c:__ftrace_set_clr_event_nolock() {
> list_for_each_entry(file, &tr->events, list) {
> ...
> if (filter)
> ret = apply_event_filter(file, filter);
>
> if (!filter || !ret)
> ret = ftrace_event_enable_disable(file, set);
> }
> }
Hmm, IMHO, there is a bad implementation in __ftrace_set_clr_event_nolock().
It should make a working copy of the filter string and pass it to
apply_event_filter(). Or pass the filter string as `const char *`
so that it is copied in the apply_event_filter() side. The ownership
of the filter string is not clear here.
>
> a multi-line filter string with non-whitespace characters after the newline
> fails with -EINVAL on the first matching event, but truncates filter in-place.
> On subsequent matching events in the loop, filter_check_newline() no longer
> sees the newline and returns 0, causing the truncated filter to be applied and
> those events to be enabled.
As a better design, we need 2 level of applying filter, the first one is to
parse the filter into an abstract "filter" object, and try to apply it for
each event. Thus the pure syntax error is rejected in the first stage.
>
> Similarly, in trace_boot_init_one_event() in kernel/trace/trace_boot.c:
>
> else if (apply_event_filter(file, buf) < 0)
> pr_err("Failed to apply filter: %s\n", buf);
>
> the error message logs the truncated prefix in buf instead of the full
> invalid filter string.
Oops, this pr_err should be removed. We already have parse_error()
(and the parse_error also needs to be updated to support this case.)
Thanks,
--
Masami Hiramatsu (Google) <[email protected]>
