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

[sashiko-bot@kernel.org]

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;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019070836.2972403-1-dapeng1.mi@linux.intel.com?part=3