Re: [Patch v3 3/4] perf/x86/intel/lbr: Reset the cached LBR stack in intel_pmu_lbr_reset()

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

On 10/19/2026 3:30 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] perf/x86/intel/lbr: stale lbr_stack.nr in intel_pmu_arch_lbr_read_xsave()
> --
>
> commit 93eb502f5c2972255761d542db115fed920f52bd
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf/x86/intel/lbr: Reset the cached LBR stack in intel_pmu_lbr_reset()
>
> This commit resets cpuc->lbr_stack.nr in intel_pmu_lbr_reset() and skips
> calling intel_pmu_store_lbr() in intel_pmu_arch_lbr_read_xsave() when
> XFEATURE_MASK_LBR is not set in xsave->header.xfeatures.
>
>> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
>> index 0b446d8912e4e..249db12e927ed 100644
>> --- a/arch/x86/events/intel/lbr.c
>> +++ b/arch/x86/events/intel/lbr.c
>> @@ -193,6 +193,8 @@ void intel_pmu_lbr_reset(void)
>>  {
>>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>  
>> +	cpuc->lbr_stack.nr = 0;
>> +
>>  	if (!x86_pmu.lbr_nr)
>>  		return;
>>  
>> @@ -1012,7 +1014,8 @@ static void intel_pmu_arch_lbr_read_xsave(struct cpu_hw_events *cpuc)
>>  	}
>>  	xsaves(&xsave->xsave, XFEATURE_MASK_LBR);
>>  
>> -	intel_pmu_store_lbr(cpuc, xsave->lbr.entries);
>> +	if (xsave->header.xfeatures & XFEATURE_MASK_LBR)
>> +		intel_pmu_store_lbr(cpuc, xsave->lbr.entries);
>>  }
>>  
>>  void intel_pmu_lbr_read(void)
> [Severity: High]
> When xsaves() sees the hardware LBR state in its INIT configuration, the
> XSAVE init optimization clears XFEATURE_MASK_LBR in
> xsave->header.xfeatures without writing to xsave->lbr.entries.
>
> If intel_pmu_store_lbr() is skipped here without resetting
> cpuc->lbr_stack.nr = 0, can cpuc->lbr_stack.nr and cpuc->lbr_entries[]
> retain stale branch entries whenever the hardware LBR state enters the
> INIT configuration without going through intel_pmu_lbr_reset()?
>
> For example, during a context switch in LBR call-stack mode,
> __intel_pmu_lbr_restore() restores the incoming task's LBR state without
> calling intel_pmu_lbr_reset():
>
> arch/x86/events/intel/lbr.c:__intel_pmu_lbr_restore() {
> 	if (!has_lbr_callstack_users(ctx) ||
> 	    task_context_opt(ctx)->lbr_stack_state == LBR_NONE) {
> 		intel_pmu_lbr_reset();
> 		return;
> 	}
> 	...
> 	x86_pmu.lbr_restore(ctx);
>
> 	task_context_opt(ctx)->lbr_stack_state = LBR_NONE;
> }
>
> If the previous task populated cpuc->lbr_stack.nr > 0 and the incoming
> task had 0 branches when scheduled out (so xrstors() restores the hardware
> LBR state to the INIT state), a PMI or branch snapshot in the new task
> before new branches are recorded will see !(xsave->header.xfeatures &
> XFEATURE_MASK_LBR) and leave the previous task's cpuc->lbr_stack.nr and
> cpuc->lbr_entries[] in place.
>
> The same situation can also occur within a single task when:
> - RET instructions in call-stack mode pop all hardware LBR entries back to
>   the INIT state after an earlier PMI populated cpuc->lbr_stack.nr,
> - a deep C-state resets the hardware Arch LBR registers to the INIT state
>   (x86_pmu.lbr_deep_c_reset) without calling intel_pmu_lbr_reset(), or
> - intel_pmu_store_pebs_lbrs() populates cpuc->lbr_stack.nr from a PEBS
>   record while the hardware LBR registers are in the INIT state.
>
> In addition to leaking or repeating stale branch entries,
> intel_pmu_lbr_read() then runs intel_pmu_lbr_filter(cpuc) a second time on
> the stale cpuc->lbr_entries[], where e->type already holds a converted
> PERF_BR_* value rather than a raw ARCH_LBR_BR_TYPE_* value, indexing
> arch_lbr_br_type_map[] with the wrong value.
>
> Should intel_pmu_arch_lbr_read_xsave() clear cpuc->lbr_stack.nr = 0 when
> !(xsave->header.xfeatures & XFEATURE_MASK_LBR)?
>
> 	if (xsave->header.xfeatures & XFEATURE_MASK_LBR)
> 		intel_pmu_store_lbr(cpuc, xsave->lbr.entries);
> 	else
> 		cpuc->lbr_stack.nr = 0;

Good catch. Would follow the suggestion. Thanks.


>