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

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

[TAKANO Mitsuhiro]

duration_time is a tool PMU event emulated in perf, so the kernel never
sees it and enable_on_exec cannot start it.  perf stat enables the events
itself only when enable_on_exec is not used; with two or more events
target__enable_on_exec() is true, so the tool event is never started and
reads as zero:

  # perf stat -e duration_time -- sleep 0.1
          103813345      duration_time
  # perf stat -e task-clock,duration_time -- sleep 0.1
                    0      duration_time

The zero value also reaches the default CPUs_utilized metric, which
divides by duration_time, so a plain `perf stat` prints "nan CPUs".

Start the tool events explicitly when enable_on_exec is used.  The second
command above then reports 103991394 and the default output prints
"0.0 CPUs" rather than "nan CPUs".

Verified on an arm64 box (Ampere Altra, Neoverse-N1), including the -r,
-I and group forms of the command; x86 was not tested.

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/builtin-stat.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index c50cf230f16e..2062623682fb 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;
 }

base-commit: f47d223627f80cb7a1eb14c585fe6cb35de5e984
-- 
2.53.0

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

[sashiko-bot]

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

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

[TAKANO Mitsuhiro]

When perf stat runs a workload, target__enable_on_exec() is true and the
tool events are left to the kernel to start on exec.  Tool events are
however emulated in perf (duration_time, user_time, system_time,
num_cpus) and have nothing in the kernel to start them, so they stay
disabled and are read as zero.

For duration_time this makes CPUs_utilized divide by zero, and plain
"perf stat" prints "nan CPUs":

  $ perf stat -e task-clock,duration_time -- sleep 0.1
               0      duration_time

Start the tool events explicitly in that case.  They are started through
evsel__tool_pmu_enable() rather than evsel__enable(), because the latter
also enables the members of a mixed PMU group, whose fds are not
available in this process, and dereferences a NULL fd:

  $ perf stat -e '{user_time,cycles}' -- sleep 0.1
  Segmentation fault

The kernel starts the kernel-side members of such a group on exec, so
only the tool event itself is enabled here.

Fixes: 0b18bced5444 ("perf tool_pmu: Make tool PMU events respect enable/disable")
Assisted-by: LLM

---
v1 -> v2: Do not call evsel__enable(), which also enables the members of a
mixed PMU group and dereferences a NULL fd; call evsel__tool_pmu_enable()
instead.  Reported by Sashiko review.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 tools/perf/builtin-stat.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index c50cf230f16e..ca4483f34add 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.
+		 */
+		evlist__for_each_entry(evsel_list, evsel) {
+			if (evsel__is_tool(evsel) &&
+			    !evsel__tool_pmu_enable(evsel))
+				evsel->disabled = false;
+		}
 	}
 	return 0;
 }

base-commit: f47d223627f80cb7a1eb14c585fe6cb35de5e984
-- 
2.53.0

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

[sashiko-bot]

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

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

[TAKANO Mitsuhiro]

When perf stat runs a workload, target__enable_on_exec() is true and the
tool events are left to the kernel to start on exec.  Tool events are
however emulated in perf (duration_time, user_time, system_time,
num_cpus) and have nothing in the kernel to start them, so they stay
disabled and are read as zero.

For duration_time this makes CPUs_utilized divide by zero, and plain
"perf stat" prints "nan CPUs":

  $ perf stat -e task-clock,duration_time -- sleep 0.1
               0      duration_time

Start the tool events explicitly in that case.  They are started through
evsel__tool_pmu_enable() rather than evsel__enable(), because the latter
also enables the members of a mixed PMU group, and those cannot be used
here: get_group_fd() passes the leader's descriptor to
sys_perf_event_open(), which fails with EINVAL for a userspace PMU
leader, so the members end up unsupported with core.fd released.  Only
the tool event itself is started; the kernel starts the members of a
group it can open.

Skip the events that failed to open, the same way the evlist and the
counter read paths skip them, so a tool event that never opened is not
started and then read through a NULL descriptor.

Fixes: 0b18bced5444 ("perf tool_pmu: Make tool PMU events respect enable/disable")
Assisted-by: LLM

---
v1 -> v2: Do not call evsel__enable(), which also enables the members of a
mixed PMU group and dereferences a NULL fd; call evsel__tool_pmu_enable()
instead.  Reported by Sashiko review.

v2 -> v3: Also require evsel->supported, as reading a tool event that never
opened dereferences a NULL core.fd, and correct the explanation of why the
members of a mixed PMU group have no descriptor: get_group_fd() forwards the
leader's descriptor to sys_perf_event_open(), which fails with EINVAL for a
userspace PMU leader.  Reported by Sashiko review.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 tools/perf/builtin-stat.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index c50cf230f16e..334b875d6a2f 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -510,6 +510,19 @@ 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.  Start them here rather than through
+		 * evsel__enable(), which also walks the members of a mixed
+		 * PMU group; those are not opened here and their core.fd can
+		 * be NULL.
+		 */
+		evlist__for_each_entry(evsel_list, evsel) {
+			if (evsel__is_tool(evsel) && evsel->supported &&
+			    !evsel__tool_pmu_enable(evsel))
+				evsel->disabled = false;
+		}
 	}
 	return 0;
 }

base-commit: f47d223627f80cb7a1eb14c585fe6cb35de5e984
-- 
2.53.0

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

[sashiko-bot]

> When perf stat runs a workload, target__enable_on_exec() is true and the
> tool events are left to the kernel to start on exec.  Tool events are
> however emulated in perf (duration_time, user_time, system_time,
> num_cpus) and have nothing in the kernel to start them, so they stay
> disabled and are read as zero.
> 
> For duration_time this makes CPUs_utilized divide by zero, and plain
> "perf stat" prints "nan CPUs":
> 
>   $ perf stat -e task-clock,duration_time -- sleep 0.1
>                0      duration_time
> 
> Start the tool events explicitly in that case.  They are started through
> evsel__tool_pmu_enable() rather than evsel__enable(), because the latter
> also enables the members of a mixed PMU group, and those cannot be used
> here: get_group_fd() passes the leader's descriptor to
> sys_perf_event_open(), which fails with EINVAL for a userspace PMU
> leader, so the members end up unsupported with core.fd released.  Only
> the tool event itself is started; the kernel starts the members of a
> group it can open.
> 
> Skip the events that failed to open, the same way the evlist and the
> counter read paths skip them, so a tool event that never opened is not
> started and then read through a NULL descriptor.
> 
> Fixes: 0b18bced5444 ("perf tool_pmu: Make tool PMU events respect enable/disable")
> Assisted-by: LLM
> 

Sashiko has reviewed this patch and found no issues.

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