Re: [PATCH] tracing: Reject filter expressions that end on an operator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tracing: unreachable operator check in predicate_parse()
--

commit 6ada36f117f4b8f90eaf23b50ed6bf590c01cc94
Author: JongHyuk Kim (MalHyuk) <malhyuk97@gmail.com>

tracing: Reject filter expressions that end on an operator

This commit adds a check in predicate_parse() to reject event filter
expressions where a pending PROCESS_AND or PROCESS_OR flag remains on the
operator stack after parsing completes.

> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 5e74d2dae40a3..744e70668940f 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -611,6 +611,13 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
>  		goto out_free;
>  	}
>  
> +	if (*top & (PROCESS_AND | PROCESS_OR)) {
> +		/* Expression ends on an operator */
> +		ret = -EINVAL;
> +		parse_error(pe, FILT_ERR_INVALID_OP, ptr - str);
> +		goto out_free;
> +	}

[Severity: Medium]
Can this check ever be reached with PROCESS_AND or PROCESS_OR set in *top?

In kernel/trace/trace_events_filter.c:predicate_parse(), PROCESS_AND and
PROCESS_OR are only set inside the inner loop after at least one predicate
has been parsed (N >= 1), and setting either flag immediately breaks out to
the outer while (*ptr) loop:

	if (*top & PROCESS_AND) {
		update_preds(prog, N - 1, invert);
		*top &= ~PROCESS_AND;
	}
	if (*next == '&') {
		*top |= PROCESS_AND;
		break;
	}
	if (*top & PROCESS_OR) {
		update_preds(prog, N - 1, !invert);
		*top &= ~PROCESS_OR;
	}
	if (*next == '|') {
		*top |= PROCESS_OR;
		break;
	}
	if (!*next)
		goto out;

If an expression ends on "&&" or "||" (such as
"common_pid==1||common_pid==1&&"), the outer while (*ptr) loop terminates and
hits the check added by commit 1fdc4164e488 ("tracing: Reject incomplete event
filter expressions") right above the out label:

	/* Only the inner loop can complete a non-empty expression. */
	if (N) {
		ret = -EINVAL;
		parse_error(pe, FILT_ERR_INVALID_FILTER, ptr - str);
		goto out_free;
	}
 out:

Because N >= 1 whenever PROCESS_AND or PROCESS_OR is set, falling out of the
outer loop always takes that goto out_free path before reaching out.

The only other way to reach out is via if (!*next) goto out inside the inner
loop, which only runs after both PROCESS_AND and PROCESS_OR have just been
cleared from *top in the same iteration.

Doesn't that make this new check (and FILT_ERR_INVALID_OP) unreachable dead
code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026042540.2478566-1-malhyuk97@gmail.com?part=1