Re: [Patch v6 06/20] perf/x86/amd: Avoid re-enabling PMU and LBR for BPF when disabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf/x86/amd: amd_pmu_v2_snapshot_branch_stack() leaves cpuc->enabled set
--

commit dbf85e8b6a5abeff651d80166ad0c339896c5b42
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86/amd: Avoid re-enabling PMU and LBR for BPF when disabled

This commit updates amd_pmu_v2_snapshot_branch_stack() to save cpuc->enabled
and cpuc->lbr_enabled before disabling the PMU and LBR, and only re-enable
them if they were previously enabled.

> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index bf631f60757cc..851d8a938f522 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -932,22 +932,28 @@ static int amd_pmu_v2_snapshot_branch_stack(struct perf_branch_entry *entries, u
>  {
>  	struct cpu_hw_events *cpuc;
>  	unsigned long flags;
> +	int pmu_enabled;
> +	int lbr_enabled;
>  
>  	/*
>  	 * The sequence of steps to freeze LBR should be completely inlined
>  	 * and contain no branches to minimize contamination of LBR snapshot
>  	 */
>  	local_irq_save(flags);
> +	cpuc = this_cpu_ptr(&cpu_hw_events);
> +	pmu_enabled = cpuc->enabled;
> +	lbr_enabled = cpuc->lbr_enabled;
>  	amd_pmu_core_disable_all();
>  	__amd_pmu_lbr_disable();
>  
> -	cpuc = this_cpu_ptr(&cpu_hw_events);
> -
>  	amd_pmu_lbr_read();
>  	cnt = min(cnt, cpuc->lbr_stack.nr);
>  	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
>  
> -	amd_pmu_v2_enable_all(0);
> +	if (lbr_enabled)
> +		amd_pmu_lbr_enable_all();
> +	if (pmu_enabled)
> +		amd_pmu_core_enable_all();

[Severity: Medium]
As reported by Sashiko on v5 [1], while __amd_pmu_lbr_disable() clears
cpuc->lbr_enabled = 0, amd_pmu_core_disable_all() only clears the hardware
global control MSR and leaves cpuc->enabled set to 1.

If a BPF program in non-NMI context calls bpf_get_branch_snapshot() ->
amd_pmu_v2_snapshot_branch_stack() while cpuc->enabled is 1, local_irq_save()
does not mask NMIs. If an NMI arrives while amd_pmu_v2_snapshot_branch_stack()
is reading LBR MSRs or copying entries, perf_event_nmi_handler() ->
amd_pmu_v2_handle_irq() will see cpuc->enabled == 1 and re-enable the core PMU
counters on exit:

arch/x86/events/amd/core.c:amd_pmu_v2_handle_irq() {
	pmu_enabled = cpuc->enabled;
	cpuc->enabled = 0;

	/* Stop counting but do not disable LBR */
	amd_pmu_core_disable_all();
	...
done:
	cpuc->enabled = pmu_enabled;

	/* Resume counting only if PMU is active */
	if (pmu_enabled)
		amd_pmu_core_enable_all();
}

Similarly, if an NMI triggers a nested BPF call to
amd_pmu_v2_snapshot_branch_stack(), the nested call will also observe
cpuc->enabled == 1 and call amd_pmu_core_enable_all() before returning to the
outer snapshot.

Should amd_pmu_v2_snapshot_branch_stack() clear cpuc->enabled = 0 after saving
pmu_enabled and restore cpuc->enabled = pmu_enabled before re-enabling the
core PMU, matching amd_pmu_v2_handle_irq()?

[1] https://lore.kernel.org/r/20261029043914.1395147-7-dapeng1.mi@linux.intel.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106043257.842864-1-dapeng1.mi@linux.intel.com?part=6