* Re: [PATCH] perf evsel: Skip group members without fds when walking a group
2026-10-23 8:56 [PATCH] perf evsel: Skip group members without fds when walking a group TAKANO Mitsuhiro
@ 2026-10-23 9:20 ` sashiko-bot
2026-10-23 16:24 ` TAKANO Mitsuhiro
0 siblings, 1 reply; 3+ messages in thread
From: sashiko-bot @ 2026-10-23 9:20 UTC (permalink / raw)
To: TAKANO Mitsuhiro, linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf evsel: incomplete fix in evsel__enable_cpu() for tool PMU groups
--
commit 9f4b8cfeed3bb333c7e55d36118aab80f6b77164
Author: TAKANO Mitsuhiro <takano32@gmail.com>
perf evsel: Skip group members without fds when walking a group
This commit skips group members that have a NULL core.fd pointer when
enabling or disabling event groups in evsel__enable_cpu(), evsel__enable(),
evsel__disable_cpu(), and evsel__disable() to avoid a segmentation fault with
tool-PMU-led event groups.
> perf evsel: Skip group members without fds when walking a group
>
> A group led by a userspace PMU event and containing kernel events is
> opened with group_fd = -1 for the members, because the kernel cannot
> group a userspace PMU with a kernel one. The members are therefore
> enabled from userspace when the leader is enabled, but their fd array is
> not set up in that case, so perf_evsel__enable_cpu() dereferences a NULL
> pointer:
>
> $ perf stat -a -e '{user_time,cycles}' -- sleep 0.05
> Segmentation fault
>
> $ perf stat -a -e '{system_time,cycles}' -- sleep 0.05
> Segmentation fault
>
> Only the leaders of such a group are opened with an fd in this path.
> Skip the members that have no fd, the same way __evlist__enable() skips
> leaders without one, in the four loops that walk the members of a group.
>
> Verified on an Ampere Altra (arm64, Neoverse-N1) with a perf built from
> this tree: the two commands above segfault before the change and print
> user_time/system_time after it, while plain groups such as
> '{cycles,instructions}', 'perf stat' with no event and '-r 2' are
> unchanged.
>
> Fixes: 0b18bced5444 ("perf tool_pmu: Make tool PMU events respect enable/disable")
> Assisted-by: LLM
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index d4cb455f4a7d4..32c54cd4acf0b 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1882,6 +1882,8 @@ int evsel__enable_cpu(struct evsel *evsel, int cpu_map_idx)
> struct evsel *member;
>
> for_each_group_member(member, evsel) {
> + if (!member->core.fd)
> + continue;
[Severity: Medium]
Does skipping !member->core.fd here mask an earlier open failure in
evsel__open_cpu() -> get_group_fd() rather than allowing the kernel member
event to open with group_fd = -1?
The commit message states that a group led by a userspace PMU event and
containing kernel events is opened with group_fd = -1 for the members, and
that their fd array is not set up in that case.
However, looking at how '{user_time,cycles}' or '{system_time,cycles}' is
opened:
1. When user_time or system_time leads the group, evsel__tool_pmu_open() in
tools/perf/util/tool_pmu.c opens /proc/stat (or /proc/<pid>/stat) and stores
that procfs file descriptor in FD(leader, idx, thread).
2. When the kernel member event cycles is opened in
tools/perf/util/evsel.c:evsel__open_cpu(), __evsel__prepare_open() allocates
cycles->core.fd and then evsel__open_cpu() calls get_group_fd():
tools/perf/util/evsel.c:get_group_fd() {
struct evsel *leader = evsel__leader(evsel);
int fd;
if (!evsel->supported || evsel__is_group_leader(evsel))
return -1;
BUG_ON(!leader->core.fd);
cpu_map_idx = evsel__hybrid_group_cpu_map_idx(evsel, cpu_map_idx);
if (cpu_map_idx == -1)
return -1;
fd = FD(leader, cpu_map_idx, thread);
BUG_ON(fd == -1 && leader->supported);
return fd == -1 ? -2 : fd;
}
3. Because get_group_fd() does not check
evsel__is_non_perf_event_open_pmu(leader) to return -1, it returns the
leader's /proc/stat file descriptor as group_fd to evsel__open_cpu():
tools/perf/util/evsel.c:evsel__open_cpu() {
...
group_fd = get_group_fd(evsel, idx, thread);
...
fd = sys_perf_event_open(&evsel->core.attr, pid, cpu.cpu,
group_fd, evsel->open_flags);
}
4. sys_perf_event_open() then fails with -EBADF because group_fd is a procfs
file descriptor rather than a perf_event file descriptor. That failure leaves
cycles->supported = false, causing __run_perf_stat() in
tools/perf/builtin-stat.c to free cycles->core.fd:
tools/perf/builtin-stat.c:__run_perf_stat() {
evlist__for_each_entry(evsel_list, counter) {
if (!counter->supported) {
perf_evsel__free_fd(&counter->core);
continue;
}
}
With this patch, cycles still fails to open and is reported as
<not supported> instead of being opened with group_fd = -1 and enabled from
userspace as intended by commit 0b18bced5444 ("perf tool_pmu: Make tool PMU
events respect enable/disable").
Furthermore, if duration_time leads the group (for example,
perf stat -a -e '{duration_time,cycles}' -- sleep 0.05),
evsel__tool_pmu_open() succeeds and sets leader->supported = true while
leaving FD(leader, cpu_map_idx, thread) == -1, which triggers
BUG_ON(fd == -1 && leader->supported) in get_group_fd() before
evsel__enable_cpu() is ever reached.
Should get_group_fd() check evsel__is_non_perf_event_open_pmu(leader) and
return -1 so that kernel members in a tool-PMU-led group actually open with
group_fd = -1?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261023085629.1694941-1-takano32@gmail.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread