Re: [PATCH v4 2/7] 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 bpf-filter: double close of tracepoint fd via perf_bpf_filter__prepare()
- [Medium] perf bpf-filter: missing tracepoint BPF terms in is_possible_tp_filter()
--

commit d87f98375f9a6f9592030472ebfbf88f3edd6fdd
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 filter terms (uid, gid, tid, cpu, time,
cgroup) on PERF_TYPE_TRACEPOINT events, and attaches it once to the
shared trace_event_call in perf_bpf_filter__prepare().

> diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c
> index 72e83ac1d6a4b..4e99af4764f1d 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;

[Severity: Medium]
Can standalone tracepoint BPF filters using gid, tid, pid, cpu, time, or
cgroup actually reach perf_bpf_filter__parse() from --filter?

In tools/perf/util/parse-events.c, set_filter() checks
is_possible_tp_filter(str) before deciding whether to route a
PERF_TYPE_TRACEPOINT filter to evsel__append_tp_filter() or
perf_bpf_filter__parse():

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) {
	...
}

Because is_possible_tp_filter() only checks for "uid" in this commit,
passing --filter 'gid == 0' or --filter 'cpu == 0' on a tracepoint event
routes the expression to the ftrace tracepoint predicate filter via
evsel__append_tp_filter(), which fails with -EINVAL in
PERF_EVENT_IOC_SET_FILTER when the tracepoint lacks matching format fields.

Although a later commit in the series ("perf bpf-filter: Add lock_held()
sample filter") adds "gid" and "lock_held" to is_possible_tp_filter(), the
tid, pid, cpu, time, and cgroup terms supported here in check_sample_flags()
and perf_get_common_sample() still remain unrouted to perf_bpf_filter__parse()
unless combined with uid, gid, or lock_held in the same filter string.

Should is_possible_tp_filter() also check for gid, tid, pid, cpu, time, and
cgroup?

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

[ ... ]

> @@ -501,15 +528,35 @@ int perf_bpf_filter__prepare(struct evsel *evsel, struct target *target)
>  		goto err;
>  	}
>  
> -	prog = skel->progs.perf_sample_filter;
> -	for (x = 0; x < xyarray__max_x(evsel->core.fd); x++) {
> -		for (y = 0; y < xyarray__max_y(evsel->core.fd); y++) {
> -			link = bpf_program__attach_perf_event_opts(prog, FD(evsel, x, y),
> -								   &pe_opts);
> -			if (IS_ERR(link)) {
> -				pr_err("Failed to attach perf sample-filter program\n");
> -				ret = PTR_ERR(link);
> -				goto err;
> +	if (is_tracepoint) {
> +		/*
> +		 * For PERF_TYPE_TRACEPOINT, PERF_EVENT_IOC_SET_BPF attaches the
> +		 * BPF program to event->tp_event->prog_array on the shared
> +		 * trace_event_call, which perf_trace_run_bpf_submit() executes
> +		 * for all CPUs and tasks before iterating the per-CPU perf
> +		 * events. Attach once on the first FD so tp_event->prog_array
> +		 * does not get duplicate copies for each per-CPU FD.
> +		 */
> +		prog = skel->progs.perf_tp_sample_filter;
> +		link = bpf_program__attach_perf_event_opts(prog, FD(evsel, 0, 0),
> +							   &pe_opts);
> +		ret = libbpf_get_error(link);
> +		if (ret) {
> +			pr_err("Failed to attach perf tracepoint sample-filter program\n");
> +			goto err;
> +		}
> +		skel->links.perf_tp_sample_filter = link;

[Severity: Medium]
Does storing link in skel->links.perf_tp_sample_filter cause FD(evsel, 0, 0)
to be closed twice during teardown?

In tools/lib/bpf/libbpf.c, bpf_program__attach_perf_event_opts() takes
ownership of the passed perf_event_fd without duplicating it, and its
detach callback closes that descriptor when the bpf_link is destroyed:

tools/lib/bpf/libbpf.c:bpf_link_perf_detach() {
	struct bpf_link_perf *perf_link = container_of(link, struct bpf_link_perf, link);
	int err = 0;

	if (ioctl(perf_link->perf_event_fd, PERF_EVENT_IOC_DISABLE, 0) < 0)
		err = -errno;

	if (perf_link->perf_event_fd != link->fd)
		close(perf_link->perf_event_fd);
	close(link->fd);
	...
}

During teardown (for example in evlist__put()), evlist__close() closes all
descriptors in evsel->core.fd, including FD(evsel, 0, 0), before
evlist__purge() destroys the evsels:

evlist__put()
  -> evlist__close()
       -> perf_evsel__close_cpu()
            -> perf_evsel__close_fd_cpu()  /* closes FD(evsel, 0, 0) */
  -> evlist__purge()
       -> evsel__put()
            -> evsel__exit()
                 -> perf_bpf_filter__destroy()
                      -> sample_filter_bpf__destroy()
                           -> bpf_link__destroy(skel->links.perf_tp_sample_filter)
                                -> bpf_link_perf_detach()  /* ioctls and closes FD(evsel, 0, 0) again */

In the non-tracepoint branch below, the returned link is not stored in
skel->links, or if skel->links.perf_tp_sample_filter is kept, should
FD(evsel, 0, 0) be duplicated before passing it to
bpf_program__attach_perf_event_opts()?

[ ... ]

>

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