Re: [Patch v5 01/18] perf/x86: Disable LBR if x86_pmu.lbr_nr exceeds MAX_LBR_ENTRIES

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Oct 29, 2026 at 05:56:15PM +0800, Mi, Dapeng wrote:
> 
> On 10/29/2026 5:38 PM, Peter Zijlstra wrote:
> > On Thu, Oct 29, 2026 at 12:38:57PM +0800, Dapeng Mi wrote:
> >> Add a sanity check for x86_pmu.lbr_nr. If the hardware reports more LBR
> >> entries than MAX_LBR_ENTRIES, disable LBR to avoid out-of-bounds access
> >> in the cached LBR stack handling path.
> > Cute, can this actually happen?
> 
> Currently it won't happen on real hardware since the maximum LBR stack
> depth what hardware supports is 32. But it's better have this sanity check
> assume the LBR stack depth could be extended to 64 or larger.
> 
> 
> >
> >> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> >> ---
> >>  arch/x86/events/amd/lbr.c    | 8 +++++++-
> >>  arch/x86/events/intel/core.c | 7 +++++++
> >>  2 files changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/events/amd/lbr.c b/arch/x86/events/amd/lbr.c
> >> index a55646fcb846..96c5b5e56053 100644
> >> --- a/arch/x86/events/amd/lbr.c
> >> +++ b/arch/x86/events/amd/lbr.c
> >> @@ -433,7 +433,13 @@ __init int amd_pmu_lbr_init(void)
> >>  	ebx.full = cpuid_ebx(EXT_PERFMON_DEBUG_FEATURES);
> >>  	x86_pmu.lbr_nr = ebx.split.lbr_v2_stack_sz;
> >>  
> >> -	pr_cont("%d-deep LBR, ", x86_pmu.lbr_nr);
> >> +	if (x86_pmu.lbr_nr > MAX_LBR_ENTRIES) {
> >> +		pr_warn("LBR entries %d exceeds max %d, disable LBR.\n",
> >> +			x86_pmu.lbr_nr, MAX_LBR_ENTRIES);
> >> +		x86_pmu.lbr_nr = 0;
> >> +	} else {
> >> +		pr_cont("%d-deep LBR, ", x86_pmu.lbr_nr);
> >> +	}
> >>  
> >>  	return 0;
> >>  }
> >> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> >> index 0a34d674df59..10c694db85ea 100644
> >> --- a/arch/x86/events/intel/core.c
> >> +++ b/arch/x86/events/intel/core.c
> >> @@ -9023,6 +9023,13 @@ __init int intel_pmu_init(void)
> >>  	 */
> >>  	if (x86_pmu.lbr_tos && !check_msr(x86_pmu.lbr_tos, 0x3UL))
> >>  		x86_pmu.lbr_nr = 0;
> >> +
> >> +	if (x86_pmu.lbr_nr > MAX_LBR_ENTRIES) {
> >> +		pr_warn("LBR entries %d exceeds max %d, disable LBR.\n",
> >> +			x86_pmu.lbr_nr, MAX_LBR_ENTRIES);
> >> +		x86_pmu.lbr_nr = 0;
> >> +	}
> >> +
> >>  	for (i = 0; i < x86_pmu.lbr_nr; i++) {
> >>  		if (!(check_msr(x86_pmu.lbr_from + i, 0xffffUL) &&
> >>  		      check_msr(x86_pmu.lbr_to + i, 0xffffUL)))
> > Would it make sense to have but one copy of this check in
> > init_hw_perf_events(), after calling into the micro-arch code?
> 
> Yes, that's what the previous version does, but Sashiko complains it's too
> late.
> 
> "
> 
> Does zeroing x86_pmu.lbr_nr here in init_hw_perf_events() happen too late,
> after intel_pmu_init() and amd_pmu_init() have already configured hardware
> and driver state using the unvalidated x86_pmu.lbr_nr value?
> 
> "
> 
> https://lore.kernel.org/all/sashiko-outbox-179814@kernel.org/
> 
> It makes sense in some ways, so move the sanity check into the vendor init
> in this version. 

Urgh. But there are no events yet that early, none of that can actually
happen.

If we're really worried about this, change the micro-arch init to spawn
in disabled state and we call x86_pmu_enable_all() at the end.

Also, this is before x86_pmu_static_call_update(), nothing really
'works' yet anyway.