Re: [Patch v4 1/5] perf/x86: Disable LBR if x86_pmu.lbr_nr exceeds MAX_LBR_ENTRIES

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

On 10/21/2026 1:42 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] perf/x86: init_hw_perf_events() disables LBR too late after vendor PMU init
> --
>
> commit c6c1bbb1f4feb929dd247ddd507177ce58d871e1
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf/x86: Disable LBR if x86_pmu.lbr_nr exceeds MAX_LBR_ENTRIES
>
> This commit disables last branch record support in init_hw_perf_events()
> if x86_pmu.lbr_nr exceeds MAX_LBR_ENTRIES to prevent out-of-bounds
> accesses in the cached branch stack handling path.
>
>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
>> index 4df792b139e05..002a04c149c29 100644
>> --- a/arch/x86/events/core.c
>> +++ b/arch/x86/events/core.c
>> @@ -2645,6 +2645,12 @@ static int __init init_hw_perf_events(void)
>>  	if (!x86_pmu.config_mask)
>>  		x86_pmu.config_mask = X86_RAW_EVENT_MASK;
>>  
>> +	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;
>> +	}
>> +
>>  	perf_events_lapic_init();
>>  	register_nmi_handler(NMI_LOCAL, perf_event_nmi_handler, 0, "PMI");
> [Severity: Medium]
> 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?
>
> In arch/x86/events/intel/lbr.c:intel_pmu_arch_lbr_init(), if lbr_nr exceeds
> MAX_LBR_ENTRIES, initialization proceeds instead of taking the clear_arch_lbr
> path:
>
> 	lbr_nr = fls(eax.split.lbr_depth_mask) * 8;
> 	if (!lbr_nr)
> 		goto clear_arch_lbr;
>
> 	/* Apply the max depth of Arch LBR */
> 	if (wrmsrq_safe(MSR_ARCH_LBR_DEPTH, lbr_nr))
> 		goto clear_arch_lbr;
> 	...
> 	x86_pmu.lbr_counters = ecx.split.lbr_counters;
> 	x86_pmu.lbr_nr = lbr_nr;
>
> 	if (!!x86_pmu.lbr_counters)
> 		x86_pmu.flags |= PMU_FL_BR_CNTR | PMU_FL_DYN_CONSTRAINT;
>
> Because clear_arch_lbr is skipped, X86_FEATURE_ARCH_LBR remains enabled,
> MSR_ARCH_LBR_DEPTH is written, pmu->task_ctx_cache is allocated via
> create_lbr_kmem_cache(), and x86_pmu.lbr_ctl_map and static branches are
> configured.
>
> Subsequently, arch/x86/events/intel/lbr.c:reserve_lbr_buffers() checks the
> feature bit and task_ctx_cache rather than x86_pmu.lbr_nr:
>
> 	if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR))
> 		return;
>
> 	for_each_possible_cpu(cpu) {
> 		cpuc = per_cpu_ptr(&cpu_hw_events, cpu);
> 		kmem_cache = x86_get_pmu(cpu)->task_ctx_cache;
> 		if (!kmem_cache || cpuc->lbr_xsave)
> 			continue;
>
> 		cpuc->lbr_xsave = kmem_cache_alloc_node(kmem_cache,
> 							GFP_KERNEL | __GFP_ZERO,
> 							cpu_to_node(cpu));
> 	}
>
> Wouldn't this still allocate per-cpu cpuc->lbr_xsave buffers on hardware
> reservation even after x86_pmu.lbr_nr is set to 0?
>
> Similarly, in arch/x86/events/intel/core.c:lbr_is_visible(), leaving
> PMU_FL_BR_CNTR set in x86_pmu.flags keeps branch_counter_nr and
> branch_counter_width visible in sysfs when x86_pmu.lbr_nr is 0:
>
> 	/* branches */
> 	if (i == 0)
> 		return x86_pmu.lbr_nr ? attr->mode : 0;
>
> 	return (x86_pmu.flags & PMU_FL_BR_CNTR) ? attr->mode : 0;
>
> In addition, both arch/x86/events/intel/core.c:intel_pmu_init() and
> arch/x86/events/amd/core.c:amd_core_pmu_init() check x86_pmu.lbr_nr before
> init_hw_perf_events() clears it:
>
> 	if (x86_pmu.lbr_nr) {
> 		intel_pmu_lbr_init();
>
> 		pr_cont("%d-deep LBR, ", x86_pmu.lbr_nr);
>
> 		/* only support branch_stack snapshot for perfmon >= v2 */
> 		if (x86_pmu.disable_all == intel_pmu_disable_all) {
> 			if (boot_cpu_has(X86_FEATURE_ARCH_LBR)) {
> 				static_call_update(perf_snapshot_branch_stack,
> 						   intel_pmu_snapshot_arch_branch_stack);
> 			} else {
> 				static_call_update(perf_snapshot_branch_stack,
> 						   intel_pmu_snapshot_branch_stack);
> 			}
> 		}
> 	}
>
> And in amd_core_pmu_init():
>
> 	if (!amd_pmu_lbr_init()) {
> 		/* LBR requires flushing on context switch */
> 		x86_pmu.sched_task = amd_pmu_lbr_sched_task;
> 		static_call_update(amd_pmu_branch_hw_config, amd_pmu_lbr_hw_config);
> 		static_call_update(amd_pmu_branch_reset, amd_pmu_lbr_reset);
> 		static_call_update(amd_pmu_branch_add, amd_pmu_lbr_add);
> 		static_call_update(amd_pmu_branch_del, amd_pmu_lbr_del);
>
> 		/* Only support branch_stack snapshot on perfmon v2 */
> 		if (x86_pmu.handle_irq == amd_pmu_v2_handle_irq)
> 			static_call_update(perf_snapshot_branch_stack, amd_pmu_v2_snapshot_branch_stack);
> 	} else if (!amd_brs_init()) {
>
> If bpf_get_branch_snapshot() is later called when x86_pmu.lbr_nr is 0, won't
> the registered perf_snapshot_branch_stack callback still run, manipulate
> branch and global control registers, and unconditionally call
> intel_pmu_enable_all(0) or amd_pmu_v2_enable_all(0)?
>
> Should the MAX_LBR_ENTRIES check be performed before or during the
> vendor-specific initialization routines (such as intel_pmu_lbr_init(),
> intel_pmu_arch_lbr_init(), and amd_pmu_lbr_init()) so that None of the
> hardware state, slab caches, flags, or static calls are enabled when
> lbr_nr is out of range?

The change is good enough. The branch sampling perf event creation is
guarded with x86_pmu.lbr_nr regardless of on Intel or AMD platforms. If
x86_pmu.lbr_nr is 0, then no events with branch sampling can be created
successfully, and if no active LBR event is present, the LBR hardware is
not really enabled.

Since it's a common validation for all x86 vendor platforms, it should be
placed into the x86 common PMU initialization path.

Thanks.


>