* 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
2026-11-11 6:16 ` Namhyung Kim
2026-11-11 10:16 ` [PATCH v2] libperf: Handle an evsel without an fd array in enable/disable TAKANO Mitsuhiro
2 siblings, 1 reply; 9+ 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] 9+ messages in thread* Re: [PATCH] perf evsel: Skip group members without fds when walking a group
2026-10-23 9:20 ` sashiko-bot
@ 2026-10-23 16:24 ` TAKANO Mitsuhiro
2026-11-10 3:08 ` Hang Zhengyang
0 siblings, 1 reply; 9+ messages in thread
From: TAKANO Mitsuhiro @ 2026-10-23 16:24 UTC (permalink / raw)
To: linux-perf-users; +Cc: sashiko-reviews, TAKANO Mitsuhiro
Thanks for the review. The members are not unusable because of the
descriptor that get_group_fd() forwards; they fail to open for their own
reason, and the crash is in the enable path that walks them afterwards.
Instrumenting both steps:
$ perf stat -a -e '{user_time,cycles}' -- sleep 0.05
get_group_fd cycles:u leader=user_time leader_fd=0x... (the /proc fd)
member cycles:u fd=(nil) supported=0
The leader is a userspace PMU event, so it opens without
perf_event_open(). The member does not open (on this box a system-wide
run is refused by perf_event_paranoid; on a permissive host it is the
EINVAL from the group descriptor), and either way it is marked
unsupported with core.fd released.
Returning -1 from get_group_fd() for a userspace PMU leader does not
change that outcome:
with get_group_fd() returning -1 when the leader is a userspace PMU:
$ perf stat -a -e '{user_time,cycles}' -- sleep 0.05
Segmentation fault
What crashes is the enable path: evlist__enable() ->
evsel__enable_cpu() walks the members of the group and calls
perf_evsel__enable_cpu() on the member that never opened. Skipping the
members without a descriptor, which is what this patch does in the four
loops, is what stops it:
with this patch:
$ perf stat -a -e '{user_time,cycles}' -- sleep 0.05
rc=0 (cycles:u is printed as <not supported>)
$ perf stat -a -e '{user_time,system_time}' -- sleep 0.05
rc=0
So get_group_fd() is not the cause here. Making the group descriptor
valid for such a leader would still leave the enable and disable paths
dereferencing a member that failed to open, so those loops have to
tolerate it regardless.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] perf evsel: Skip group members without fds when walking a group
2026-10-23 16:24 ` TAKANO Mitsuhiro
@ 2026-11-10 3:08 ` Hang Zhengyang
2026-11-11 2:33 ` TAKANO Mitsuhiro
0 siblings, 1 reply; 9+ messages in thread
From: Hang Zhengyang @ 2026-11-10 3:08 UTC (permalink / raw)
To: TAKANO Mitsuhiro, Ian Rogers
Cc: linux-perf-users, Arnaldo Carvalho de Melo, Namhyung Kim
On Fri, 23 Oct 2026 16:24:28 +0000, TAKANO Mitsuhiro wrote:
> So get_group_fd() is not the cause here.
On x86 (AMD Zen 3, perf-tools-next 13a8d580433a) it is: cycles gets
the /proc/stat fd of user_time as group_fd and fails with EBADF. Your
patch fixes the segfault for '{user_time,cycles}' with -a, -I, -r and
--delay, with no new perf test failures.
Tested-by: Hang Zhengyang <hang.zhengyang1010@gmail.com>
To also get the member counted, get_group_fd() can return -1 for a
userspace PMU leader, as 0b18bced5444 describes, and the members then
must not use PERF_FORMAT_GROUP. On top of your patch this makes
'{user_time,cycles,instructions}' count:
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2370,6 +2370,10 @@ static int get_group_fd(struct evsel *evsel, int cpu_map_idx, int thread)
if (!evsel->supported || evsel__is_group_leader(evsel))
return -1;
+ /* A userspace PMU leader has no perf event to group with */
+ if (evsel__is_non_perf_event_open_pmu(leader))
+ return -1;
+
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -721,7 +721,7 @@ static int create_perf_stat_counter(struct evsel *evsel,
- if (leader->core.nr_members > 1)
+ if (leader->core.nr_members > 1 && !evsel__is_non_perf_event_open_pmu(leader))
attr->read_format |= PERF_FORMAT_ID|PERF_FORMAT_GROUP;
cycles and instructions are then not scheduled together, though.
Ian, is that fine, or would you rather break such groups up? I can
send a patch either way. (Analysis and testing done with an AI
assistant; I reproduced the crash and the EBADF.)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] perf evsel: Skip group members without fds when walking a group
2026-11-10 3:08 ` Hang Zhengyang
@ 2026-11-11 2:33 ` TAKANO Mitsuhiro
2026-11-11 3:48 ` Hang Zhengyang
0 siblings, 1 reply; 9+ messages in thread
From: TAKANO Mitsuhiro @ 2026-11-11 2:33 UTC (permalink / raw)
To: Hang Zhengyang; +Cc: linux-perf-users, TAKANO Mitsuhiro
Thanks for testing this on Zen 3, and for the EBADF detail: it says the
member is being grouped against a descriptor that is not a perf event at
all, which is what the patch stops the walk from doing.
On the counting improvement you describe, I would rather not fold it into
this patch. Returning -1 from get_group_fd() for a leader that has no
perf event changes what the kernel is asked for: the members stop using
PERF_FORMAT_GROUP and start being read on their own, so on top of the
patch that stops the crash it becomes a change in how the group is
created, and it needs its own explanation and its own testing across the
PMU types that can lead a group.
Would you post it as a follow-up on top of this one? You have the
measurement I do not: the "{user_time,cycles,instructions}" case counting
on Zen 3. If you would rather have it here, say so and I will post a v2
that carries your diff with a Suggested-by, but then the changelog is
about a behaviour change rather than a segfault fix and I would want a
run of the group tests on a second host before asking anyone to take it.
^ permalink raw reply [flat|nested] 9+ messages in thread
* 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-11-11 6:16 ` Namhyung Kim
2026-11-11 10:16 ` [PATCH v2] libperf: Handle an evsel without an fd array in enable/disable TAKANO Mitsuhiro
2 siblings, 0 replies; 9+ messages in thread
From: Namhyung Kim @ 2026-11-11 6:16 UTC (permalink / raw)
To: TAKANO Mitsuhiro; +Cc: linux-perf-users, Arnaldo Carvalho de Melo, Ian Rogers
Hello,
On Fri, Oct 23, 2026 at 08:56:29AM +0000, TAKANO Mitsuhiro wrote:
> 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>
> ---
Sorry for the delay, but it seems easier to fix the fd handling in
libperf like this (not tested).
Thanks,
Namhyung
diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
index f747c0bc692d8a61..43ea1dfcebf421e5 100644
--- a/tools/lib/perf/evsel.c
+++ b/tools/lib/perf/evsel.c
@@ -57,7 +57,7 @@ void perf_evsel__delete(struct perf_evsel *evsel)
}
#define FD(_evsel, _cpu_map_idx, _thread) \
- ((int *)xyarray__entry(_evsel->fd, _cpu_map_idx, _thread))
+ (_evsel->fd ? (int *)xyarray__entry(_evsel->fd, _cpu_map_idx, _thread) : NULL)
#define MMAP(_evsel, _cpu_map_idx, _thread) \
(_evsel->mmap ? ((struct perf_mmap *) xyarray__entry(_evsel->mmap, _cpu_map_idx, _thread)) \
: NULL)
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2] libperf: Handle an evsel without an fd array in enable/disable
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-11-11 6:16 ` Namhyung Kim
@ 2026-11-11 10:16 ` TAKANO Mitsuhiro
2026-11-11 10:28 ` sashiko-bot
2 siblings, 1 reply; 9+ messages in thread
From: TAKANO Mitsuhiro @ 2026-11-11 10:16 UTC (permalink / raw)
To: linux-perf-users, Arnaldo Carvalho de Melo, Namhyung Kim
Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland, stable,
linux-kernel, TAKANO Mitsuhiro
A group led by a tool PMU event is opened with the members' group_fd set
to the leader's file descriptor. For a tool event that descriptor comes
from open("/proc/stat"), not from perf_event_open(), so a member the
kernel refuses is marked unsupported; perf stat then releases the fd
array of that member while the evsel stays in the group.
Enabling the group goes through evsel__enable_cpu(), which enables the
members of a mixed PMU group from userspace because the kernel does not
do it along with the leader. The member has no fd array left, so
perf_evsel__run_ioctl() reads its loop bound through xyarray__max_y()
and dereferences a NULL pointer:
$ perf stat -a -e '{user_time,cycles}' -- sleep 0.05
Segmentation fault
Guard the array in both places: FD() returns NULL when it is absent,
which perf_evsel__ioctl() and perf_evsel__read() already test for, and
xyarray__max_x()/max_y() return 0, so the enable and disable paths that
walk the array become no-ops; perf_evsel__ioctl() returns -1 for such an
evsel, as it already does for a closed fd. The FD() guard alone is not
enough because that loop bound is read first; measured on the same tree,
rc=139 with the FD() change only and rc=0 with both.
Verified on an arm64 KVM guest (Neoverse-N1) with a perf built from this
tree. '{user_time,cycles}' and '{system_time,cycles}' segfault on the
unpatched tree (rc=139) and exit 0 here, reporting user_time (resp.
system_time) and a '<not supported> cycles:u' line. Real-PMU groups
such as '{cycles,instructions}', 'perf stat' with no event and '-r 2'
print identical output on both trees, and 'perf test -v 10' passes the
same 78 subtests. The libperf tests report the same five test-evlist
failures on both trees: the events cannot be opened by this user
(perf_event_paranoid=4), and two cases cannot read the syscall tracepoint
id because /sys/kernel/debug is mounted root-only.
The events are still not counted: a member the kernel refused stays
unsupported. Hang Zhengyang's follow-up opens these members with
group_fd = -1 so that they are counted.
Written with Hermes Agent, an LLM assistant, which made the change and
wrote this changelog. I built perf with it, reproduced the segfault on
the unpatched tree and confirmed it is gone with the patch, and ran
'perf test' and the libperf tests on this box.
Fixes: 0b18bced5444 ("perf tool_pmu: Make tool PMU events respect enable/disable")
Link: https://lore.kernel.org/avQIOtt7Y67oXQem@z2
Suggested-by: Namhyung Kim <namhyung@kernel.org>
Cc: stable@vger.kernel.org
Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v2: follow Namhyung's suggestion and fix it in libperf instead of
skipping the members in tools/perf/util/evsel.c. The FD() guard
alone leaves the crash in place, because the loop bound is read
from the same NULL array through xyarray__max_y(), so the
xyarray accessors get a NULL check as well. Measured on the same
box with the FD() guard only: rc=139, with both: rc=0.
tools/lib/perf/evsel.c | 3 ++-
tools/lib/perf/include/internal/xyarray.h | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
index f747c0bc692d..e23df3b2c1f1 100644
--- a/tools/lib/perf/evsel.c
+++ b/tools/lib/perf/evsel.c
@@ -57,7 +57,8 @@ void perf_evsel__delete(struct perf_evsel *evsel)
}
#define FD(_evsel, _cpu_map_idx, _thread) \
- ((int *)xyarray__entry(_evsel->fd, _cpu_map_idx, _thread))
+ (_evsel->fd ? (int *)xyarray__entry(_evsel->fd, _cpu_map_idx, _thread) \
+ : NULL)
#define MMAP(_evsel, _cpu_map_idx, _thread) \
(_evsel->mmap ? ((struct perf_mmap *) xyarray__entry(_evsel->mmap, _cpu_map_idx, _thread)) \
: NULL)
diff --git a/tools/lib/perf/include/internal/xyarray.h b/tools/lib/perf/include/internal/xyarray.h
index f10af3da7b21..8bf76c12984e 100644
--- a/tools/lib/perf/include/internal/xyarray.h
+++ b/tools/lib/perf/include/internal/xyarray.h
@@ -32,12 +32,12 @@ static inline void *xyarray__entry(struct xyarray *xy, size_t x, size_t y)
static inline int xyarray__max_y(struct xyarray *xy)
{
- return xy->max_y;
+ return xy ? xy->max_y : 0;
}
static inline int xyarray__max_x(struct xyarray *xy)
{
- return xy->max_x;
+ return xy ? xy->max_x : 0;
}
#endif /* __LIBPERF_INTERNAL_XYARRAY_H */
base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
--
2.53.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2] libperf: Handle an evsel without an fd array in enable/disable
2026-11-11 10:16 ` [PATCH v2] libperf: Handle an evsel without an fd array in enable/disable TAKANO Mitsuhiro
@ 2026-11-11 10:28 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-11-11 10:28 UTC (permalink / raw)
To: TAKANO Mitsuhiro, linux-perf-users
> A group led by a tool PMU event is opened with the members' group_fd set
> to the leader's file descriptor. For a tool event that descriptor comes
> from open("/proc/stat"), not from perf_event_open(), so a member the
> kernel refuses is marked unsupported; perf stat then releases the fd
> array of that member while the evsel stays in the group.
>
> Enabling the group goes through evsel__enable_cpu(), which enables the
> members of a mixed PMU group from userspace because the kernel does not
> do it along with the leader. The member has no fd array left, so
> perf_evsel__run_ioctl() reads its loop bound through xyarray__max_y()
> and dereferences a NULL pointer:
>
> $ perf stat -a -e '{user_time,cycles}' -- sleep 0.05
> Segmentation fault
>
> [ ... ]
> Fixes: 0b18bced5444 ("perf tool_pmu: Make tool PMU events respect enable/disable")
> Link: https://lore.kernel.org/avQIOtt7Y67oXQem@z2
> Suggested-by: Namhyung Kim <namhyung@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
Sashiko has reviewed this patch and found no issues.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261111101625.2204836-1-takano32@gmail.com?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread