Re: [PATCH v6 02/11] perf bpf-filter: Support BPF sample filters on tracepoint events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf parse-events: is_possible_tp_filter() misroutes generic cpu tp filters
- [Medium] perf parse-events: misrouted tracepoint filters in is_possible_tp_filter()
--

commit dbe9f68d8a043d94d321ad8e9a98e3b1d3114baf
Author: Ian Rogers <irogers@google.com>

perf bpf-filter: Support BPF sample filters on tracepoint events

This commit adds a BPF_PROG_TYPE_TRACEPOINT sample filter program to evaluate
context-independent filter terms (uid, gid, tid, pid, cpu, time, cgroup) on
tracepoint events, and updates set_filter() and perf_bpf_filter__prepare() to
route and attach those filters.

> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 2f3317ce17f08..d6bab89ade5de 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c

[ ... ]

> @@ -2593,10 +2595,40 @@ foreach_evsel_in_last_glob(struct evlist *evlist,
>  	return 0;
>  }
>  
> +static bool is_filter_word_char(char c)
> +{
> +	return isalnum((unsigned char)c) || c == '_' || c == '/' || c == '.';
> +}
> +
> +static bool has_filter_word(const char *str, const char *word)
> +{
> +	size_t len = strlen(word);
> +	const char *p = str;
> +
> +	while ((p = strstr(p, word)) != NULL) {
> +		bool start_ok = (p == str) || !is_filter_word_char(p[-1]);
> +		bool end_ok = !is_filter_word_char(p[len]);
> +
> +		if (start_ok && end_ok)
> +			return true;
> +		p += len;
> +	}
> +	return false;
> +}
> +
>  /* Will a tracepoint filter work for str or should a BPF filter be used? */
> -static bool is_possible_tp_filter(const char *str)
> +static bool is_possible_tp_filter(struct evsel *evsel, const char *str)
>  {
> -	return strstr(str, "uid") == NULL;
> +	static const char *const bpf_tp_terms[] = {
> +		"uid", "gid", "tid", "pid", "cpu", "time", "cgroup",
> +	};

[Severity: Medium]
In tools/perf/util/parse-events.c, is_possible_tp_filter() includes "cpu" in
bpf_tp_terms[] and returns false whenever evsel__field(evsel, "cpu") is NULL.

In kernel/trace/trace_events.c, trace_define_generic_fields() registers "cpu"
in ftrace_generic_fields for all tracepoint events:

kernel/trace/trace_events.c:trace_define_generic_fields() {
	int ret;

	__generic_field(int, CPU, FILTER_CPU);
	__generic_field(int, cpu, FILTER_CPU);
	__generic_field(int, common_cpu, FILTER_CPU);

While trace_find_event_field() resolves ftrace_generic_fields when applying a
tracepoint filter via PERF_EVENT_IOC_SET_FILTER, f_next() and f_show() only
emit ftrace_common_fields and event-specific fields in the tracefs format
file parsed by evsel__field().

For any tracepoint that does not define its own event-specific "cpu" field
(such as sched:sched_switch or kmem:mm_page_alloc), evsel__field(evsel, "cpu")
returns NULL, so parse_filter() -> set_filter() -> is_possible_tp_filter()
returns false.

Will this cause valid ftrace tracepoint predicate filters referencing the
generic "cpu" field (such as compound filters combining "cpu" with
event-specific fields, "&&", parentheses, or "CPUS{...}" masks) to be
misrouted to perf_bpf_filter__parse() and fail to parse, while forcing
standalone "cpu" tracepoint filters onto the shared
trace_event_call->prog_array BPF filter requiring CAP_BPF?

> +
> +	for (size_t i = 0; i < ARRAY_SIZE(bpf_tp_terms); i++) {
> +		if (has_filter_word(str, bpf_tp_terms[i]) &&
> +		    !evsel__field(evsel, bpf_tp_terms[i]))
> +			return false;
> +	}
> +	return true;
>  }

[Severity: Medium]
Can parse_filter() -> foreach_evsel_in_last_glob() -> set_filter() ->
is_possible_tp_filter() in tools/perf/util/parse-events.c also misroute
tracepoint filters in the following cases?

First, when perf is built without HAVE_LIBTRACEEVENT (NO_LIBTRACEEVENT=1),
PERF_TYPE_TRACEPOINT events and kernel ftrace filters (applied via
PERF_EVENT_IOC_SET_FILTER in perf_evsel__apply_filter()) are still supported
via tp_pmu__id(), but the stub in tools/perf/util/evsel.h unconditionally
returns NULL:

tools/perf/util/evsel.h:
static inline struct tep_format_field *
evsel__field(struct evsel *evsel __maybe_unused, const char *name __maybe_unused)
{
	return NULL;
}

When evsel__field() returns NULL, any tracepoint filter referencing a format
field whose name appears in bpf_tp_terms[] (such as "pid" on
sched:sched_wakeup, sched:sched_process_fork, or signal:signal_generate)
causes is_possible_tp_filter() to return false. A filter like
--filter "pid == 1234" on sched:sched_wakeup will then be routed to
perf_tp_sample_filter and match current's TGID (bpf_get_current_pid_tgid() >>
32) instead of the tracepoint's target "pid" field, while compound filters
like --filter "pid == 1234 && success == 1" will fail to parse in
perf_bpf_filter__parse().

Second, even with HAVE_LIBTRACEEVENT enabled, has_filter_word() scans the raw
filter string with strstr() without skipping quoted string literals, and
is_filter_word_char() treats quotes, hyphens, and glob characters as word
boundaries. On an event without a field of that name (for example,
sched:sched_switch), a valid tracepoint string filter such as
prev_comm == "cpu" or next_comm == "systemd-time" matches inside the quoted
literal, causing is_possible_tp_filter() to return false and
perf_bpf_filter__parse() to reject the filter.

Third, if a filter on an event like sched:sched_wakeup combines a tracepoint
field whose name is in bpf_tp_terms[] with a BPF-only term (such as
"pid == 1234, uid == 0"), is_possible_tp_filter() returns false for the
entire string, causing "pid" to be evaluated as current's TGID in BPF rather
than the tracepoint's "pid" field.

>  
>  static int set_filter(struct evsel *evsel, const void *arg)
> @@ -2611,7 +2643,7 @@ static int set_filter(struct evsel *evsel, const void *arg)
>  		return -1;
>  	}
>  
> -	if (evsel->core.attr.type == PERF_TYPE_TRACEPOINT && is_possible_tp_filter(str)) {
> +	if (evsel->core.attr.type == PERF_TYPE_TRACEPOINT && is_possible_tp_filter(evsel, str)) {
>  		if (evsel__append_tp_filter(evsel, str) < 0) {
>  			fprintf(stderr,
>  				"not enough memory to hold filter string\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022203003.1779152-1-irogers@google.com?part=2