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

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

On 11/6/2026 1:06 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] perf/x86/intel: cpuc->enabled not cleared in intel_pmu_snapshot_branch_stack()
> --
>
> commit f8f32e2d56da9a93e561d91b6c3fb89e787c5d9e
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf/x86/intel: Avoid re-enabling PMU and LBR for BPF when disabled
>
> This commit updates __intel_pmu_snapshot_branch_stack() and its callers
> to save the PMU and LBR enabled state and only re-enable them after
> taking a branch snapshot if they were originally enabled.
>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 9d8398da1989f..739c454827ac5 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -2821,7 +2821,8 @@ static void intel_pmu_enable_all(int added)
>>  
>>  static noinline int
>>  __intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries,
>> -				  unsigned int cnt, unsigned long flags)
>> +				  unsigned int cnt, unsigned long flags,
>> +				  int pmu_enabled, int lbr_enabled)
>>  {
>>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>  
>> @@ -2829,7 +2830,22 @@ __intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries,
>>  	cnt = min_t(unsigned int, cnt, cpuc->lbr_stack.nr);
>>  
>>  	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
>> -	intel_pmu_enable_all(0);
>> +
>> +	/*
>> +	 * Do not blindly call intel_pmu_enable_all() to re-enable LBR.
>> +	 * This path may be entered from the PMI handler through
>> +	 * bpf_overflow_handler().
>> +	 *
>> +	 * If PMU version < 4, LBR is disabled by the LBR freeze mechanism,
>> +	 * LBR can't be re-enabled here.
>> +	 * If PMU version >= 4, LBR is only frozen instead of disabled,
>> +	 * LBR should be re-enabled here.
>> +	 */
>> +	if (lbr_enabled)
>> +		intel_pmu_lbr_enable_all(false);
>> +	if (pmu_enabled)
>> +		___intel_pmu_enable_all();
>> +
>>  	local_irq_restore(flags);
>>  	return cnt;
>>  }
>> @@ -2837,27 +2853,41 @@ __intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries,
>>  static int
>>  intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
>>  {
>> +	struct cpu_hw_events *cpuc;
>>  	unsigned long flags;
>> +	int pmu_enabled;
>> +	int lbr_enabled;
>>  
>>  	/* must not have branches... */
>>  	local_irq_save(flags);
>> +	cpuc = this_cpu_ptr(&cpu_hw_events);
>> +	pmu_enabled = cpuc->enabled;
>> +	lbr_enabled = cpuc->lbr_enabled;
>>  	__intel_pmu_disable_all(false); /* we don't care about BTS */
>>  	__intel_pmu_lbr_disable();
>>  	/*            ... until here */
>> -	return __intel_pmu_snapshot_branch_stack(entries, cnt, flags);
>> +	return __intel_pmu_snapshot_branch_stack(entries, cnt, flags,
>> +						 pmu_enabled, lbr_enabled);
>>  }
>>  
>>  static int
>>  intel_pmu_snapshot_arch_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
>>  {
>> +	struct cpu_hw_events *cpuc;
>>  	unsigned long flags;
>> +	int pmu_enabled;
>> +	int lbr_enabled;
>>  
>>  	/* must not have branches... */
>>  	local_irq_save(flags);
>> +	cpuc = this_cpu_ptr(&cpu_hw_events);
>> +	pmu_enabled = cpuc->enabled;
>> +	lbr_enabled = cpuc->lbr_enabled;
>>  	__intel_pmu_disable_all(false); /* we don't care about BTS */
>>  	__intel_pmu_arch_lbr_disable();
>>  	/*            ... until here */
>> -	return __intel_pmu_snapshot_branch_stack(entries, cnt, flags);
>> +	return __intel_pmu_snapshot_branch_stack(entries, cnt, flags,
>> +						 pmu_enabled, lbr_enabled);
>>  }
> [Severity: Medium]
> Should intel_pmu_snapshot_branch_stack() and
> intel_pmu_snapshot_arch_branch_stack() also clear cpuc->enabled = 0
> before calling __intel_pmu_disable_all(false), and restore
> cpuc->enabled = pmu_enabled in __intel_pmu_snapshot_branch_stack()?
>
> While __intel_pmu_lbr_disable() and __intel_pmu_arch_lbr_disable() clear
> cpuc->lbr_enabled = 0, __intel_pmu_disable_all(false) only writes 0 to
> MSR_CORE_PERF_GLOBAL_CTRL without updating cpuc->enabled.
>
> When bpf_get_branch_snapshot() is invoked outside PMI context (where
> cpuc->enabled is 1), local_irq_save() does not mask NMIs. If an in-flight
> PMI or another NMI arrives while __intel_pmu_snapshot_branch_stack() is
> reading the LBRs, perf_event_nmi_handler() -> intel_pmu_handle_irq()
> observes cpuc->enabled == 1 and re-enables the PMU (and LBR on PMU
> version < 4) before returning:

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


>
> arch/x86/events/intel/core.c:intel_pmu_handle_irq() {
> 	pmu_enabled = cpuc->enabled;
> 	...
> done:
> 	if (mid_ack)
> 		apic_write(APIC_LVTPC, APIC_DM_NMI);
> 	/* Only restore PMU state when it's active. See x86_pmu_disable(). */
> 	cpuc->enabled = pmu_enabled;
> 	if (pmu_enabled)
> 		__intel_pmu_enable_all(true);
> }
>
> Similarly, a nested snapshot from an NMI or fentry/kprobe BPF program
> during __intel_pmu_snapshot_branch_stack() would see cpuc->enabled == 1
> and prematurely call ___intel_pmu_enable_all() before the outer snapshot
> finishes.
>
> Setting cpuc->enabled = 0 is a plain store with no branches, so could it
> be cleared alongside saving pmu_enabled and restored in
> __intel_pmu_snapshot_branch_stack() to match x86_pmu_disable(),
> __intel_pmu_quiesce(), and intel_pmu_handle_irq()?
>