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

[sashiko-bot@kernel.org]

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