Re: [Patch v5 06/18] 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 while PMU is disabled
--

commit cb298524c7ea53df65b77e5a09ad097e570397f6
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 the current
PMU and LBR enabled state and only re-enable them after taking the LBR
snapshot if they were originally enabled. This avoids unconditionally
re-enabling the PMU and LBR when called from a PMI context where they were
already disabled.

> 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]
Should amd_pmu_v2_snapshot_branch_stack() in arch/x86/events/amd/core.c also
clear cpuc->enabled = 0 while the core PMU is disabled, and restore
cpuc->enabled = pmu_enabled before re-enabling it?

While __amd_pmu_lbr_disable() sets cpuc->lbr_enabled = 0 and
amd_pmu_lbr_enable_all() sets cpuc->lbr_enabled = 1, amd_pmu_core_disable_all()
only clears the hardware MSR and leaves cpuc->enabled set to 1.

If a BPF program running in non-NMI context (such as a kprobe, tracepoint, or
fentry/fexit hook) calls bpf_get_branch_snapshot() ->
amd_pmu_v2_snapshot_branch_stack() while cpuc->enabled is 1, local_irq_save()
masks normal interrupts but does not mask NMIs. If an NMI arrives while
amd_pmu_v2_snapshot_branch_stack() is in amd_pmu_lbr_read() or memcpy(),
perf_event_nmi_handler() -> amd_pmu_v2_handle_irq() will observe
cpuc->enabled == 1:

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 a BPF program triggered inside an NMI calls
amd_pmu_v2_snapshot_branch_stack() re-entrantly during that window, it will
also see cpuc->enabled == 1.

In both cases, wouldn't the NMI path call amd_pmu_core_enable_all() on exit
and prematurely re-enable hardware PMU counting before the outer
amd_pmu_v2_snapshot_branch_stack() has finished?

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