Re: [PATCH v5 3/8] perf bpf-filter: Support BPF sample filters on tracepoint events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf bpf-filter: tracepoint BPF filter terms bypassed in is_possible_tp_filter()
--

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

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

This commit adds a tracepoint bpf program (perf_tp_sample_filter) to
evaluate context-independent sample filter terms (uid, gid, tid, cpu,
time, and cgroup) on tracepoint events using bpf helpers. It also updates
perf_bpf_filter__prepare(), perf_bpf_filter__pin(), and
perf_bpf_filter__unpin() to handle loading and pinning with the new
tracepoint program.

> diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c
> index 1596b08fe4e6a..ac0e7cf40f3af 100644
> --- a/tools/perf/util/bpf-filter.c
> +++ b/tools/perf/util/bpf-filter.c
> @@ -123,15 +123,26 @@ static int check_sample_flags(struct evsel *evsel, struct perf_bpf_filter_expr *
>  {
>  	const struct perf_sample_info *info;
>  
> -	if (expr->term >= PBF_TERM_SAMPLE_START && expr->term <= PBF_TERM_SAMPLE_END &&
> -	    (evsel->core.attr.sample_type & (1 << (expr->term - PBF_TERM_SAMPLE_START))))
> -		return 0;
> -
>  	if (expr->term == PBF_TERM_UID || expr->term == PBF_TERM_GID) {
>  		/* Not dependent on the sample_type as computed from a BPF helper. */
>  		return 0;
>  	}
>  
> +	if (evsel->core.attr.type == PERF_TYPE_TRACEPOINT) {
> +		if (expr->term == PBF_TERM_TID || expr->term == PBF_TERM_CPU ||
> +		    expr->term == PBF_TERM_TIME || expr->term == PBF_TERM_CGROUP)
> +			return 0;
> +		if (expr->op != PBF_OP_GROUP_BEGIN) {
> +			pr_err("Error: %s tracepoint event does not support BPF sample term %d\n",
> +			       evsel__name(evsel), expr->term);
> +			return -1;
> +		}
> +	}

[Severity: Medium]
check_sample_flags() in tools/perf/util/bpf-filter.c and
perf_get_common_sample() in tools/perf/util/bpf_skel/sample_filter.bpf.c
enable tracepoint bpf sample filtering for uid, gid, tid (and pid via
entry->part), cpu, time, and cgroup.

However, when parse_filter() calls set_filter() in
tools/perf/util/parse-events.c, tracepoint events are checked with
is_possible_tp_filter():

tools/perf/util/parse-events.c:is_possible_tp_filter() {
	return strstr(str, "uid") == NULL;
}

tools/perf/util/parse-events.c:set_filter() {
	...
	if (evsel->core.attr.type == PERF_TYPE_TRACEPOINT && is_possible_tp_filter(str)) {
		if (evsel__append_tp_filter(evsel, str) < 0) {
			fprintf(stderr,
				"not enough memory to hold filter string\n");
			return -1;
		}

		return 0;
	}
	...
	if (!nr_addr_filters)
		return perf_bpf_filter__parse(&evsel->bpf_filters, str);
}

A later commit in this series ("perf bpf-filter: Add lock_held() sample
filter") updates is_possible_tp_filter() to also check for "gid" and
"lock_held", but filters on tracepoint events using terms such as cgroup,
time, tid, or pid (or gid at this commit) without "uid" are still routed to
evsel__append_tp_filter() instead of perf_bpf_filter__parse(), causing the
tracepoint filter ioctl to fail when those terms are not tracepoint format
fields.

Should is_possible_tp_filter() in tools/perf/util/parse-events.c also
account for the other tracepoint bpf filter terms enabled here so that
set_filter() routes them to evsel->bpf_filters?

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