Re: [Patch v3 4/4] perf/x86/intel/lbr: Limit branch snapshots to valid entries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] perf/x86/intel/lbr: stale LBR data or -ENOENT in intel_pmu_lbr_read() for PEBS
- [Medium] perf/x86/intel/lbr: unreset lbr_stack.nr in intel_pmu_arch_lbr_read_xsave()
- [Medium] perf/x86: late lbr_nr clamping in init_hw_perf_events() after arch LBR init
--

commit da27a6ac96e6d944e33e171c276d0b044be96def
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86/intel/lbr: Limit branch snapshots to valid entries

This patch limits __intel_pmu_snapshot_branch_stack() to copy up to
cpuc->lbr_stack.nr entries rather than x86_pmu.lbr_nr, and resets
cpuc->lbr_stack.nr to 0 in intel_pmu_lbr_read() when there are no active
LBR users or LBR is excluded on the host.

> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 0a34d674df590..5dfb9e9156e88 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2822,7 +2822,7 @@ __intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries,
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  
>  	intel_pmu_lbr_read();
> -	cnt = min_t(unsigned int, cnt, x86_pmu.lbr_nr);
> +	cnt = min_t(unsigned int, cnt, cpuc->lbr_stack.nr);
>  
>  	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
>  	intel_pmu_enable_all(0);
> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
> index 249db12e927ed..cc0300aba7de0 100644
> --- a/arch/x86/events/intel/lbr.c
> +++ b/arch/x86/events/intel/lbr.c
> @@ -1022,14 +1022,19 @@ void intel_pmu_lbr_read(void)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  
> +	if (!cpuc->lbr_users || vlbr_exclude_host()) {
> +		/* Reset lbr_stack.nr to prevent stale data leakage. */
> +		cpuc->lbr_stack.nr = 0;
> +		return;
> +	}
> +
>  	/*
>  	 * Don't read when all LBRs users are using adaptive PEBS.
>  	 *
>  	 * This could be smarter and actually check the event,
>  	 * but this simple approach seems to work for now.
>  	 */
> -	if (!cpuc->lbr_users || vlbr_exclude_host() ||
> -	    cpuc->lbr_users == cpuc->lbr_pebs_users)
> +	if (cpuc->lbr_users == cpuc->lbr_pebs_users)
>  		return;

[Severity: High]
Does skipping x86_pmu.lbr_read(cpuc) here when cpuc->lbr_users ==
cpuc->lbr_pebs_users cause __intel_pmu_snapshot_branch_stack() to return
0 entries or copy stale cpuc->lbr_entries[] to BPF programs?

The commit message notes that when all active LBR users are PEBS events,
the LBR stack is already updated from the PEBS records. However, PEBS LBR
records are only written into cpuc->lbr_entries[] and cpuc->lbr_stack.nr
during counter overflow PMI draining:

  handle_pmi_common()
    -> x86_pmu_drain_pebs()
      -> setup_pebs_adaptive_sample_data() / setup_arch_pebs_sample_data()
        -> intel_pmu_store_pebs_lbrs()

In contrast, bpf_get_branch_snapshot() invokes
intel_pmu_snapshot_branch_stack() or intel_pmu_snapshot_arch_branch_stack()
synchronously from a software or tracing event outside of any PEBS drain.
Those functions freeze the hardware LBRs and call
__intel_pmu_snapshot_branch_stack() -> intel_pmu_lbr_read() to read the
current hardware LBR registers.

When all active LBR events on the CPU are adaptive PEBS events, returning
early in intel_pmu_lbr_read() without reading the hardware LBRs or
updating cpuc->lbr_stack.nr leads to several issues:

1. If no PEBS PMI has occurred since the last intel_pmu_lbr_reset(),
   cpuc->lbr_stack.nr is still 0, so __intel_pmu_snapshot_branch_stack()
   copies 0 entries and bpf_get_branch_snapshot() fails with -ENOENT even
   though the LBR hardware has been actively recording branches.

2. If an earlier PEBS PMI occurred, cpuc->lbr_entries[] and
   cpuc->lbr_stack.nr hold the historical branch stack from that old PEBS
   overflow sample rather than the live branches frozen at the snapshot
   point.

3. Stale or cross-task entries can also remain in cpuc->lbr_entries[] with
   cpuc->lbr_stack.nr > 0 across transitions that do not call
   intel_pmu_lbr_reset():

   arch/x86/events/intel/lbr.c:intel_pmu_lbr_add() {
       if (x86_pmu.intel_cap.pebs_baseline && event->attr.precise_ip > 0)
           cpuc->lbr_pebs_users++;
       perf_sched_cb_inc(event->pmu);
       if (!cpuc->lbr_users++ && !event->total_time_running)
           intel_pmu_lbr_reset();
   }

   Scheduling in a previously active PEBS LBR event where
   event->total_time_running > 0 (or removing a non-PEBS LBR event via
   intel_pmu_lbr_del() while a PEBS LBR event remains active) does not
   reset cpuc->lbr_stack.nr. Similarly, on a context switch with
   PERF_SAMPLE_BRANCH_CALL_STACK:

   arch/x86/events/intel/lbr.c:__intel_pmu_lbr_restore() {
       ...
       x86_pmu.lbr_restore(ctx);

       task_context_opt(ctx)->lbr_stack_state = LBR_NONE;
   }

   __intel_pmu_lbr_restore() updates the hardware LBR MSRs for the
   incoming task without resetting cpuc->lbr_stack.nr, leaving the
   previous task's cached cpuc->lbr_entries[] to be copied by
   __intel_pmu_snapshot_branch_stack().

>  
>  	x86_pmu.lbr_read(cpuc);

[Severity: Medium]
Can x86_pmu.lbr_read(cpuc) leave a stale non-zero cpuc->lbr_stack.nr when
intel_pmu_arch_lbr_read_xsave() sees the hardware Arch LBR state in its
XSAVE INIT configuration?

In the preceding patch ("perf/x86/intel/lbr: Reset the cached LBR stack in
intel_pmu_lbr_reset()"), intel_pmu_arch_lbr_read_xsave() was updated to:

arch/x86/events/intel/lbr.c:intel_pmu_arch_lbr_read_xsave() {
	...
	xsaves(&xsave->xsave, XFEATURE_MASK_LBR);

	if (xsave->header.xfeatures & XFEATURE_MASK_LBR)
		intel_pmu_store_lbr(cpuc, xsave->lbr.entries);
}

When 0 branches have been recorded in hardware, the Arch LBR state is in
its XSAVE INIT configuration, so xsaves() clears XFEATURE_MASK_LBR in
xsave->header.xfeatures. Unlike the MSR fallback path
intel_pmu_store_lbr(cpuc, NULL) which reads 0 from MSR_ARCH_LBR_FROM_0 and
sets cpuc->lbr_stack.nr = 0, skipping intel_pmu_store_lbr() here without
an else branch leaves cpuc->lbr_stack.nr at its previous value.

If cpuc->lbr_stack.nr was non-zero from an earlier read and the hardware
Arch LBR state subsequently returns to the INIT state without
intel_pmu_lbr_reset() being called (for example, when
__intel_pmu_lbr_restore() restores a task's call-stack LBR context via
intel_pmu_arch_lbr_xrstors() whose saved xfeatures has XFEATURE_MASK_LBR
clear, when RET instructions pop all prior CALL entries in call-stack
mode, when a CPU wakes from a deep C-state that resets Arch LBR registers,
or when an LBR event with event->total_time_running > 0 is scheduled back
in), __intel_pmu_snapshot_branch_stack() will see the old
cpuc->lbr_stack.nr and copy stale cpuc->lbr_entries[] into entries.

Should intel_pmu_arch_lbr_read_xsave() set cpuc->lbr_stack.nr = 0 when
!(xsave->header.xfeatures & XFEATURE_MASK_LBR)?


[Severity: Medium]
Also in the earlier patch in this series ("perf/x86: Clamp x86_pmu.lbr_nr
to MAX_LBR_ENTRIES"), init_hw_perf_events() clamps x86_pmu.lbr_nr after
vendor PMU initialization has already run:

arch/x86/events/core.c:init_hw_perf_events() {
	...
	err = x86_pmu.init();
	...
	x86_pmu.lbr_nr = min(x86_pmu.lbr_nr, MAX_LBR_ENTRIES);
}

During x86_pmu.init() -> intel_pmu_init() -> intel_pmu_arch_lbr_init(),
the Arch LBR depth is selected from the highest bit in
eax.split.lbr_depth_mask (up to 64), tested against MSR_ARCH_LBR_DEPTH,
and validated against xfeature_size(XFEATURE_LBR):

arch/x86/events/intel/lbr.c:intel_pmu_arch_lbr_init() {
	...
	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_nr = lbr_nr;

	if (has_arch_lbr_xsave() &&
	    xfeature_size(XFEATURE_LBR) != get_lbr_state_size()) {
		WARN_ON(1);
		goto clear_arch_lbr;
	}
}

If hardware enumerates a maximum depth greater than MAX_LBR_ENTRIES (32)
in eax.split.lbr_depth_mask, clamping x86_pmu.lbr_nr to 32 afterwards in
init_hw_perf_events() means intel_pmu_arch_lbr_reset() will later execute
an unchecked wrmsrq(MSR_ARCH_LBR_DEPTH, x86_pmu.lbr_nr) with depth 32:

arch/x86/events/intel/lbr.c:intel_pmu_arch_lbr_reset() {
	/* Write to ARCH_LBR_DEPTH MSR, all LBR entries are reset to 0 */
	wrmsrq(MSR_ARCH_LBR_DEPTH, x86_pmu.lbr_nr);
}

If depth 32 (bit 3 in eax.split.lbr_depth_mask) is not supported by the
processor or VM, won't that wrmsrq(MSR_ARCH_LBR_DEPTH, 32) trigger an
unhandled #GP fault during CPU startup or LBR reset, in addition to
get_lbr_state_size() and the boot logs having used the unclamped
x86_pmu.lbr_nr?

Should intel_pmu_arch_lbr_init() mask or bound the selected depth to
MAX_LBR_ENTRIES before validating MSR_ARCH_LBR_DEPTH and
get_lbr_state_size()?

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