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

[Mi, Dapeng]

On 10/29/2026 6:19 PM, Peter Zijlstra wrote:
> 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@xxxxxxxxxxxxxxx>
>>>> ---
>>>>  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@xxxxxxxxxx/
>>
>> 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.

Yeah, I agree. I don't think it would cause any real functionality issue by
deferring the LBR disabling into init_hw_perf_events(). The only flaw is
that the vendor pmu init function has already printed the LBR stack depth
which indicates LBR works, and then init_hw_perf_events() prints LBR is
disabled. It may cause some confusion for uses. But anyway, I don't think
it's a big deal.