Re: [PATCH v1 28/41] perf pmu: Add perf_pmu__probe() for basic open, counting, and sampling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf pmu: collect_probe_candidate_cb() stops on unopenable ftrace event
- [Medium] perf pmu: raw attr fallback in __perf_pmu__probe() clobbers open_err
--

commit 006ae863dd0267ee072ef7ac0650c59a05a689b6
Author: Ian Rogers <irogers@google.com>

perf pmu: Add perf_pmu__probe() for basic open, counting, and sampling

This commit adds struct perf_pmu::probed and perf_pmu__probe() to actively
probe a live PMU via perf_event_open() on a worker thread pinned to an
online CPU associated with the PMU. It selects a representative probe event
across PMU types and records whether opening, counting, and sampling are
supported.

> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index fecfe791bf8ae..9033bada9f472 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c

[ ... ]

> +static int collect_probe_candidate_cb(void *vstate, struct pmu_event_info *info)
> +{
> +	struct probe_candidates *cands = vstate;
> +	char buf[512];
> +
> +	if (info->deprecated)
> +		return 0;
> +	if (info->str && strstr(info->str, "=?"))
> +		return 0;
> +	if (!info->name)
> +		return 0;
> +
> +	if (info->pmu && info->pmu->type == PERF_TYPE_TRACEPOINT) {
> +		probe_candidates__add(cands, info->name);
> +		return 1;
> +	} else if (strchr(info->name, '/')) {

[Severity: Medium]
When __perf_pmu__probe() cannot open any of the three hardcoded tracepoints
in fast_tps and falls back to perf_pmu__for_each_event(),
collect_probe_candidate_cb() adds only the very first enumerated tracepoint
and returns 1 to stop iteration.

Because the kernel registers __start_ftrace_events first in tracefs and
tp_pmu__for_each_tp_sys() reads /sys/kernel/tracing/events in directory
order, the first enumerated event is typically an internal ftrace:* event
(such as ftrace:bprint or ftrace:print) whose tp_event->class->reg is NULL,
which perf_trace_init() rejects with -EINVAL.

Should collect_probe_candidate_cb() skip internal ftrace:* events or collect
up to MAX_PROBE_CANDIDATES tracepoints before stopping, so the fallback does
not fail with EINVAL on the first ftrace event?

[ ... ]

> +static void __perf_pmu__probe(struct perf_pmu *pmu, int pmu_cpu)
> +{
> +	struct probe_candidates cands = { .nr = 0 };
> +	struct perf_event_attr base_attr;
> +	char uprobe_exe_path[PATH_MAX];
> +	__u64 uprobe_offset = 0;
> +	unsigned long flags = perf_event_open_cloexec_flag();
> +	int keepalive_fd = -1;
> +	pid_t pid = 0;
> +	int cpu = 0;
> +	int last_err = 0;
> +	bool found = false;
> +
> +	if (pmu->type == PERF_TYPE_TRACEPOINT) {
> +		static const struct {
> +			const char *sys;
> +			const char *name;
> +		} fast_tps[] = {
> +			{ "sched", "sched_switch" },
> +			{ "raw_syscalls", "sys_enter" },
> +			{ "irq", "irq_handler_entry" },
> +		};
> +
> +		for (size_t i = 0; i < ARRAY_SIZE(fast_tps); i++) {
> +			int tp_id = tp_pmu__id(fast_tps[i].sys, fast_tps[i].name);
> +
> +			if (tp_id >= 0) {

[ ... ]

> +	if (!found && cands.nr == 0) {
> +		memset(&base_attr, 0, sizeof(base_attr));
> +		base_attr.type = pmu->type;
> +		base_attr.size = sizeof(base_attr);
> +		if (pmu->perf_event_attr_init_default)
> +			pmu->perf_event_attr_init_default(pmu, &base_attr);
> +		if (try_open_attr_variants(&base_attr, pmu->is_uncore, pmu_cpu,
> +					   flags, &pid, &cpu, &last_err)) {
> +			char buf[256];
> +
> +			snprintf(buf, sizeof(buf), "%s//", pmu->name);
> +			pmu->probed.probe_event = strdup(buf);
> +			found = true;
> +		}
> +	}

[Severity: Medium]
When probing a tracepoint, kprobe, uprobe, or breakpoint PMU fails in
__perf_pmu__probe(), does falling through into this cands.nr == 0 block
clobber pmu->probed.open_err with EINVAL?

The kprobe, uprobe, and PERF_TYPE_BREAKPOINT branches probe their targets
directly without populating cands, and the PERF_TYPE_TRACEPOINT branch
ignores negative error returns (such as -EACCES when tracefs is mode 0700)
from tp_pmu__id() and perf_pmu__for_each_event(), leaving cands.nr == 0 and
last_err == 0.

Because none of those PMUs defines perf_event_attr_init_default, the
cands.nr == 0 fallback calls try_open_attr_variants() with a zeroed
base_attr (config == 0, kprobe_func == NULL, uprobe_path == NULL, or
bp_type == HW_BREAKPOINT_EMPTY). The kernel rejects all 16 attempts with
-EINVAL, and try_open_attr_variants() overwrites *last_err whenever it is 0
or ENOENT, replacing the original failure reason with EINVAL.

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