[PATCH] perf evsel: Skip group members without fds when walking a group

[PATCH] perf evsel: Skip group members without fds when walking a group

[TAKANO Mitsuhiro]

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>
---
 tools/perf/util/evsel.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index d4cb455f4a7d..32c54cd4acf0 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;
 			if (evsel__is_non_perf_event_open_pmu(evsel) ||
 			    evsel__is_non_perf_event_open_pmu(member)) {
 				/*
@@ -1917,6 +1919,8 @@ int evsel__enable(struct evsel *evsel)
 		struct evsel *member;
 
 		for_each_group_member(member, evsel) {
+			if (!member->core.fd)
+				continue;
 			if (evsel__is_non_perf_event_open_pmu(evsel) ||
 			    evsel__is_non_perf_event_open_pmu(member)) {
 				/*
@@ -1952,6 +1956,8 @@ int evsel__disable_cpu(struct evsel *evsel, int cpu_map_idx)
 		struct evsel *member;
 
 		for_each_group_member(member, evsel) {
+			if (!member->core.fd)
+				continue;
 			if (evsel__is_non_perf_event_open_pmu(evsel) ||
 			    evsel__is_non_perf_event_open_pmu(member)) {
 				/*
@@ -1992,6 +1998,8 @@ int evsel__disable(struct evsel *evsel)
 		struct evsel *member;
 
 		for_each_group_member(member, evsel) {
+			if (!member->core.fd)
+				continue;
 			if (evsel__is_non_perf_event_open_pmu(evsel) ||
 			    evsel__is_non_perf_event_open_pmu(member)) {
 				/*

base-commit: f47d223627f80cb7a1eb14c585fe6cb35de5e984
-- 
2.53.0

Re: [PATCH] perf evsel: Skip group members without fds when walking a group

[sashiko-bot]

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

Re: [PATCH] perf evsel: Skip group members without fds when walking a group

[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.