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

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

[Namhyung Kim]

The --bpf-counters and --for-each-cgroup options use a set of shared
events among the given cgroups rather than adding events for each cgroup
respectively.  It only uses cgroup-ID to compare and calculate the
result.  So no need to open and keep FDs for cgroups in BPF mode.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-stat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 92cdb2df7285befb..49e523058185b753 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2888,7 +2888,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) {
 			parse_options_usage(stat_usage, stat_options,
 					    "for-each-cgroup", 0);
 			goto out;
-- 
2.55.0.795.g602f6c329a-goog

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

[sashiko-bot]

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

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

[Namhyung Kim]

On Wed, Jul 08, 2026 at 08:14:33PM +0000, sashiko-bot@kernel.org wrote:
> 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).

I think it's a false alarm.  The evlist__expand_cgroup() is only used if
stat_config.cgroup_list is set and then it should not have nr_cgroups.
There's a check for that right before the call to the function.

So in that case, it cannot pass the if condition below and never set to
target.use_bpf to false.

Thanks,
Namhyung