Re: [PATCH v1 32/41] perf pmu: Probe execution and AUX area features

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf pmu: false can_inherit on CPU-wide PMUs in __perf_pmu__probe()
- [Medium] perf pmu: unclamped sample_freq=4000 in __perf_pmu__probe() freq probe
--

commit 6dd54e3c43c6f5454547c748099797fce6fa163c
Author: Ian Rogers <irogers@google.com>

perf pmu: Probe execution and AUX area features

This commit extends perf_pmu__probe() to probe event execution, grouping,
auto-counter reload, and AUX area features via perf_event_open() and
ioctl(PERF_EVENT_IOC_ENABLE).

> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index a1e27e678ccfc..e7e05e29b695d 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c

[ ... ]

> @@ -3576,6 +3659,256 @@ static void __perf_pmu__probe(struct perf_pmu *pmu, int pmu_cpu)

[ ... ]

> +	{
> +		struct perf_event_attr inh_attr = base_attr;
> +		int fd;
> +
> +		inh_attr.disabled = 1;
> +		inh_attr.inherit = 1;
> +		fd = sys_perf_event_open(&inh_attr, pid, cpu, -1, flags);
> +		if (fd >= 0) {
> +			close(fd);
> +			pmu->probed.can_inherit = true;
> +		}
> +	}

[Severity: Medium]
Will this falsely set pmu->probed.can_inherit to true on uncore and
CPU-wide-only PMUs?

Earlier in __perf_pmu__probe()->try_open_attr_variants(), pid is set to -1
first for uncore PMUs and as a fallback for any PMU that rejects per-task
events:

tools/perf/util/pmu.c:try_open_attr_variants() {
	if (is_uncore) {
		pids[0] = -1;
		pids[1] = 0;
	} else {
		pids[0] = 0;
		pids[1] = -1;
	}
}

In the kernel, when pid is -1, task is NULL. perf_event_alloc() only
rejects perf_invalid_context PMUs when task is non-NULL (or cgroup_fd != -1)
and ignores attr.inherit = 1 on CPU-wide events:

kernel/events/core.c:perf_event_alloc() {
	if (pmu->task_ctx_nr == perf_invalid_context && (task || cgroup_fd != -1))
		return ERR_PTR(-EINVAL);
}

As a result, calling sys_perf_event_open(&inh_attr, pid, cpu, -1, flags)
with pid == -1 succeeds on uncore and CPU-wide-only PMUs even though those
PMUs cannot monitor tasks or inherit events across child tasks. Should the
can_inherit probe explicitly pass pid = 0 instead of pid?

> +
> +	if (pmu->probed.can_sample) {
> +		struct perf_event_attr freq_attr = base_attr;
> +		int fd;
> +
> +		freq_attr.disabled = 1;
> +		freq_attr.freq = 1;
> +		freq_attr.sample_freq = 4000;
> +		freq_attr.sample_type = PERF_SAMPLE_IP;
> +		fd = sys_perf_event_open(&freq_attr, pid, cpu, -1, flags);
> +		if (fd >= 0) {
> +			close(fd);
> +			pmu->probed.can_freq = true;
> +		}
> +	}

[Severity: Medium]
Can hardcoding sample_freq = 4000 here cause pmu->probed.can_freq to be
falsely reported as false when kernel.perf_event_max_sample_rate has been
lowered?

In SYSCALL_DEFINE5(perf_event_open), the kernel rejects frequency mode
events when attr.sample_freq exceeds sysctl_perf_event_sample_rate:

kernel/events/core.c:SYSCALL_DEFINE5(perf_event_open) {
	if (attr.freq) {
		if (attr.sample_freq > sysctl_perf_event_sample_rate)
			return -EINVAL;
	}
}

Because perf_sample_event_took() dynamically throttles
sysctl_perf_event_sample_rate down to as low as 1 * HZ when PMIs take too
long (or an administrator may configure a limit below 4000), should this
probe use a minimal value like sample_freq = 1 (or clamp to
kernel/perf_event_max_sample_rate like record_opts__config_freq() does)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103234502.1008382-1-irogers@google.com?part=32