tracing/filters: Fix event filter parsing and matching bugs

Hi,

Here is the 4th version of fixes for various bugs in the trace event
filter parser and predicate evaluation in kernel/trace/trace_events_filter.c,
along with a new ftracetest test case covering filter syntax checks.

The previous version is here:
 https://lore.kernel.org/all/179314820431.684037.4147763044424284198.stgit@devnote2

In this version, according to the comment from Sashiko and Steve, dropped
a redundant patch and reorder the patches for separating real bug fixes
(1-8) and cleanups & test (9-15). Steve and I agreed that the patches which
just fixes the error message will be a cleanup or improvement, so I removed
Fixes/Cc:stable tags from those.

Changes in v4:
- Dropped the separate regex_match_end() zero-length string fix patch
  ([01/16] in v3) as redundant per review from Sashiko.
- In [01/15], introduce regex_match_end_dynstr() for FILTER_STATIC_STRING
  fields defined with size 0 (such as the 'buf' field of ftrace/print
  events / trace_marker), so that suffix filtering on unbounded NUL-terminated
  strings works as expected without penalizing fixed-size buffer matching.
- In [02/15], clean up process_system_preds() by replacing the 'fail'
  boolean with 'ret' and consolidating the cleanup logic between fail
  and fail_mem, as suggested by Steven Rostedt.
- In [09/15] (was [10/16]), verify that all characters following the
  newline are whitespace before writing '\0' in filter_check_newline(),
  preventing in-place truncation of the filter string on error, as
  pointed out by Sashiko.
- In [10/15] (was [08/16]), drop Fixes and Cc: stable tags, as this is an
  error log improvement cleanup rather than a bug fix.
  Also move it after other Cc: stable patches.
- In [11/15] to [14/15], drop Fixes and Cc: stable tags from error
  position fixes as these are cleanups.


Summary of the patches:

~~~~~~(fixes)~~~~~~
- [01/15] Introduce regex_match_end_str() for fixed-size string fields

76 more lines

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

regex_match_end() calculates string length as "len - 1", assuming @len
is the string length including trailing '\0'. For dynamic strings
(filter_pred_strloc) and pointer strings (filter_pchar), this assumption
holds and allows O(1) constant-time suffix matching.

However, for fixed-size string fields (FILTER_STATIC_STRING) and "comm"
(FILTER_COMM), the callers pass the buffer capacity rather than the
actual string length (field->size for filter_pred_string, and
TASK_COMM_LEN for filter_pred_comm). Because these buffers hold
null-terminated strings (e.g. prev_comm in sched_switch holding "bash"
in a 16-byte buffer), regex_match_end() tests against the tail of the
buffer capacity (typically trailing '\0' padding at offset 13) rather
than the actual string end. Consequently, suffix matching like
comm ~ "*sh" or prev_comm ~ "*sh" fails for all strings shorter than
the buffer capacity.

Furthermore, for trace events like ftrace/print (trace_marker), the
"buf" field is defined with size 0, denoting an unbounded NUL-terminated
string. regex_match_end() and regex_match_end_str() return 0 when len
is 0, causing suffix matching to fail on ftrace/print events as well.

To fix this without penalizing dynamic and pointer strings with an
unnecessary scan on every event:
- Introduce regex_match_end_str() using strnlen(str, len) for fixed-size
  buffers (FILTER_COMM and FILTER_STATIC_STRING with field->size > 0).
- Introduce regex_match_end_dynstr() using strlen(str) for unbounded
  strings (FILTER_STATIC_STRING with field->size == 0).

Assign these match callbacks in parse_pred() when MATCH_END_ONLY is
requested.

Fixes: a3291c14ecf0 ("tracing/filters: Fix MATCH_END_ONLY filter matching")
Fixes: 10f20e9f9da9 ("tracing: Have zero size length in filter logic be full string")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v4:
 - Drop the separate zero-length string fix patch as redundant per

12 more lines

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 5e74d2dae40a..28977d10e60b 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1083,6 +1083,30 @@ static int regex_match_end(char *str, struct regex *r, int len)
 	return 0;
 }
 
+static int regex_match_end_str(char *str, struct regex *r, int len)
+{
+	int slen;
+
+	if (len < r->len)
+		return 0;
+
+	slen = strnlen(str, len);
+	if (slen >= r->len &&
+	    memcmp(str + slen - r->len, r->pattern, r->len) == 0)
+		return 1;
+	return 0;
+}
+
+static int regex_match_end_dynstr(char *str, struct regex *r, int len)
+{
+	int slen = strlen(str);
+
+	if (slen >= r->len &&
+	    memcmp(str + slen - r->len, r->pattern, r->len) == 0)
+		return 1;
+	return 0;
+}
+
 static int regex_match_glob(char *str, struct regex *r, int len)
 {
 	return glob_match_len(r->pattern, str, len) ? 1 : 0;

@@ -2022,10 +2046,18 @@ static int parse_pred(const char *str, void *data,
 
 		if (field->filter_type == FILTER_COMM) {
 			pred->fn_num = FILTER_PRED_FN_COMM;
+			if (pred->regex->match == regex_match_end)
+				pred->regex->match = regex_match_end_str;
 
 		} else if (field->filter_type == FILTER_STATIC_STRING) {
 			pred->fn_num = FILTER_PRED_FN_STRING;
 			pred->regex->field_len = field->size;
+			if (pred->regex->match == regex_match_end) {
+				if (!field->size)
+					pred->regex->match = regex_match_end_dynstr;
+				else
+					pred->regex->match = regex_match_end_str;
+			}
 
 		} else if (field->filter_type == FILTER_DYN_STRING) {
 			pred->fn_num = FILTER_PRED_FN_STRLOC;

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In process_system_preds(), "fail" was initialized to true to indicate
whether any event in the subsystem successfully applied the filter.
However, "fail = false;" was placed at the end of the event loop outside
the error check, unconditionally clearing "fail" on every iteration even
when process_preds() returned an error.

As a result, if every event in the subsystem fails to match the filter
(e.g., an invalid field), process_system_preds() returns 0 (success)
instead of -EINVAL. Consequently, create_system_filter() considers the
operation successful and silently resets the subsystem filter display to
the default global filter message instead of reporting the parse error.

Replace "fail" with "ret", clearing it to 0 only when at least one event
filter succeeds. If "ret" is still non-zero after the loop, jump to
"fail" to report FILT_ERR_BAD_SUBSYS_FILTER and fall through to
"fail_mem". In "fail_mem", free any replaced filters via
delay_free_filter() if !list_empty(&filter_list->list), or via
free_filter_list() otherwise, consolidating the cleanup paths.

Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
Cc: <redacted>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v4:
 - Clean up process_system_preds() by replacing the 'fail' boolean with
   'ret' and consolidating the cleanup logic between fail and fail_mem,
   as suggested by Steven Rostedt.
---
 kernel/trace/trace_events_filter.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 28977d10e60b..b35ecb01697b 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -2337,7 +2337,7 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 	struct filter_list *filter_item;
 	struct event_filter *filter = NULL;
 	struct filter_head *filter_list;
-	bool fail = true;
+	int ret = -ENOMEM;
 	int err;
 
 	filter_list = kmalloc_obj(*filter_list);

@@ -2364,8 +2364,10 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 			filter_disable(file);
 			parse_error(pe, FILT_ERR_BAD_SUBSYS_FILTER, 0);
 			append_filter_err(tr, pe, filter);
-		} else
+		} else {
 			event_set_filtered_flag(file);
+			ret = 0;
+		}
 
 
 		filter_item = kzalloc_obj(*filter_item);

@@ -2380,11 +2382,9 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 		filter_item->filter = event_filter(file);
 		event_set_filter(file, filter);
 		filter = NULL;
-
-		fail = false;
 	}
 
-	if (fail)
+	if (ret)
 		goto fail;
 
 	/*

@@ -2396,19 +2396,18 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 	return 0;
  fail:
 	/* No call succeeded */
-	free_filter_list(filter_list);
 	parse_error(pe, FILT_ERR_BAD_SUBSYS_FILTER, 0);
-	return -EINVAL;
+	ret = -EINVAL;
  fail_mem:
 	__free_filter(filter);
 
-	/* If any call succeeded, we still need to sync */
-	if (!fail)
+	/* If any filter was replaced, we still need to sync */
+	if (!list_empty(&filter_list->list))
 		delay_free_filter(filter_list);
 	else
 		free_filter_list(filter_list);
 
-	return -ENOMEM;
+	return ret;
 }
 
 static int create_filter_start(char *filter_string, bool set_str,

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In filter_pred_##size##_cpumask(), if the event's scalar CPU value is
out of range (cpu >= nr_cpu_ids), it unconditionally returns 0.

However, if the predicate operation is OP_NE (e.g., field != CPUS{0,1}),
an out-of-range CPU cannot match any valid CPU in the user-provided
cpumask and the condition is satisfied. Returning 0 produces a false
negative.

Return "pred->op == OP_NE" instead of 0 when cpu >= nr_cpu_ids.

Fixes: 3cbec9d7b93d ("tracing/filters: Enable filtering a scalar field by a cpumask")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_events_filter.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index b35ecb01697b..3f0c73819365 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -759,7 +759,7 @@ static int filter_pred_##size##_cpumask(struct filter_pred *pred, void *event)	\
 	unsigned int cpu = *addr;						\
 										\
 	if (cpu >= nr_cpu_ids)							\
-		return 0;							\
+		return pred->op == OP_NE;					\
 										\
 	return do_filter_scalar_cpumask(pred->op, cpu, pred->mask);		\
 }

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Several error paths in filter parsing return -EINVAL (or -1) without
calling parse_error() to set pe->lasterr:

1. In parse_pred(), when an invalid token is encountered where a field name
   is expected (e.g., "common_pid == 1 || (common_pid == 2 && )"), len is 0
   and it returns -1 without calling parse_error().
2. In process_preds(), when !nr_preds (e.g., empty or whitespace-only
   filter), it returns -EINVAL before predicate_parse() can set
   FILT_ERR_NO_FILTER.
3. In predicate_parse(), when op_stack overflows or the jump target check
   fails, it returns -EINVAL without calling parse_error().

Because pe->lasterr remains 0 on these paths, append_filter_err() falls
back to printing an uninformative "Error: (0)" instead of reporting the
diagnostic and error position.

Call parse_error() with the appropriate error code on all of these paths.

Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_events_filter.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 3f0c73819365..b8d765ca9f1a 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -515,6 +515,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
 		switch (*next) {
 		case '(':					/* #2 */
 			if (top - op_stack > nr_parens) {
+				parse_error(pe, FILT_ERR_TOO_MANY_OPEN,
+					    next - str);
 				ret = -EINVAL;
 				goto out_free;
 			}

@@ -638,6 +640,7 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
 		prog[i].when_to_branch = invert;
 		/* Make sure the program always moves forward */
 		if (WARN_ON(prog[i].target <= i)) {
+			parse_error(pe, FILT_ERR_INVALID_FILTER, 0);
 			ret = -EINVAL;
 			goto out_free;
 		}

@@ -1709,8 +1712,10 @@ static int parse_pred(const char *str, void *data,
 
 	len = i - s;
 
-	if (!len)
-		return -1;
+	if (!len) {
+		parse_error(pe, FILT_ERR_INVALID_FILTER, pos + i);
+		return -EINVAL;
+	}
 
 	field_name = kmemdup_nul(str + s, len, GFP_KERNEL);
 	if (!field_name)

@@ -2306,8 +2311,10 @@ static int process_preds(struct trace_event_call *call,
 		return ret;
 	}
 
-	if (!nr_preds)
+	if (!nr_preds) {
+		parse_error(pe, FILT_ERR_NO_FILTER, 0);
 		return -EINVAL;
+	}
 
 	prog = predicate_parse(filter_string, nr_parens, nr_preds,
 			       parse_pred, call, pe);

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In parse_pred(), after matching the operator and skipping whitespace,
the filter string may terminate (str[i] == '\0') if the operand is
missing (e.g. "ip ==" or "common_pid ==").

For function events, the loop 'for (i++; str[i]; i++)' unconditionally
increments 'i' past the NUL terminator, causing an out-of-bounds read.
For other event types, allocating a predicate only to fail later during
value parsing is wasteful.

Check if 'str[i]' is NUL immediately after skipping whitespace after
the operator, and report FILT_ERR_INVALID_FILTER.

Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v3:
 - Split from the monolithic function event parsing patch.
 - Check if str[i] is NUL immediately after skipping whitespace after
   the operator in parse_pred() across all event types, as suggested
   by Steven Rostedt.
---
 kernel/trace/trace_events_filter.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index b8d765ca9f1a..a7a89e0f9b87 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1762,6 +1762,11 @@ static int parse_pred(const char *str, void *data,
 	while (isspace(str[i]))
 		i++;
 
+	if (!str[i]) {
+		parse_error(pe, FILT_ERR_INVALID_FILTER, pos + i);
+		goto err_free;
+	}
+
 	s = i;
 
 	pred = kzalloc_obj(*pred);

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In parse_pred(), when parsing a quoted operand for function events (e.g.
ip == "schedule"):
1. The loop 'for (i++; str[i]; i++)' breaks when str[i] matches the
   closing quote, but 'i' is never incremented past the closing quote.
   As a result, parse_pred() returns 'i' pointing at the closing quote,
   causing the caller predicate_parse() to immediately re-parse the
   closing quote as the start of the next token and fail with
   FILT_ERR_TOO_MANY_PREDS.
2. If the closing quote is missing (e.g. ip == "schedule), the loop stops
   at the NUL terminator without reporting an error.
3. The loop started with 'i++', which was intended to skip the opening
   quote, but also unconditionally skipped the first character of unquoted
   function operands.

Fix this by advancing 'i' and setting 's = i' past the opening quote
before the loop, starting the loop at 'i' without unconditional
increment, reporting FILT_ERR_MISSING_QUOTE if the closing quote is
missing, and advancing 'i' past the closing quote.

Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v3:
 - Split from the monolithic function event parsing patch.
 - Allow empty quoted operand ("") as valid syntax per Steven Rostedt.
---
 kernel/trace/trace_events_filter.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index a7a89e0f9b87..05579895c810 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1859,22 +1859,27 @@ static int parse_pred(const char *str, void *data,
 		 * Quotes are not required, but if they exist then we need
 		 * to read them till we hit a matching one.
 		 */
-		if (str[i] == '\'' || str[i] == '"')
-			q = str[i];
-		else
+		if (str[i] == '\'' || str[i] == '"') {
+			q = str[i++];
+			s = i;
+		} else {
 			q = 0;
+		}
 
-		for (i++; str[i]; i++) {
+		for (; str[i]; i++) {
 			if (q && str[i] == q)
 				break;
 			if (!q && (str[i] == ')' || str[i] == '&' ||
 				   str[i] == '|'))
 				break;
 		}
-		/* Skip quotes */
-		if (q)
-			s++;
+		if (q && !str[i]) {
+			parse_error(pe, FILT_ERR_MISSING_QUOTE, pos + i);
+			goto err_free;
+		}
 		len = i - s;
+		if (q)
+			i++;
 		if (len >= MAX_FILTER_STR_VAL) {
 			parse_error(pe, FILT_ERR_OPERAND_TOO_LONG, pos + i);
 			goto err_free;

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In parse_pred(), when parsing an unquoted operand for function events,
if the filter expression has no operand before a delimiter (e.g.
'(ip == )' or '(ip == && x == 1)'), the loop terminates with len == 0.

While an empty quoted string (e.g. 'ip == ""') is syntactically valid
and denotes an empty pattern, an unquoted empty operand is a syntax
error where an operand was expected before the delimiter. Previously,
this accepted the predicate with an empty pattern.

Reject empty unquoted operands by checking '!q && !len' and reporting
FILT_ERR_INVALID_VALUE.

Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v3:
 - Split from the monolithic function event parsing patch into a
   standalone patch rejecting empty unquoted operands (!q && !len).
---
 kernel/trace/trace_events_filter.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 05579895c810..a645af9eb1f8 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1880,6 +1880,10 @@ static int parse_pred(const char *str, void *data,
 		len = i - s;
 		if (q)
 			i++;
+		if (!q && !len) {
+			parse_error(pe, FILT_ERR_INVALID_VALUE, pos + i);
+			goto err_free;
+		}
 		if (len >= MAX_FILTER_STR_VAL) {
 			parse_error(pe, FILT_ERR_OPERAND_TOO_LONG, pos + i);
 			goto err_free;

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Prior to commit 80765597bc58 ("tracing: Rewrite filter logic to be simpler
and faster"), parse_pred() checked whether select_comparison_fn() returned
NULL and failed with FILT_ERR_INVALID_OP and -EINVAL. That check was lost
during the rewrite, and commit 3cbec9d7b93d ("tracing/filters: Enable
filtering a scalar field by a cpumask") similarly omitted a default error
case when matching field->size for CPUS{} comparisons.

If a non-string field (FILTER_OTHER) has a size other than 1, 2, 4, or 8
(such as a fixed-size integer array field), pred->fn_num remains
FILTER_PRED_FN_NOP (0) and parse_pred() returns success. Subsequently,
update_preds() rejects FILTER_PRED_FN_NOP via WARN_ON_ONCE() and returns
-EINVAL without setting a parse_error() message, triggering a kernel
warning from user-supplied filter input.

Restore the check for FILTER_PRED_FN_NOP in both the numeric scalar and
CPUS{} branches of parse_pred() and report FILT_ERR_INVALID_OP.

Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
Fixes: 3cbec9d7b93d ("tracing/filters: Enable filtering a scalar field by a cpumask")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_events_filter.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index a645af9eb1f8..40efcef94e02 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -2013,6 +2013,10 @@ static int parse_pred(const char *str, void *data,
 				break;
 			}
 		}
+		if (pred->fn_num == FILTER_PRED_FN_NOP) {
+			parse_error(pe, FILT_ERR_INVALID_OP, pos + i);
+			goto err_free;
+		}
 
 	/* This is either a string, or an integer */
 	} else if (str[i] == '\'' || str[i] == '"') {

@@ -2166,6 +2170,10 @@ static int parse_pred(const char *str, void *data,
 		else {
 			pred->fn_num = select_comparison_fn(pred->op, field->size,
 							    field->is_signed);
+			if (pred->fn_num == FILTER_PRED_FN_NOP) {
+				parse_error(pe, FILT_ERR_INVALID_OP, pos + i);
+				goto err_free;
+			}
 			if (pred->op == OP_NE)
 				pred->not = 1;
 		}

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Each trace event or subsystem only holds a single filter, and
append_filter_err() / tracing_log_err() caret placement assumes a
single-line filter string. However, apply_event_filter() and
apply_subsystem_event_filter() only called strstrip(), which strips
trailing newlines but leaves embedded newlines intact.

An embedded newline in a filter string (for example inside 'CPUS{0\n"}')
causes cpulist_parse() / bitmap_parselist() to treat '\n' as the end of
the cpulist and ignore trailing characters before '}', desynchronizing
quote tracking between calc_stack() and parse_pred() and misaligning
error_log caret reporting.

Introduce an internal function filter_check_newline() to convert the
first newline to NUL and check that the rest of the input contains no
non-whitespace characters. If any non-whitespace character follows the
newline, report it as an invalid input error (-EINVAL) without modifying
the input string in-place.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v4:
 - Verify all characters following the newline are whitespace before
   writing '\0' in filter_check_newline(), preventing in-place truncation
   on error, as pointed out by Sashiko.
Changes in v3:
 - Introduce filter_check_newline() to convert the first newline to
   NUL and reject any non-whitespace characters after the newline with
   an invalid input error (-EINVAL).
---
 kernel/trace/trace_events_filter.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 40efcef94e02..0a362ccb14ad 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -2439,6 +2439,30 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 	return ret;
 }
 
+/*
+ * Check that the rest of the input after the first newline contains no
+ * non-whitespace characters, and convert the newline to '\0'.
+ */
+static int filter_check_newline(char *str)
+{
+	char *nl, *p;
+
+	if (!str)
+		return 0;
+
+	nl = strchr(str, '\n');
+	if (!nl)
+		return 0;
+
+	for (p = nl + 1; *p; p++) {
+		if (!isspace(*p))
+			return -EINVAL;
+	}
+
+	*nl = '\0';
+	return 0;
+}
+
 static int create_filter_start(char *filter_string, bool set_str,
 			       struct filter_parse_error **pse,
 			       struct event_filter **filterp)

@@ -2450,6 +2474,10 @@ static int create_filter_start(char *filter_string, bool set_str,
 	if (WARN_ON_ONCE(*pse || *filterp))
 		return -EINVAL;
 
+	err = filter_check_newline(filter_string);
+	if (err)
+		return err;
+
 	filter = kzalloc_obj(*filter);
 	if (filter && set_str) {
 		filter->filter_string = kstrdup(filter_string, GFP_KERNEL);

@@ -2568,6 +2596,10 @@ int apply_event_filter(struct trace_event_file *file, char *filter_string)
 	if (file->flags & EVENT_FILE_FL_FREED)
 		return -ENODEV;
 
+	err = filter_check_newline(filter_string);
+	if (err)
+		return err;
+
 	if (!strcmp(strstrip(filter_string), "0")) {
 		filter_disable(file);
 		filter = event_filter(file);

@@ -2622,6 +2654,10 @@ int apply_subsystem_event_filter(struct trace_subsystem_dir *dir,
 	if (!dir->nr_events)
 		return -ENODEV;
 
+	err = filter_check_newline(filter_string);
+	if (err)
+		return err;
+
 	if (!strcmp(strstrip(filter_string), "0")) {
 		filter_free_subsystem_preds(dir, tr);
 		remove_filter_string(system->filter);

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In parse_pred(), when parsing an unquoted operand for function events,
the loop only stops at ')', '&', '|', or the NUL terminator.

However, calc_stack() parses '(', '\'', and '"' independently to
determine stack depth and string boundaries. If an unquoted function
operand contains stray quotes or an open parenthesis (e.g. "ip == foo'bar"
or "ip == foo(bar)"), parse_pred() consumes them as part of the function
name operand, causing parse_pred() and calc_stack() to desynchronize.

Reject unquoted function event operands containing '(', '\'', or '"'
by reporting FILT_ERR_INVALID_VALUE.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v4:
 - Drop Fixes and Cc: stable tags since this is an error log improvement
   cleanup rather than a bug fix, as pointed out by Steven Rostedt.
Changes in v3:
 - Split from the monolithic function event parsing patch into a
   standalone patch rejecting stray quotes and '(' in unquoted operands.
---
 kernel/trace/trace_events_filter.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 0a362ccb14ad..e91ec41bfd2a 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1869,9 +1869,17 @@ static int parse_pred(const char *str, void *data,
 		for (; str[i]; i++) {
 			if (q && str[i] == q)
 				break;
-			if (!q && (str[i] == ')' || str[i] == '&' ||
-				   str[i] == '|'))
-				break;
+			if (!q) {
+				if (str[i] == ')' || str[i] == '&' ||
+				    str[i] == '|')
+					break;
+				if (str[i] == '(' || str[i] == '\'' ||
+				    str[i] == '"') {
+					parse_error(pe, FILT_ERR_INVALID_VALUE,
+						    pos + i);
+					goto err_free;
+				}
+			}
 		}
 		if (q && !str[i]) {
 			parse_error(pe, FILT_ERR_MISSING_QUOTE, pos + i);

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In predicate_parse(), 'ptr' has already been advanced past 'next',
shifting 'pos' passed to parse_pred() one character to the right.

In parse_pred(), several error paths report 'pos + i' after 'i' has
already been advanced to subsequent tokens:
- When a field is not found (FILT_ERR_FIELD_NOT_FOUND) or when a
  function event field is not "ip" (FILT_ERR_IP_FIELD_ONLY), 'pos + i'
  points past the field name or to the operand instead of the field.
- In operator validation checks for string and numeric operands, and
  when select_comparison_fn() returns NOP, 'pos + i' points to the
  operand or past it instead of the unsupported operator.

Fix these by passing 'next - str' in predicate_parse(), saving the
field position in 'field_pos' and the operator position in 'op_pos', and
passing 'pos + field_pos' or 'pos + op_pos' accordingly. Also update the
tracing-error-log.tc selftest to reflect the corrected error position.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v4:
 - Drop Fixes and Cc: stable tags since this error position fix is a
   cleanup.
Changes in v3:
 - Group all error position fixes for commit 80765597bc58 together into
   a single patch for ease of backporting.
---
 kernel/trace/trace_events_filter.c                 |   19 ++++++++++++-------
 .../ftrace/test.d/ftrace/tracing-error-log.tc      |    2 +-
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index e91ec41bfd2a..6ff208f0e033 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -537,7 +537,7 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
 		inverts[N] = invert;				/* #4 */
 		prog[N].target = N-1;
 
-		len = parse_pred(next, data, ptr - str, pe, &prog[N].pred);
+		len = parse_pred(next, data, next - str, pe, &prog[N].pred);
 		if (len < 0) {
 			ret = len;
 			goto out_free;

@@ -1700,12 +1700,15 @@ static int parse_pred(const char *str, void *data,
 	int ret;
 	int op;
 	int s;
+	int field_pos = 0;
+	int op_pos = 0;
 	int i = 0;
 
 	/* First find the field to associate to */
 	while (isspace(str[i]))
 		i++;
 	s = i;
+	field_pos = i;
 
 	while (isalnum(str[i]) || str[i] == '_')
 		i++;

@@ -1726,7 +1729,7 @@ static int parse_pred(const char *str, void *data,
 	field = trace_find_event_field(call, field_name);
 	kfree(field_name);
 	if (!field) {
-		parse_error(pe, FILT_ERR_FIELD_NOT_FOUND, pos + i);
+		parse_error(pe, FILT_ERR_FIELD_NOT_FOUND, pos + field_pos);
 		return -EINVAL;
 	}
 

@@ -1745,6 +1748,8 @@ static int parse_pred(const char *str, void *data,
 	while (isspace(str[i]))
 		i++;
 
+	op_pos = i;
+
 	/* Make sure this op is supported */
 	for (op = 0; ops[op]; op++) {
 		/* This is why '<=' must come before '<' in ops[] */

@@ -1753,7 +1758,7 @@ static int parse_pred(const char *str, void *data,
 	}
 
 	if (!ops[op]) {
-		parse_error(pe, FILT_ERR_INVALID_OP, pos + i);
+		parse_error(pe, FILT_ERR_INVALID_OP, pos + op_pos);
 		goto err_free;
 	}
 

@@ -1850,7 +1855,7 @@ static int parse_pred(const char *str, void *data,
 		 * (perf doesn't use it) and grab everything.
 		 */
 		if (strcmp(field->name, "ip") != 0) {
-			parse_error(pe, FILT_ERR_IP_FIELD_ONLY, pos + i);
+			parse_error(pe, FILT_ERR_IP_FIELD_ONLY, pos + field_pos);
 			goto err_free;
 		}
 		pred->fn_num = FILTER_PRED_FN_NOP;

@@ -2039,7 +2044,7 @@ static int parse_pred(const char *str, void *data,
 		case OP_EQ:
 			break;
 		default:
-			parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + i);
+			parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + op_pos);
 			goto err_free;
 		}
 

@@ -2140,7 +2145,7 @@ static int parse_pred(const char *str, void *data,
 		}
 
 		if (op == OP_GLOB) {
-			parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + i);
+			parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + op_pos);
 			goto err_free;
 		}
 

@@ -2179,7 +2184,7 @@ static int parse_pred(const char *str, void *data,
 			pred->fn_num = select_comparison_fn(pred->op, field->size,
 							    field->is_signed);
 			if (pred->fn_num == FILTER_PRED_FN_NOP) {
-				parse_error(pe, FILT_ERR_INVALID_OP, pos + i);
+				parse_error(pe, FILT_ERR_INVALID_OP, pos + op_pos);
 				goto err_free;
 			}
 			if (pred->op == OP_NE)

diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/tracing-error-log.tc b/tools/testing/selftests/ftrace/test.d/ftrace/tracing-error-log.tc
index 6c190620db47..2c900873ce3c 100644
--- a/tools/testing/selftests/ftrace/test.d/ftrace/tracing-error-log.tc
+++ b/tools/testing/selftests/ftrace/test.d/ftrace/tracing-error-log.tc

@@ -10,6 +10,6 @@ fail() { #msg
     exit_fail
 }
 
-ftrace_errlog_check 'event filter parse error' '((sig >= 10 && sig < 15) || dsig ^== 17) && comm != bash' 'events/signal/signal_generate/filter'
+ftrace_errlog_check 'event filter parse error' '((sig >= 10 && sig < 15) || ^dsig == 17) && comm != bash' 'events/signal/signal_generate/filter'
 
 exit 0

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In parse_pred(), by the time the '.function' field size and operator
checks are reached, 'i' has already been advanced past the operator to
the operand.

Save the position of the '.function' modifier in 'fn_pos' and pass
'pos + fn_pos' for ILLEGAL_FIELD_OP and 'pos + op_pos' for INVALID_OP
so the error caret points to '.function' and the operator respectively.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v4:
 - Drop Fixes and Cc: stable tags since this error position fix is a
   cleanup.
Changes in v3:
 - Newly added to fix error positions for .function modifier and
   operator checks in parse_pred().
---
 kernel/trace/trace_events_filter.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 6ff208f0e033..9b1100769676 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1701,6 +1701,7 @@ static int parse_pred(const char *str, void *data,
 	int op;
 	int s;
 	int field_pos = 0;
+	int fn_pos = 0;
 	int op_pos = 0;
 	int i = 0;
 

@@ -1742,6 +1743,7 @@ static int parse_pred(const char *str, void *data,
 	/* See if the field is a kernel function name */
 	if ((len = str_has_prefix(str + i, ".function"))) {
 		function = true;
+		fn_pos = i;
 		i += len;
 	}
 

@@ -1785,7 +1787,7 @@ static int parse_pred(const char *str, void *data,
 	if (function) {
 		/* The field must be the same size as long */
 		if (field->size != sizeof(long)) {
-			parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + i);
+			parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + fn_pos);
 			goto err_free;
 		}
 

@@ -1795,7 +1797,7 @@ static int parse_pred(const char *str, void *data,
 		case OP_EQ:
 			break;
 		default:
-			parse_error(pe, FILT_ERR_INVALID_OP, pos + i);
+			parse_error(pe, FILT_ERR_INVALID_OP, pos + op_pos);
 			goto err_free;
 		}
 

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In parse_pred(), CPUS mask error paths report incorrect error positions:
- Checking 'str[i++] != '\'{'\'' post-increments 'i' before calling
  parse_error(), shifting the error caret one character past the
  missing brace position.
- In the CPUS operator check and comparison function selection check,
  'pos + i' points to the CPUS operand or past it instead of the
  unsupported operator.

Check 'str[i] != '\'{'\'' before incrementing 'i', and pass 'pos + op_pos'
for the operator checks so the error caret points accurately.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v4:
 - Drop Fixes and Cc: stable tags since these error position fixes are
   cleanups.
Changes in v3:
 - Group error position fixes for CPUS mask parsing into a single patch
   targeting commit 39f7c41c908b and commit 3cbec9d7b93d.
---
 kernel/trace/trace_events_filter.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 9b1100769676..1a9a1cc8daa2 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1932,16 +1932,17 @@ static int parse_pred(const char *str, void *data,
 		case OP_BAND:
 			break;
 		default:
-			parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + i);
+			parse_error(pe, FILT_ERR_ILLEGAL_FIELD_OP, pos + op_pos);
 			goto err_free;
 		}
 
 		/* Skip CPUS */
 		i += 4;
-		if (str[i++] != '{') {
+		if (str[i] != '{') {
 			parse_error(pe, FILT_ERR_MISSING_BRACE_OPEN, pos + i);
 			goto err_free;
 		}
+		i++;
 		maskstart = i;
 
 		/* Walk the cpulist until closing } */

@@ -2029,7 +2030,7 @@ static int parse_pred(const char *str, void *data,
 			}
 		}
 		if (pred->fn_num == FILTER_PRED_FN_NOP) {
-			parse_error(pe, FILT_ERR_INVALID_OP, pos + i);
+			parse_error(pe, FILT_ERR_INVALID_OP, pos + op_pos);
 			goto err_free;
 		}
 

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In parse_pred(), when the '~' operator is used with a 'within' filter,
'pos + i' points to the closing quote of the operand rather than to
the unsupported '~' operator.

Pass 'pos + op_pos' so the error caret points to the invalid operator.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v4:
 - Drop Fixes and Cc: stable tags since this error position fix is a
   cleanup.
Changes in v3:
 - Newly added to fix error position for within filter operator check.
---
 kernel/trace/trace_events_filter.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 1a9a1cc8daa2..1a50de4e7e8d 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -2106,7 +2106,7 @@ static int parse_pred(const char *str, void *data,
 			unsigned long func;
 
 			if (op == OP_GLOB) {
-				parse_error(pe, FILT_ERR_GLOB_WITHIN, pos + i);
+				parse_error(pe, FILT_ERR_GLOB_WITHIN, pos + op_pos);
 				goto err_free;
 			}
 

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Add a new ftracetest test case (filter-syntax-check.tc) to verify valid
event filter expressions, error_log caret reporting for malformed filter
expressions (empty filters, missing fields, unbalanced parentheses and
quotes, dangling logical operators, invalid fields/values/cpulists),
newline truncation on multi-line filter input, and error returns on
invalid subsystem filters.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v3:
 - Replace negated pipelines (! cmd) with explicit
   "if cmd; then exit_fail; fi" checks to properly catch unexpected success
   under POSIX sh with set -e.
 - Add test cases for empty string matching (prev_comm == "",
   prev_comm ~ "").
 - Add test cases for out-of-range CPU IDs with CPUS{} comparison.
 - Add test cases for missing operands after operators.
 - Add test cases for empty operands and stray operators.
 - Add test cases for operand type mismatches and brace errors.
 - Add test cases for function event filter syntax checks (including
   INVALID_OP and ILLEGAL_FIELD_OP caret checks).
 - Add test cases for multi-line input validation on event and subsystem
   filters.
---
 .../ftrace/test.d/filter/filter-syntax-check.tc    |  100 ++++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/filter/filter-syntax-check.tc

diff --git a/tools/testing/selftests/ftrace/test.d/filter/filter-syntax-check.tc b/tools/testing/selftests/ftrace/test.d/filter/filter-syntax-check.tc
new file mode 100644
index 000000000000..02fb2be6e6eb
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/filter/filter-syntax-check.tc

@@ -0,0 +1,100 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: event filter - test filter syntax error check
+# requires: set_event events/sched/sched_switch/filter error_log
+# flags: instance
+
+check_error() { # command-with-error-pos-by-^
+    ftrace_errlog_check 'event filter parse error' "$1" 'events/sched/sched_switch/filter'
+}
+
+# Valid filter expressions
+echo '(common_pid == 0)' > events/sched/sched_switch/filter
+echo '((common_pid == 0) && (prev_pid == 0))' > events/sched/sched_switch/filter
+echo 'prev_comm ~ "*sh"' > events/sched/sched_switch/filter
+echo 'prev_comm ~ "*"' > events/sched/sched_switch/filter
+echo 'prev_comm == ""' > events/sched/sched_switch/filter
+echo 'prev_comm ~ ""' > events/sched/sched_switch/filter
+echo 'common_pid == CPUS{0}' > events/sched/sched_switch/filter
+echo 'common_pid != CPUS{0}' > events/sched/sched_switch/filter
+echo 0 > events/sched/sched_switch/filter
+
+# Empty or meaningless filter expressions
+check_error '^()'						# NO_FILTER
+check_error '^== 1'						# INVALID_FILTER
+check_error 'common_pid ==^'					# INVALID_FILTER
+check_error 'common_pid !=^'					# INVALID_FILTER
+check_error 'common_pid >=^'					# INVALID_FILTER
+
+# Unbalanced parentheses and quotes
+check_error '^(common_pid == 1'					# TOO_MANY_OPEN
+check_error 'common_pid == 1^)'					# TOO_MANY_CLOSE
+check_error 'prev_comm == ^"bash'				# MISSING_QUOTE
+
+# Dangling logical operators and empty operands
+check_error 'common_pid == 1 &&^'				# TOO_MANY_PREDS
+check_error 'common_pid == 1 ||^'				# TOO_MANY_PREDS
+check_error 'common_pid == 1 || (common_pid == 2 && ^)'		# INVALID_FILTER
+check_error '(common_pid == ^)'					# INVALID_VALUE
+check_error 'common_pid == ^&& common_pid == 0'			# INVALID_VALUE
+
+# Invalid field, operator, or operand type
+check_error '^no_such_field == 1'				# FIELD_NOT_FOUND
+check_error 'common_pid == ^@'					# INVALID_VALUE
+check_error 'common_pid == ^"1"'				# EXPECT_DIGIT
+check_error 'prev_comm == ^0'					# EXPECT_STRING
+check_error 'common_pid ^~ 1'					# ILLEGAL_FIELD_OP
+
+# CPUS{} syntax errors
+check_error 'common_pid ^> CPUS{0}'				# ILLEGAL_FIELD_OP
+check_error 'common_pid == CPUS^0}'				# MISSING_BRACE_OPEN
+check_error 'common_pid == CPUS{0^'				# MISSING_BRACE_CLOSE
+check_error 'common_pid == CPUS{^}'				# INVALID_CPULIST
+check_error 'common_pid == CPUS{99999^}'			# INVALID_CPULIST
+
+# Function event filter errors (if supported)
+if [ -f events/kmem/kmem_cache_free/filter ]; then
+    check_kmem_error() {
+        ftrace_errlog_check 'event filter parse error' "$1" 'events/kmem/kmem_cache_free/filter'
+    }
+    check_kmem_error 'call_site.function == non_exist_func^'	# NO_FUNCTION
+    check_kmem_error 'call_site.function ==^'			# INVALID_FILTER
+    check_kmem_error 'call_site.function ^> 0'			# INVALID_OP
+    check_kmem_error 'common_type^.function == 0'		# ILLEGAL_FIELD_OP
+    echo 0 > events/kmem/kmem_cache_free/filter
+fi
+
+# Multi-line input with non-whitespace characters after newline must fail
+if ( printf "common_pid == 0\ncommon_pid == 1" | \
+     dd of=events/sched/sched_switch/filter bs=4096 ) 2> /dev/null; then
+    exit_fail
+fi
+if ( printf "common_pid == CPUS{0\n\"} && common_pid == 0" | \
+     dd of=events/sched/sched_switch/filter bs=4096 ) 2> /dev/null; then
+    exit_fail
+fi
+
+# Trailing whitespace characters after newline are permitted
+printf "common_pid == 0\n  \n\t \n" | \
+    dd of=events/sched/sched_switch/filter bs=4096 2> /dev/null
+echo 0 > events/sched/sched_switch/filter
+
+# Subsystem filter must fail on invalid filter expressions
+echo 'common_pid >= 0' > events/sched/filter
+if echo 'no_such_field == 1' > events/sched/filter 2> /dev/null; then
+    exit_fail
+fi
+if echo 'common_pid == 1 &&' > events/sched/filter 2> /dev/null; then
+    exit_fail
+fi
+if ( printf "common_pid == 0\nfoo" | \
+     dd of=events/sched/filter bs=4096 ) 2> /dev/null; then
+    exit_fail
+fi
+
+# Subsystem filter with trailing whitespace after newline must succeed
+printf "common_pid == 0\n  \n\t \n" | \
+    dd of=events/sched/filter bs=4096 2> /dev/null
+echo 0 > events/sched/filter
+
+exit 0

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tracing/filters: process_system_preds() returns 0 on ENOMEM after match
--

commit a15564919ee696f8c1b9df5aeee0a37935f51db7
Author: Masami Hiramatsu (Google) [off-list ref]

tracing/filters: Fix false success return in process_system_preds()

This commit replaces the boolean fail flag in process_system_preds() with
ret so that process_system_preds() returns -EINVAL when no event in the
subsystem matches the filter. It also consolidates the fail and fail_mem
error cleanup paths.

19 more quoted lines

[Severity: Medium]
Can process_system_preds() return 0 instead of -ENOMEM when an allocation
fails after at least one event matches the filter?

In kernel/trace/trace_events_filter.c:process_system_preds(), ret is
initialized to -ENOMEM and cleared to 0 as soon as process_preds() succeeds
for an event:

	err = process_preds(file->event_call, filter_string, filter, pe);
	if (err) {
		...
	} else {
		event_set_filtered_flag(file);
		ret = 0;
	}

	filter_item = kzalloc_obj(*filter_item);
	if (!filter_item)
		goto fail_mem;

If kzalloc_obj(*filter_item) fails on that same iteration, or if
kzalloc_obj(*filter), kstrdup(), or kzalloc_obj(*filter_item) fails on a
subsequent event in the loop, jumping to fail_mem returns ret (which is now
0) instead of -ENOMEM.

That causes create_system_filter() and apply_subsystem_event_filter() to
treat the aborted update as a success, leaving the remaining events in the
subsystem without the new filter applied. Also, if kzalloc_obj(*filter_item)
fails right after event_set_filtered_flag(file) runs, EVENT_FILE_FL_FILTERED
is set and trace_buffered_event_enable() is called, while fail_mem frees the
new filter without installing it via event_set_filter(file, filter).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179323784538.740114.15323117974242744566.stgit@devnote2?part=2

On Thu, 29 Oct 2026 01:45:53 +0000, sashiko-bot@kernel.org [off-list ref] wrote:

[Severity: Medium]
Can process_system_preds() return 0 instead of -ENOMEM when an allocation
fails after at least one event matches the filter?

In kernel/trace/trace_events_filter.c:process_system_preds(), ret is
initialized to -ENOMEM and cleared to 0 as soon as process_preds() succeeds
for an event:

	err = process_preds(file->event_call, filter_string, filter, pe);
	if (err) {
		...
	} else {
		event_set_filtered_flag(file);
		ret = 0;
	}

	filter_item = kzalloc_obj(*filter_item);
	if (!filter_item)
		goto fail_mem;

If kzalloc_obj(*filter_item) fails on that same iteration, or if
kzalloc_obj(*filter), kstrdup(), or kzalloc_obj(*filter_item) fails on a
subsequent event in the loop, jumping to fail_mem returns ret (which is now
0) instead of -ENOMEM.

That causes create_system_filter() and apply_subsystem_event_filter() to
treat the aborted update as a success, leaving the remaining events in the
subsystem without the new filter applied. Also, if kzalloc_obj(*filter_item)
fails right after event_set_filtered_flag(file) runs, EVENT_FILE_FL_FILTERED
is set and trace_buffered_event_enable() is called, while fail_mem frees the
new filter without installing it via event_set_filter(file, filter).

Oops, right. Hmm, it is better to set ENOMEM for each goto fail_mem...

Thanks,

-- 
Masami Hiramatsu (Google) [off-list ref]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

In process_system_preds(), "fail" was initialized to true to indicate
whether any event in the subsystem successfully applied the filter.
However, "fail = false;" was placed at the end of the event loop outside
the error check, unconditionally clearing "fail" on every iteration even
when process_preds() returned an error.

As a result, if every event in the subsystem fails to match the filter
(e.g., an invalid field), process_system_preds() returns 0 (success)
instead of -EINVAL. Consequently, create_system_filter() considers the
operation successful and silently resets the subsystem filter display to
the default global filter message instead of reporting the parse error.

Fix this by clearing "fail" only when at least one event filter succeeds.
Consolidate the cleanup logic in fail and fail_mem by using ret
initialized to -ENOMEM, setting ret to -EINVAL in fail, and falling
through to fail_mem, freeing any replaced filters via delay_free_filter()
if !list_empty(&filter_list->list).

Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
Cc: <redacted>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v4.1:
 - Recover 'fail' flag instead of clearing 'ret'.
Changes in v4:
 - Clean up process_system_preds() by replacing the 'fail' boolean with
   'ret' and consolidating the cleanup logic between fail and fail_mem,
   as suggested by Steven Rostedt.
---
 kernel/trace/trace_events_filter.c |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 28977d10e60b..981cddf01e65 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -2338,6 +2338,7 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 	struct event_filter *filter = NULL;
 	struct filter_head *filter_list;
 	bool fail = true;
+	int ret = -ENOMEM;
 	int err;
 
 	filter_list = kmalloc_obj(*filter_list);

@@ -2364,9 +2365,10 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 			filter_disable(file);
 			parse_error(pe, FILT_ERR_BAD_SUBSYS_FILTER, 0);
 			append_filter_err(tr, pe, filter);
-		} else
+		} else {
 			event_set_filtered_flag(file);
-
+			fail = false;
+		}
 
 		filter_item = kzalloc_obj(*filter_item);
 		if (!filter_item)

@@ -2380,8 +2382,6 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 		filter_item->filter = event_filter(file);
 		event_set_filter(file, filter);
 		filter = NULL;
-
-		fail = false;
 	}
 
 	if (fail)

@@ -2396,19 +2396,18 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 	return 0;
  fail:
 	/* No call succeeded */
-	free_filter_list(filter_list);
 	parse_error(pe, FILT_ERR_BAD_SUBSYS_FILTER, 0);
-	return -EINVAL;
+	ret = -EINVAL;
  fail_mem:
 	__free_filter(filter);
 
-	/* If any call succeeded, we still need to sync */
-	if (!fail)
+	/* If any filter was replaced, we still need to sync */
+	if (!list_empty(&filter_list->list))
 		delay_free_filter(filter_list);
 	else
 		free_filter_list(filter_list);
 
-	return -ENOMEM;
+	return ret;
 }
 
 static int create_filter_start(char *filter_string, bool set_str,