Re: [Patch v6 08/20] perf/x86/intel: 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/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:

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()?

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