Re: [PATCH 3/5] alpha/perf: implement ProfileMe sampling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] alpha/perf: wrong register and stack state in alpha_perf_event_profileme()
--

commit 4ba1c82f19c556ae6c582ff386ff198b05c6a1eb
Author: Matt Turner <mattst88@gmail.com>

alpha/perf: implement ProfileMe sampling

This commit implements ProfileMe performance counter sampling on Alpha
EV67 and later processors, selecting it via the profileme config attribute
or precise_ip and attributing samples to the PC latched in PMPC.

> diff --git a/arch/alpha/kernel/perf_event.c b/arch/alpha/kernel/perf_event.c
> index 70840c05d39bf..24c96feda9424 100644
> --- a/arch/alpha/kernel/perf_event.c
> +++ b/arch/alpha/kernel/perf_event.c

[ ... ]

> @@ -832,6 +904,61 @@ void perf_event_print_debug(void)
>  }
>  
>  
> +/*
> + * Handle the close of a ProfileMe window on PCTR0.
> + *
> + * PCTR0 stops counting when the window closes and does not start again
> + * until PCTR_CTL is written, so the status must be read first and the
> + * counters then rearmed unconditionally.
> + */
> +static void alpha_perf_event_profileme(struct perf_event *event,
> +				       struct hw_perf_event *hwc,
> +				       struct pt_regs *regs)
> +{
> +	struct perf_sample_data data;
> +	struct pt_regs pm_regs;
> +	unsigned long i_stat, pmpc, ovr;
> +
> +	i_stat = wrperfmon(PERFMON_CMD_I_STAT, 0);
> +	pmpc = wrperfmon(PERFMON_CMD_PMPC, 0);
> +
> +	alpha_perf_event_update(event, hwc, 0, alpha_pmu->pmc_max_period[0]+1);
> +
> +	/*
> +	 * When PCTR0 counts instructions, up to 7 of them are counted after
> +	 * the overflow but before the window opens. I_STAT reports how many.
> +	 */
> +	if (!(hwc->config_base & EV67_PCTR_PCTR0_CYCLES)) {
> +		ovr = (i_stat >> EV67_I_STAT_OVR_SHIFT) & EV67_I_STAT_OVR_MASK;
> +		local64_sub(ovr, &event->count);
> +		local64_add(ovr, &hwc->period_left);
> +	}
> +
> +	/*
> +	 * Attribute the sample to the profiled instruction rather than to
> +	 * wherever the interrupt landed, including the user/kernel mode that
> +	 * perf derives from regs. A PALmode PMPC is a physical address that
> +	 * perf cannot resolve. That happens for ITB miss, Istream access
> +	 * violation and interrupt redirects, and for instructions inside
> +	 * PALcode. Keep the interrupted PC, which for redirects is the
> +	 * instruction that trapped.
> +	 */
> +	pm_regs = *regs;
> +	if (!(pmpc & EV67_PMPC_PAL)) {
> +		pm_regs.pc = pmpc & EV67_PMPC_PC_MASK;
> +		/* PS bit 3 is the mode bit that user_mode() tests. */
> +		if (pm_regs.pc < TASK_SIZE)
> +			pm_regs.ps |= 8UL;
> +		else
> +			pm_regs.ps &= ~8UL;
> +	}
> +
> +	perf_sample_data_init(&data, 0, hwc->last_period);
> +
> +	if (alpha_perf_event_set_period(event, hwc, 0))
> +		perf_event_overflow(event, &data, &pm_regs);
> +}

[Severity: High]
When a user-mode instruction is profiled (pm_regs.pc < TASK_SIZE), but the
PMI is delivered after the CPU has entered kernel mode (!user_mode(regs)),
regs is the kernel interrupt trap frame containing live kernel GPRs, while
the user task's registers live in task_pt_regs(current).

By copying *regs into pm_regs and setting pm_regs.ps |= 8UL before calling
perf_event_overflow(), user_mode(&pm_regs) evaluates to true in
kernel/events/core.c:perf_sample_regs_user():

	if (user_mode(regs)) {
		regs_user->abi = perf_reg_abi(current);
		regs_user->regs = regs;
	} else if (is_user_task(current)) {
		perf_get_regs_user(regs_user, regs);
	}

and in kernel/events/core.c:perf_sample_regs_intr():

	if (exclude_kernel && !user_mode(regs)) {
		regs_intr->abi = PERF_SAMPLE_REGS_ABI_NONE;
		regs_intr->regs = NULL;
	} else {
		regs_intr->regs = regs;
		regs_intr->abi = perf_reg_abi(current);
	}

Doesn't this cause PERF_SAMPLE_REGS_USER and PERF_SAMPLE_REGS_INTR (even
when attr.exclude_kernel is set) to record live kernel registers from the
kernel interrupt trap frame into the user-visible perf ring buffer instead
of fetching task_pt_regs(current), while also corrupting user DWARF
callgraph unwinding?

Additionally, if current is a kernel thread when this happens,
perf_sample_regs_user() sets regs_user->regs = &pm_regs while
perf_sample_ustack_size() returns 0 (since current->mm is NULL), which
causes perf_prepare_sample() and perf_output_sample_ustack() to disagree
on the PERF_SAMPLE_STACK_USER record size.

Similarly, when !user_mode(&pm_regs) is true (pm_regs.pc >= TASK_SIZE),
passing the stack-local &pm_regs to perf_event_overflow() affects
arch/alpha/kernel/perf_regs.c:perf_reg_value():

	if (idx == PERF_REG_ALPHA_SP) {
		if (user_mode(regs))
			return user_stack_pointer(regs);
		if (!object_is_on_stack(regs))
			return 0;
		/* Same kernel sp approximation as stacktrace.c. */
		return (unsigned long)(regs + 1);
	}

and arch/alpha/kernel/stacktrace.c:arch_stack_walk():

	if (regs && task == current) {
		ksp = (unsigned long)(regs + 1);
	}

Because pm_regs is a local variable on the stack, object_is_on_stack(&pm_regs)
is true, so &pm_regs + 1 points inside alpha_perf_event_profileme()'s own
stack frame rather than the end of the interrupted trap frame (regs + 1).
And if the PMI landed in user mode after a profiled kernel instruction,
clearing bit 3 of pm_regs.ps causes perf_callchain_kernel() and
PERF_SAMPLE_REGS_INTR to treat the user register frame as a kernel frame
and unwind the kernel stack starting from &pm_regs + 1.

Could the PC and PS adjustments be made in-place on regs (and restored
after perf_event_overflow()) only when the PMPC mode matches the
interrupted mode in regs, or how should mode-mismatched samples handle register
and stack sampling?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-alpha-profileme-v1-0-d44b52bf2500@gmail.com?part=3