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

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 11/6/2026 1:01 PM, sashiko-bot@kernel.org wrote:
> 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:

This issue should never happen in reality since amd_pmu_core_disable_all()
would disable PMU, so no PMI can be interrupt the BPF LBR snapshot.



>
> 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
>