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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf stat: NULL dereference in enable_counters() on unsupported events
--

commit 63681c497cb81f27ece4b03357873947bd854a0c
Author: TAKANO Mitsuhiro <takano32@gmail.com>

perf stat: Start tool events when enable_on_exec is used

This commit explicitly enables tool events in enable_counters() when
target__enable_on_exec() is true, since tool events are emulated in userspace
and are not started by the kernel on exec.

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index c50cf230f16ed..2062623682fb2 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -510,6 +510,15 @@ 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.
> +		 */
> +		evlist__for_each_entry(evsel_list, evsel) {
> +			if (evsel__is_tool(evsel))
> +				evsel__enable(evsel);
> +		}
>  	}
>  	return 0;
>  }

[Severity: Medium]
Can calling evsel__enable(evsel) here without checking evsel->supported or
evsel->core.fd dereference a null pointer when an event fails to open?

Prior to calling enable_counters(), __run_perf_stat() frees core.fd for any
unsupported event:

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;
		}
	...
}

When target__enable_on_exec() is false, evlist__enable() ->
__evlist__enable() skips events where pos->core.fd is null:

tools/perf/util/evlist.c:__evlist__enable() {
	...
		if (!evsel__is_group_leader(pos) || !pos->core.fd)
			continue;
	...
}

In the new else branch, if a user_time or system_time event fails in
evsel__tool_pmu_open() after evsel->disabled = true is set (for example on a
subsequent thread or repeat run), evsel->supported is false and
evsel->core.fd is freed. Calling evsel__enable() then follows:

evsel__enable()
  -> evsel__tool_pmu_enable()
    -> evsel__tool_pmu_enable_cpu()
      -> tool_pmu__read_stat()

where tool_pmu__read_stat() dereferences evsel->core.fd via:

	int fd = FD(evsel, cpu_map_idx, thread);

Similarly, if a tool event such as user_time or system_time is the group
leader of a group whose member failed to open (such as
perf stat -e "{user_time,cycles}" -- sleep 0.1, where opening cycles with the
procfs fd as group_fd fails and frees cycles->core.fd), evsel__enable()
iterates over the group members:

tools/perf/util/evsel.c:evsel__enable() {
	...
		for_each_group_member(member, evsel) {
			if (evsel__is_non_perf_event_open_pmu(evsel) ||
			    evsel__is_non_perf_event_open_pmu(member)) {
				int mem_err = evsel__enable(member);
	...
}

which calls evsel__enable(member) -> perf_evsel__enable(&member->core) and
dereferences member->core.fd when evaluating xyarray__max_x(evsel->fd).

Should enable_counters() and the group member loop in evsel__enable() check
supported (or core.fd) before enabling?

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