Re: [PATCH v1 34/41] perf pmu: Probe user and interrupt sample register masks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf pmu: probe_pmu_regs_one() skips non-PEBS extended GPRs via reg_mask()
- [Medium] perf pmu: probe_pmu_regs_one() probes invalid extended regs on arm64/powerpc
--

commit 0bf5f581edb6cbd00d2dffa574e5d67ba1f8061e
Author: Ian Rogers <irogers@google.com>

perf pmu: Probe user and interrupt sample register masks

This commit extends perf_pmu__probe() with probe_pmu_regs_one() to probe
supported general-purpose, extended general-purpose, and SIMD register
sampling masks for PERF_SAMPLE_REGS_USER and PERF_SAMPLE_REGS_INTR.

> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 3585147c52422..9179231aa7d5a 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -3386,6 +3386,237 @@ static void probe_sample_types(struct perf_pmu *pmu,
>  	pmu->probed.can_weight = !!(pmu->probed.sample_types & PERF_SAMPLE_WEIGHT);
>  }
>  
> +/*
> + * Probe supported general-purpose, extended GPR, and SIMD vector/predicate
> + * register sampling masks for either PERF_SAMPLE_REGS_USER (!@intr) or
> + * PERF_SAMPLE_REGS_INTR (@intr).
> + */
> +static void probe_pmu_regs_one(const struct perf_pmu *pmu,
> +			       const struct perf_event_attr *base_attr,
> +			       pid_t pid, int cpu, unsigned long flags,
> +			       int max_precise, bool intr,
> +			       u64 *out_mask, int *out_abi,
> +			       u64 *out_simd_mask, u64 *out_pred_mask)
> +{
> +	int dummy_abi = -1;
> +	u64 gpr_mask = intr ? perf_intr_reg_mask(EM_HOST, &dummy_abi) :
> +			      perf_user_reg_mask(EM_HOST, &dummy_abi);
> +	u64 sample_type = intr ? PERF_SAMPLE_REGS_INTR : PERF_SAMPLE_REGS_USER;
> +	u64 mask = 0;
> +	int abi = 0;
> +	struct perf_event_attr attr = *base_attr;
> +	int fd;
> +
> +	attr.disabled = 1;
> +	attr.sample_period = base_attr->sample_period ?: 10000;
> +	attr.sample_type = sample_type;
> +
> +	if (gpr_mask) {
> +		if (intr)
> +			attr.sample_regs_intr = gpr_mask;
> +		else
> +			attr.sample_regs_user = gpr_mask;
> +
> +		fd = sys_perf_event_open(&attr, pid, cpu, -1, flags);
> +		if (fd >= 0) {
> +			close(fd);
> +			mask |= gpr_mask;
> +		} else {
> +			for (int b = 0; b < 64; b++) {
> +				if (!(gpr_mask & BIT_ULL(b)))
> +					continue;
> +				if (intr)
> +					attr.sample_regs_intr = BIT_ULL(b);
> +				else
> +					attr.sample_regs_user = BIT_ULL(b);
> +				fd = sys_perf_event_open(&attr, pid, cpu, -1, flags);
> +				if (fd >= 0) {
> +					close(fd);
> +					mask |= BIT_ULL(b);
> +				}
> +			}
> +		}
> +	}
> +
> +	if (perf_pmu__is_software(pmu)) {
> +		*out_mask = mask;
> +		*out_abi = abi;
> +		return;
> +	}
> +
> +	/*
> +	 * Probe extended GPRs outside the base GPR mask when the host ABI
> +	 * supports PERF_SAMPLE_REGS_ABI_SIMD (e.g. APX R16-R31 and CET SSP).
> +	 * Note: perf_pmu__probe() actively probes the running host kernel via
> +	 * perf_event_open(), so EM_HOST is used intentionally here.
> +	 */
> +	{
> +		int host_abi = 0;
> +		u64 host_mask = intr ? perf_intr_reg_mask(EM_HOST, &host_abi) :
> +				       perf_user_reg_mask(EM_HOST, &host_abi);
> +		u64 ext_gpr_mask = host_mask & ~gpr_mask;
> +
> +		if ((host_abi & PERF_SAMPLE_REGS_ABI_SIMD) && ext_gpr_mask) {

[Severity: Medium]
Does gating this probe on perf_intr_reg_mask() and perf_user_reg_mask()
skip probing extended GPRs on x86 systems that support CET SSP or APX
extended GPRs on non-PEBS events, but do not have Architectural PEBS?

On x86, perf_intr_reg_mask() and perf_user_reg_mask() call
__perf_reg_mask_x86() -> __arch__reg_mask(), which hardcodes
attr.precise_ip = 1 (and on hybrid systems only checks the first core PMU):

tools/perf/util/perf-regs-arch/perf_regs_x86.c:__arch__reg_mask() {
	struct perf_event_attr attr = {
		.type				= PERF_TYPE_HARDWARE,
		.config				= PERF_COUNT_HW_CPU_CYCLES,
		.sample_type			= sample_type,
		.precise_ip			= 1,
		.disabled			= 1,
		.exclude_kernel			= 1,
		.sample_simd_regs_enabled	= has_simd_regs,
	};
}

In the x86 kernel, x86_pmu_hw_config() allows sampling SSP and APX extended
GPRs via xsaves_nmi() when precise_ip == 0 (validated by
event_simd_regs_validate()), whereas precise_ip > 0 additionally runs
pebs_simd_regs_validate():

arch/x86/events/core.c:pebs_simd_regs_validate() {
	if (event_needs_ssp(event) &&
	    !(x86_pmu.arch_pebs && (caps & ARCH_PEBS_GPR)))
		return -EINVAL;
	if (event_needs_ymm(event) &&
	    !(x86_pmu.arch_pebs && (caps & ARCH_PEBS_VECR_YMMH)))
		return -EINVAL;
	if (event_needs_egprs(event) &&
	    !(x86_pmu.arch_pebs && (caps & ARCH_PEBS_VECR_EGPRS)))
		return -EINVAL;
}

On CET-capable CPUs without Architectural PEBS (or in VMs without PEBS),
__arch__reg_mask() fails with precise_ip = 1, so __perf_reg_mask_x86()
returns PERF_REGS_MASK with host_abi = 0 and ext_gpr_mask = 0. Because the
if condition here is false, probe_pmu_regs_one() never runs the e_attr probe
below that tries base_attr->precise_ip (0) before falling back to
precise_ip = 1, leaving SSP and R16..R31 out of out_mask and out_abi.

[ ... ]

> +	if (!(abi & PERF_SAMPLE_REGS_ABI_SIMD)) {
> +		u64 ext_mask = GENMASK_ULL(63, 32) & ~gpr_mask;
> +		struct perf_event_attr x_attr = *base_attr;
> +		bool batch_ok = false;
> +
> +		x_attr.disabled = 1;
> +		x_attr.sample_period = base_attr->sample_period ?: 10000;
> +		x_attr.sample_type = sample_type;
> +		if (intr && max_precise > 0)
> +			x_attr.precise_ip = 1;
> +
> +		if (ext_mask) {
> +			if (intr)
> +				x_attr.sample_regs_intr = ext_mask;
> +			else
> +				x_attr.sample_regs_user = ext_mask;
> +
> +			fd = sys_perf_event_open(&x_attr, pid, cpu, -1, flags);
> +			if (fd >= 0) {
> +				close(fd);
> +				mask |= ext_mask;
> +				batch_ok = true;
> +			}
> +		}
> +
> +		if (!batch_ok) {
> +			for (int b = 0; b < 64; b++) {
> +				const char *rname;
> +
> +				if (gpr_mask & BIT_ULL(b))
> +					continue;
> +				rname = perf_reg_name(b, EM_HOST, EF_HOST, 0);
> +				if (!rname || !strcmp(rname, "unknown"))
> +					continue;
> +				if (intr)
> +					x_attr.sample_regs_intr = BIT_ULL(b);
> +				else
> +					x_attr.sample_regs_user = BIT_ULL(b);
> +				fd = sys_perf_event_open(&x_attr, pid, cpu, -1, flags);
> +				if (fd >= 0) {
> +					close(fd);
> +					mask |= BIT_ULL(b);
> +				}
> +			}
> +		}
> +	}

[Severity: Medium]
On non-x86 architectures such as arm64 and powerpc, can this fallback loop
record user-only extended registers in intr_regs_mask and interrupt-only
extended registers in user_regs_mask?

At the start of probe_pmu_regs_one(), dummy_abi = -1 is passed to
perf_intr_reg_mask() and perf_user_reg_mask():

tools/perf/util/perf_regs.c:perf_intr_reg_mask() {
	case EM_AARCH64:
		mask = __perf_reg_mask_arm64(/*intr=*/true);
		break;
	...
	case EM_PPC:
	case EM_PPC64:
		mask = __perf_reg_mask_powerpc(/*intr=*/true);
		break;
}

Because only EM_386 and EM_X86_64 inspect the abi argument in
perf_intr_reg_mask() and perf_user_reg_mask(), gpr_mask on arm64 (with SVE)
and powerpc (Power9/10/11) already includes direction-valid extended
registers. For software PMUs and PMUs without PERF_PMU_CAP_EXTENDED_REGS,
that causes the initial batch gpr_mask probe to fail in perf_event_alloc()
with -EOPNOTSUPP and fall back to 64 individual syscalls.

Then, on hardware PMUs with PERF_PMU_CAP_EXTENDED_REGS, the batch probe of
ext_mask = GENMASK_ULL(63, 32) & ~gpr_mask fails with -EINVAL because bits
32..63 include reserved register bits. The fallback loop then probes every
bit not in gpr_mask that has a valid perf_reg_name().

On arm64 when intr is true, __perf_reg_mask_arm64(true) intentionally
excluded PERF_REG_ARM64_VG (bit 46), and on powerpc when intr is false,
__perf_reg_mask_powerpc(false) intentionally excluded the PMU SPRs
PERF_REG_POWERPC_MMCR0..SIAR (bits 44..57). However, perf_reg_name() returns
valid names for those bits, and the kernel's perf_reg_validate() on arm64
and powerpc does not distinguish sample_regs_user from sample_regs_intr:

arch/powerpc/perf/perf_regs.c:perf_reg_validate() {
	if (!mask || mask & REG_RESERVED)
		return -EINVAL;
	return 0;
}

As a result, sys_perf_event_open() succeeds and adds PERF_REG_ARM64_VG to
pmu->probed.intr_regs_mask on arm64 and PERF_REG_POWERPC_MMCR0..SIAR to
pmu->probed.user_regs_mask on powerpc.

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