Re: [PATCH] perf/x86/amd/core: Avoid enabling BRS from the SVM reload path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] An NMI race condition during PMU reprogramming can lead to an NMI storm (Denial of Service) by erroneously re-enabling a throttled PMU event.
--

--- Patch [1]: [PATCH] perf/x86/amd/core: Avoid enabling BRS from the SVM reload path ---
commit d7d24046acc25b0323b31722e9413ffccd9d4f43
Author: Sandipan Das <sandipan.das@amd.com>

perf/x86/amd/core: Avoid enabling BRS from the SVM reload path

This commit fixes a general protection fault (#GP) during an SVM toggle on
processors that support LBR but not BRS. It separates the reprogramming of
event selectors from the BRS enablement to avoid writing to the deprecated
DebugExtnCfg MSR.

> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
[ ... ]
> @@ -752,13 +752,11 @@ static void amd_pmu_enable_event(struct perf_event *event)
>  	x86_pmu_enable_event(event);
>  }
>  
> -static void amd_pmu_enable_all(int added)
> +static void __amd_pmu_enable_all(void)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  	int idx;
>  
> -	amd_brs_enable_all();
> -
>  	for_each_set_bit(idx, x86_pmu.cntr_mask, X86_PMC_IDX_MAX) {
>  		/* only activate events which are marked as active */
>  		if (!test_bit(idx, cpuc->active_mask))
> @@ -773,6 +771,12 @@ static void amd_pmu_enable_all(int added)
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about a
pre-existing issue in the __amd_pmu_enable_all() event iteration loop in
arch/x86/events/amd/core.c.

The loop checks test_bit(idx, cpuc->active_mask), and then subsequently calls
amd_pmu_enable_event(cpuc->events[idx]).

Could an NMI strike between evaluating the active_mask and actually enabling
the event?

CPU0 in __amd_pmu_enable_all():
   if (!test_bit(idx, cpuc->active_mask)) 
       // evaluates to false, event is active, execution is interrupted

NMI fires:
x86_pmu_handle_irq()
   // event overflows and is throttled
   x86_pmu_stop()
       clear_bit(idx, cpuc->active_mask)
       hwc->state |= PERF_HES_STOPPED
   // cpuc->events[idx] is NOT set to NULL here

CPU0 resumes:
   if (cpuc->events[idx])
       amd_pmu_enable_event(cpuc->events[idx]);

Would this result in the hardware re-arming a throttled event? On the next
overflow, an NMI would be generated. However, because the event is no longer
in active_mask, would the NMI handler ignore it, failing to clear the
overflow status?

Could this uncleared overflow trigger an unrecoverable NMI storm and lock up
the CPU, especially since an unprivileged user might be able to reach this
path by profiling a KVM guest during an SVM reload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/702fa204d574b03d14e3664c7d4b201db048bbfd.1783506528.git.sandipan.das@amd.com?part=1