Re: [PATCH v2] perf stat: Start tool events when enable_on_exec is used

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf stat: NULL deref in enable_counters() on unsupported tool event
- [Medium] perf stat: inaccurate mixed group enable_on_exec handling and NULL fd crash
--

commit 3216cd73e8c11e372e4510bb56208ee17997bddb
Author: TAKANO Mitsuhiro <takano32@gmail.com>

perf stat: Start tool events when enable_on_exec is used

This commit starts emulated tool events explicitly via
evsel__tool_pmu_enable() in enable_counters() when target__enable_on_exec()
is true, avoiding calling evsel__enable() on mixed PMU groups.

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index c50cf230f16ed..ca4483f34addb 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -510,6 +510,18 @@ static int enable_counters(void)
>  	if (!target__enable_on_exec(&target)) {
>  		if (!all_counters_use_bpf)
>  			evlist__enable(evsel_list);
> +	} else {
> +		/*
> +		 * Tool events are emulated in perf, so enable_on_exec above
> +		 * does not start them.  Do not go through evsel__enable(),
> +		 * which would also enable the members of a mixed PMU group
> +		 * the kernel already handles.
> +		 */

[Severity: Medium]
Is this comment and the commit message's explanation of mixed PMU groups
accurate?

When running a mixed group led by a tool event such as
perf stat -e "{user_time,cycles}" -- sleep 0.1,
parse_events__sort_events_and_fix_groups() keeps both events in the same
group because perf_pmu__is_software() returns true for PERF_PMU_TYPE_TOOL
in evsel__compute_group_pmu_name().

During __run_perf_stat(), evsel__tool_pmu_open() stores the procfs fd in
FD(user_time, idx, thread). When opening the member cycles in
evsel__open_cpu(), get_group_fd() in tools/perf/util/evsel.c does not check
evsel__is_non_perf_event_open_pmu(leader):

tools/perf/util/evsel.c:get_group_fd() {
	...
	fd = FD(leader, cpu_map_idx, thread);
	BUG_ON(fd == -1 && leader->supported);
	...
	return fd == -1 ? -2 : fd;
}

For user_time or system_time, get_group_fd() passes the procfs fd as
group_fd to sys_perf_event_open(), which fails with -EINVAL, sets
cycles->supported = false, and causes __run_perf_stat() to free
cycles->core.fd to NULL via perf_evsel__free_fd(). (For
"{duration_time,cycles}", FD(duration_time, 0, 0) is -1 while
duration_time->supported is true, hitting BUG_ON(fd == -1 &&
leader->supported) in get_group_fd().)

Additionally, in create_perf_stat_counter() in tools/perf/builtin-stat.c:

	if (evsel__is_group_leader(evsel)) {
		attr->disabled = 1;

		if (target__enable_on_exec(&target))
			attr->enable_on_exec = 1;
	}

attr->disabled and attr->enable_on_exec are only set on the group leader.
Because of this:

1. Kernel members under a tool group leader do not have enable_on_exec set,
   and their fds are NULL here because sys_perf_event_open() failed with
   -EINVAL in get_group_fd(), rather than being unavailable in this process
   or started by the kernel on exec.

2. Tool members under a kernel group leader (for example,
   "{cycles,duration_time}" or "{cycles,user_time}") have attr->disabled == 0,
   so evsel__tool_pmu_open() sets evsel->disabled = false and starts
   counting immediately at open time rather than when enable_counters()
   runs.

3. Because this patch only avoids evsel__enable() in the
   target__enable_on_exec(&target) branch of enable_counters(),
   evsel__enable_cpu(), evsel__enable(), evsel__disable_cpu(), and
   evsel__disable() in tools/perf/util/evsel.c still iterate over
   for_each_group_member(member, evsel) without checking member->core.fd !=
   NULL, which will still dereference a NULL member->core.fd in
   perf_evsel__run_ioctl() -> xyarray__max_y(evsel->fd) whenever
   !target__enable_on_exec(&target) or !target__none(&target) is true (such
   as with -a, -C, -p, -t, or -D).

Should mixed group opening in get_group_fd(), member attribute setup in
create_perf_stat_counter(), and NULL core.fd checks in the evsel
enable/disable helpers be addressed instead?

> +		evlist__for_each_entry(evsel_list, evsel) {
> +			if (evsel__is_tool(evsel) &&
> +			    !evsel__tool_pmu_enable(evsel))
> +				evsel->disabled = false;
> +		}

[Severity: Medium]
Can this loop dereference a NULL evsel->core.fd when a tool event is
unsupported?

Before calling enable_counters(), __run_perf_stat() in
tools/perf/builtin-stat.c frees counter->core.fd (setting it to NULL) for
any unsupported event:

	evlist__for_each_entry(evsel_list, counter) {
		if (!counter->supported) {
			perf_evsel__free_fd(&counter->core);
			continue;
		}

Unlike __evlist__enable() (which checks !pos->core.fd) and
read_counter_cpu() (which checks !counter->supported), this loop calls
evsel__tool_pmu_enable(evsel) without checking evsel->supported or
evsel->core.fd.

In evsel__tool_pmu_open() in tools/perf/util/tool_pmu.c:

				if (evsel->core.attr.disabled) {
					evsel->disabled = true;
					*start_time = INVALID_START_TIME;

evsel->disabled is set to true as soon as an entry is opened with
attr.disabled == 1, and out_close does not reset evsel->disabled if a failure
occurs.

When target__enable_on_exec(&target) is true, a user_time or system_time
event can end up with evsel->disabled == true, evsel->supported == false,
and evsel->core.fd == NULL (for example, if a weak group "{user_time,cycles}:W"
resets and fails to reopen user_time on the second pass, if a later CPU index
fails to open when evlist__has_bpf_output() is true, or across repeat runs
after --control disables counters).

When that happens, evsel__tool_pmu_enable() -> evsel__tool_pmu_enable_cpu()
-> tool_pmu__read_stat() evaluates FD(evsel, cpu_map_idx, thread) and
dereferences evsel->core.fd == NULL.

Should this check evsel->supported (or evsel->core.fd) before calling
evsel__tool_pmu_enable(evsel)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023085554.1694434-1-takano32@gmail.com?part=1