Re: [PATCH] perf stat: Do not open cgroups for BPF counters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Fallback from BPF counter mode to standard cgroup mode leaves cgroup file descriptors uninitialized (-1), causing standard perf event opening to fail with -EBADF.
--

--- Patch [1]: [PATCH] perf stat: Do not open cgroups for BPF counters ---
commit 510c63ce03b2362940d1f693b45d5faa86cf3bc1
Author: Namhyung Kim <namhyung@kernel.org>

perf stat: Do not open cgroups for BPF counters

This commit prevents opening and keeping file descriptors for cgroups
when using the --bpf-counters and --for-each-cgroup options. BPF mode
uses a set of shared events and compares cgroup-IDs, so it doesn't need
individual cgroup file descriptors.

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -2874,7 +2874,8 @@ int cmd_stat(int argc, const char **argv)
>  			goto out;
>  		}
>  
> -		if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list, true) < 0) {
> +		if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list,
> +					  !target.use_bpf) < 0) {

[Severity: Medium]
Will this break the fallback path when we exceed BPERF_CGROUP__MAX_EVENTS?

Later in cmd_stat(), we have a fallback that disables BPF counters if there
are too many events:

	if (target.use_bpf && nr_cgroups &&
	    (evsel_list->core.nr_entries / nr_cgroups) > BPERF_CGROUP__MAX_EVENTS) {
		pr_warning("Disabling BPF counters due to more events...");
		target.use_bpf = false;
	}

If this fallback is triggered, target.use_bpf becomes false, but the cgroups
were already expanded with !target.use_bpf (which was false at the time),
leaving the cgroup file descriptors uninitialized (-1).

When the standard event opening later calls evsel__open_cpu() in
tools/perf/util/evsel.c:

	if (evsel->cgrp)
		pid = evsel->cgrp->fd;

Could it attempt to use the uninitialized cgroup file descriptor (-1) with
the perf_event_open syscall, causing it to fail with -EBADF? Should the
cgroup file descriptors be reopened if the fallback to standard mode is
taken?

>  			parse_options_usage(stat_usage, stat_options,
>  					    "for-each-cgroup", 0);
>  			goto out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708200407.1240640-1-namhyung@kernel.org?part=1