tracing/filters: Fix event filter parsing and matching bugs

Hi,

Here is the 3rd 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/179291742944.492959.11659339795290028998.stgit@devnote2

This version splitted some combined patches into dedicated patches, dropped a
wrong patch, and introduced 5 new patches to fix wrong error position reported
in error_log (this is required to fix syntax test failures).

Changes in v3:
[01/16]
 - Split zero-length string underflow and out-of-bounds fix from
   fixed-size string matching.
 - Return 0 immediately if "len <= r->len" in regex_match_end()
   without scanning the string.
[02/16]
 - Split from zero-length string fix into a dedicated patch.
 - Introduce regex_match_end_str() for fixed-size buffers and assign
   it in parse_pred() for FILTER_COMM and FILTER_STATIC_STRING, avoiding
   strnlen() overhead for dynamic and pointer strings in regex_match_end().
[06/16] ([05/09] in v2)
 - 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.
[07/16] ([05/09] in v2)
 - Fix quoted operand parsing in function event filter.
 - Allow empty quoted operand ("") as valid syntax per Steven Rostedt.
[08/16] ([05/09] in v2)
 - Reject empty unquoted operands (!q && !len) in function events.
[09/16] ([05/09] in v2)
 - Reject stray quotes and '(' in unquoted function event operands.
([06/09] in v2)
 - Dropped.
[11/16]
 - Introduce filter_check_newline() to convert the first newline to
   NUL and reject any non-whitespace characters after the newline with

110 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'. However, if @len is 0
(denoting an empty dynamic array where the size of @str is 0), "len - 1"
underflows to -1.

Furthermore, if the string length @len is less than or equal to the
pattern length r->len, the string contains fewer characters than the
pattern and cannot match.

Add a check for "len <= r->len" to safely return 0 without accessing
@str when @len is 0 or smaller than the pattern.

Fixes: a3291c14ecf0 ("tracing/filters: Fix MATCH_END_ONLY filter matching")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v3:
 - Split zero-length string out-of-bounds fix from fixed-size string
   matching.
 - Return 0 immediately if "len <= r->len" without scanning the string.
Changes in v2:
 - Check "len < r->len" to guard against negative lengths and avoid
   calling strnlen() when the buffer capacity is smaller than pattern.
 - Use strnlen(str, len) directly instead of calling strlen(str) when
   len is 0, avoiding an out-of-bounds read on 0-length dynamic arrays.
---
 kernel/trace/trace_events_filter.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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

@@ -1075,8 +1075,12 @@ static int regex_match_middle(char *str, struct regex *r, int len)
 
 static int regex_match_end(char *str, struct regex *r, int len)
 {
-	int strlen = len - 1;
+	int strlen;
 
+	if (len <= r->len)
+		return 0;
+
+	strlen = len - 1;
 	if (strlen >= r->len &&
 	    memcmp(str + strlen - r->len, r->pattern, r->len) == 0)
 		return 1;

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.

To fix this without penalizing dynamic and pointer strings with an
unnecessary strnlen() scan on every event, introduce a dedicated match
function regex_match_end_str() for fixed-size buffers that uses
strnlen(str, len) to locate the string end within the buffer capacity.
In parse_pred(), assign regex_match_end_str to pred->regex->match for
FILTER_COMM and FILTER_STATIC_STRING fields when MATCH_END_ONLY is
requested.

Fixes: a3291c14ecf0 ("tracing/filters: Fix MATCH_END_ONLY filter matching")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v3:
 - Split from zero-length string fix into a dedicated patch.
 - Introduce regex_match_end_str() for fixed-size buffers and assign
   it in parse_pred() for FILTER_COMM and FILTER_STATIC_STRING, avoiding
   strnlen() overhead for dynamic and pointer strings in regex_match_end().
---
 kernel/trace/trace_events_filter.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 59a0d3ba7274..f7c067a58ba4 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1087,6 +1087,20 @@ 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_glob(char *str, struct regex *r, int len)
 {
 	return glob_match_len(r->pattern, str, len) ? 1 : 0;

@@ -2026,10 +2040,14 @@ 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)
+				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" is 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.

Move "fail = false;" into the else branch of the error check so that
"fail" is only cleared when at least one event filter succeeds. Because
file->filter is still replaced even when process_preds() fails, check
!list_empty(&filter_list->list) on the fail and fail_mem paths so any
replaced old filters are freed after an RCU grace period via
delay_free_filter().

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 |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index f7c067a58ba4..617bc3c0bb0c 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -2354,8 +2354,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);

@@ -2370,8 +2372,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)

@@ -2386,14 +2386,17 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 	return 0;
  fail:
 	/* No call succeeded */
-	free_filter_list(filter_list);
+	if (!list_empty(&filter_list->list))
+		delay_free_filter(filter_list);
+	else
+		free_filter_list(filter_list);
 	parse_error(pe, FILT_ERR_BAD_SUBSYS_FILTER, 0);
 	return -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);

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 617bc3c0bb0c..3f56e234a02c 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 3f56e234a02c..2554120ff6d6 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;
 		}

@@ -1703,8 +1706,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)

@@ -2296,8 +2301,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 2554120ff6d6..3d2a075ec425 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1756,6 +1756,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 3d2a075ec425..170d51ff337c 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1853,22 +1853,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 170d51ff337c..96977eb9af85 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1874,6 +1874,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>

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.

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 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 96977eb9af85..48e16d75be74 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1863,9 +1863,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>

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 48e16d75be74..9d1446464058 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -2015,6 +2015,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] == '"') {

@@ -2164,6 +2168,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).

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
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 |   37 ++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 9d1446464058..f3c18324b07a 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;
+}
+
 static int create_filter_start(char *filter_string, bool set_str,
 			       struct filter_parse_error **pse,
 			       struct event_filter **filterp)

@@ -2452,6 +2477,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);

@@ -2570,6 +2599,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);

@@ -2624,6 +2657,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 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.

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:
 - 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 f3c18324b07a..903caf62a407 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;

@@ -1694,12 +1694,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++;

@@ -1720,7 +1723,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;
 	}
 

@@ -1739,6 +1742,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[] */

@@ -1747,7 +1752,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;
 	}
 

@@ -1844,7 +1849,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;

@@ -2033,7 +2038,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;
 		}
 

@@ -2130,7 +2135,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;
 		}
 

@@ -2169,7 +2174,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.

Fixes: e6745a4da964 ("tracing: Add a way to filter function addresses to function names")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
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 903caf62a407..b1b4f1a57dc9 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1695,6 +1695,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;
 

@@ -1736,6 +1737,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;
 	}
 

@@ -1779,7 +1781,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;
 		}
 

@@ -1789,7 +1791,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.

Fixes: 39f7c41c908b ("tracing/filters: Enable filtering a cpumask field by another cpumask")
Fixes: 3cbec9d7b93d ("tracing/filters: Enable filtering a scalar field by a cpumask")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
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 b1b4f1a57dc9..291c9805e119 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1926,16 +1926,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 } */

@@ -2023,7 +2024,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.

Fixes: 7884b0bf098c ("tracing: Add "within" filter for call-stack-based event filtering")
Cc: <redacted>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
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 291c9805e119..bd54fa6e58ee 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -2096,7 +2096,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: redundant check in regex_match_end() makes patch a no-op
--

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

tracing/filters: Fix regex_match_end() for zero-length strings

This commit adds an early return in regex_match_end() when len <= r->len to
avoid underflowing len - 1 and accessing the input string when len is 0 or
smaller than the pattern.

[Severity: Medium]
Does this change have any functional effect in regex_match_end() in
kernel/trace/trace_events_filter.c?

In regex_match_end(), len, strlen, and r->len (defined in struct regex in
kernel/trace/trace.h and initialized from strlen(search) >= 0 in
filter_build_regex()) are all signed int variables.

When len is 0, strlen = len - 1 evaluates to -1. Because both strlen and
r->len are signed ints and r->len >= 0, the existing check:

	if (strlen >= r->len &&
	    memcmp(str + strlen - r->len, r->pattern, r->len) == 0)

evaluates -1 >= r->len, which is false and short-circuits before calling
memcmp() or dereferencing str.

More generally, for signed integers len >= 0 and r->len >= 0, the new check
len <= r->len is equivalent to (len - 1) < r->len (i.e. strlen < r->len),
which is the exact negation of the existing strlen >= r->len check. With the
early return in place, strlen >= r->len is also unconditionally true and
redundant.

Since the pre-existing condition already prevented out-of-bounds access when
len == 0 or len <= r->len, is this patch (along with the Fixes and stable
tags) necessary, or was a different change intended here?

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

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.

[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():

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);
	}
}

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.

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.

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tracing/filters: unhandled zero len in regex_match_end_str()
--

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

tracing/filters: Fix MATCH_END_ONLY regex matching for fixed-size strings

This commit introduces regex_match_end_str() to handle suffix regex matching
on fixed-size string buffers using strnlen(), and assigns it in parse_pred()
for comm and static string fields.

[Severity: Medium]
How does regex_match_end_str() behave when len is 0?

For the ftrace/print event, the buf field is defined in
kernel/trace/trace_export.c with a size of 0:

#undef __dynamic_array
#define __dynamic_array(_type, _item) {					\
	.type = #_type "[]", .name = #_item,				\
	.size = 0, .align = __alignof__(_type),				\
	is_signed_type(_type), .filter_type = FILTER_OTHER },

Because its type is "char[]", filter_assign_type() classifies it as a static
string field while field->size remains 0. In parse_pred(),
pred->regex->field_len is set to field->size (0) and pred->regex->match is
set to 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)
				pred->regex->match = regex_match_end_str;

When a trigger or eprobe with a suffix filter (such as buf ~ "*foo") is
evaluated via write_marker_to_buffer() -> event_triggers_call() ->
filter_match_preds() -> filter_pred_string(), regex_match_end_str() is called
with len == 0.

As noted in the comment above regex_match_full() (added in commit
10f20e9f9da9 ("tracing: Have zero size length in filter logic be full
string")), a len of zero indicates that str is a NUL-terminated string, which
regex_match_full(), regex_match_front(), and regex_match_middle() handle
explicitly:

static int regex_match_full(char *str, struct regex *r, int len)
{
	/* len of zero means str is dynamic and ends with '\0' */
	if (!len)
		return strcmp(str, r->pattern) == 0;

9 more lines

On Wed, 28 Oct 2026 00:59:33 +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: redundant check in regex_match_end() makes patch a no-op
--

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

tracing/filters: Fix regex_match_end() for zero-length strings

This commit adds an early return in regex_match_end() when len <= r->len to
avoid underflowing len - 1 and accessing the input string when len is 0 or
smaller than the pattern.

quoted

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 5e74d2dae40a3..59a0d3ba72740 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1075,8 +1075,12 @@ static int regex_match_middle(char *str, struct regex *r, int len)
 
 static int regex_match_end(char *str, struct regex *r, int len)
 {
-	int strlen = len - 1;
+	int strlen;
 
+	if (len <= r->len)
+		return 0;
+
+	strlen = len - 1;
 	if (strlen >= r->len &&
 	    memcmp(str + strlen - r->len, r->pattern, r->len) == 0)
 		return 1;

[Severity: Medium]
Does this change have any functional effect in regex_match_end() in
kernel/trace/trace_events_filter.c?

Oops, right. Please drop this patch. This is my fault (I suggested).

quoted

+	if (len <= r->len)

and

quoted

 	if (strlen >= r->len &&

are the same...

Thanks!

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

On Wed, 28 Oct 2026 09:43:43 +0900, Masami Hiramatsu (Google) [off-list ref] wrote:

BTW, this is required for skipping strnlen().

And this also checks actual length.

Thanks,

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

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;

15 more quoted lines

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]

On Wed, 28 Oct 2026 01:05:18 +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: unhandled zero len in regex_match_end_str()
--

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

tracing/filters: Fix MATCH_END_ONLY regex matching for fixed-size strings

This commit introduces regex_match_end_str() to handle suffix regex matching
on fixed-size string buffers using strnlen(), and assigns it in parse_pred()
for comm and static string fields.

quoted

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 59a0d3ba72740..f7c067a58ba41 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1087,6 +1087,20 @@ 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;
+}

[Severity: Medium]
How does regex_match_end_str() behave when len is 0?

For the ftrace/print event, the buf field is defined in
kernel/trace/trace_export.c with a size of 0:

22 more quoted lines

Good catch... this may be yet another issue.

I think we need another different set of matching functions for dynamic
string. Sharing it with static string (where len = 0 means zero length)
and dynamic string (where len = 0 means variable length string) is
a bad idea.

As noted in the comment above regex_match_full() (added in commit
10f20e9f9da9 ("tracing: Have zero size length in filter logic be full
string")), a len of zero indicates that str is a NUL-terminated string, which
regex_match_full(), regex_match_front(), and regex_match_middle() handle
explicitly:

static int regex_match_full(char *str, struct regex *r, int len)
{
	/* len of zero means str is dynamic and ends with '\0' */
	if (!len)
		return strcmp(str, r->pattern) == 0;

In regex_match_end_str(), however, when len is 0, the (len < r->len) check
returns 0 for any non-empty pattern, and strnlen(str, 0) also returns 0.

Should regex_match_end_str() check for !len and use strlen(str) in that case,
so that suffix filters on ftrace/print work as expected?

I rather like to have regex_match_end_dynstr() in this case.

Anyway, we should have a unit test for filters.

Thanks,

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

On Wed, 28 Oct 2026 09:43:53 +0900
"Masami Hiramatsu (Google)" [off-list ref] wrote:

34 more quoted lines

I still prefer my suggestion which cleans up the code and gets rid of the
duplicate if statement for freeing.

  https://lore.kernel.org/all/20261026153258.5259323d@gandalf.local.home/

-- Steve

On Wed, 28 Oct 2026 09:44:48 +0900
"Masami Hiramatsu (Google)" [off-list ref] wrote:

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.

Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
Cc: <redacted>

I wouldn't really call this a fix, as I can't see this breaking anything.
Yes, the calc_stack may say there's more elements than what parse_pred()
sees, but the parsing still fails.

This looks more of a clean up than a fix.

Unless, the current code allows something to actually be broken. Does it?

-- Steve

On Wed, 28 Oct 2026 10:28:10 +0900
"Masami Hiramatsu (Google)" [off-list ref] wrote:

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.

I noticed this when I applied the patch.

Yes, the correct solution is to pass in const char *filter to
__ftrace_set_clr_event_nolock() (I was planning on doing this after the
merge window closed). But no, making a copy in
__ftrace_set_clr_event_nolock() is *not* the right solution. There's
nothing in that function that modifies that string to suggest it needs to
make a copy of it.

The correct solution is to pass const char *filter to apply_event_filter()
and have *that* function do the copy. That was my plan to make a patch for.

-- Steve

On Wed, 28 Oct 2026 09:50:37 -0400, Steven Rostedt [off-list ref] wrote:

On Wed, 28 Oct 2026 09:44:48 +0900
"Masami Hiramatsu (Google)" [off-list ref] wrote:

quoted

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.

Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
Cc: <redacted>

I wouldn't really call this a fix, as I can't see this breaking anything.
Yes, the calc_stack may say there's more elements than what parse_pred()
sees, but the parsing still fails.

This looks more of a clean up than a fix.

Agreed. I think fixin error_log UI issue will be a part of improvement.
OK, let me remove the Fixes and Cc: stable tags from those patches.

Unless, the current code allows something to actually be broken. Does it?

No. Hmm, should I split those "improving parse_error()" patches from
this series?

Thanks,

-- Steve

quoted

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 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 96977eb9af85..48e16d75be74 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -1863,9 +1863,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);

1 more quoted lines

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

On Wed, 28 Oct 2026 10:03:48 -0400, Steven Rostedt [off-list ref] wrote:

On Wed, 28 Oct 2026 10:28:10 +0900
"Masami Hiramatsu (Google)" [off-list ref] wrote:

quoted

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.

I noticed this when I applied the patch.

Yes, the correct solution is to pass in const char *filter to
__ftrace_set_clr_event_nolock() (I was planning on doing this after the
merge window closed). But no, making a copy in
__ftrace_set_clr_event_nolock() is *not* the right solution. There's
nothing in that function that modifies that string to suggest it needs to
make a copy of it.

The correct solution is to pass const char *filter to apply_event_filter()
and have *that* function do the copy. That was my plan to make a patch for.

OK, so at this moment I will follow Sashiko's suggestion.

Thanks!

-- Steve

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

On Wed, 28 Oct 2026 09:28:46 -0400, Steven Rostedt [off-list ref] wrote:

On Wed, 28 Oct 2026 09:43:53 +0900
"Masami Hiramatsu (Google)" [off-list ref] wrote:

quoted

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

In process_system_preds(), "fail" is 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.

Move "fail = false;" into the else branch of the error check so that
"fail" is only cleared when at least one event filter succeeds. Because
file->filter is still replaced even when process_preds() fails, check
!list_empty(&filter_list->list) on the fail and fail_mem paths so any
replaced old filters are freed after an RCU grace period via
delay_free_filter().

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 |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index f7c067a58ba4..617bc3c0bb0c 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c

@@ -2354,8 +2354,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

42 more quoted lines

Ah, indeed. Sorry I missed it. Let me update it.

Thanks!

-- Steve

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