Re: [PATCH v3 11/16] tracing/filters: Check for trailing characters after newline in filter string

On Wed, 28 Oct 2026 01:00:29 +0000, sashiko-bot@kernel.org [off-list ref] 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) [off-list ref]

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.

quoted

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) [off-list ref]