Re: [Patch v5 11/18] perf/x86/intel: Use a dedicated LBR stack for BPF snapshotting

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

On 10/29/2026 6:41 PM, Peter Zijlstra wrote:
> On Thu, Oct 29, 2026 at 12:39:07PM +0800, Dapeng Mi wrote:
>> BPF LBR snapshotting currently reuses the default LBR stack
>> (cpuc->lbr_stack) when storing sampled data. This is unsafe when the
>> BPF helper runs in PMI context via bpf_overflow_handler(): it can
>> overwrite the cached LBR state for active PEBS LBR events, causing
>> incorrect LBR data to be reported to userspace. When the PEBS and BPF
>> stacks have different depths, this can also lead to out-of-bounds reads
>> when the LBR stack is later exported.
>>
>> Use a dedicated LBR stack for BPF snapshotting so the BPF path no
>> longer clobbers the shared LBR state used by other perf events.
>>
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Fixes: 425507fa5f32 ("perf/x86/intel/lbr: Optimize v4 LBR unfreezing")
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> AMD has the same problem, no?

No. According what I know, currently AMD doesn't support branch sampling in
IBS and the LBR is frozen in the whole PMI handling, BPF would see same LBR
stack here with the PMI handler at the PMI entry. So although BPF path
indeed overwrites the default cpuc->lbr_stack, it won't break anything
since the snapshotted LBR stacks are same for PMI and BPF.

But this indeed causes the deviation on the coding level. A generic way is
to add BPF dedicated LBR stack in cpuc structure, then use the dedicated
BPF stack to store BPF snapshot for both AMD and Intel. But it could waste
some memory on AMD platforms.

diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index d0ed4a2e1683..6e5144fbde9f 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -530,6 +530,9 @@ struct cpu_hw_events {
        struct perf_branch_stack        lbr_stack;
        struct perf_branch_entry        lbr_entries[MAX_LBR_ENTRIES];
        u64                             lbr_counters[MAX_LBR_ENTRIES]; /*
branch stack extra */
+       struct perf_branch_stack        bpf_lbr_stack;
+       struct perf_branch_entry        bpf_lbr_entries[MAX_LBR_ENTRIES];
+       u64                             bpf_lbr_counters[MAX_LBR_ENTRIES];
/* branch stack extra */
        union {
                struct er_account               *lbr_sel;
                struct er_account               *lbr_ctl;

What do you prefer?

Thanks.


>
>> ---
>>  arch/x86/events/intel/core.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 5eda551cc9f2..482a148916bf 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -2819,18 +2819,24 @@ static void intel_pmu_enable_all(int added)
>>  	__intel_pmu_enable_all(false);
>>  }
>>  
>> +struct lbr_snapshot {
>> +	struct perf_branch_stack stack;
>> +	struct perf_branch_entry entries[MAX_LBR_ENTRIES];
>> +	u64 counters[MAX_LBR_ENTRIES];
>> +};
>> +static DEFINE_PER_CPU(struct lbr_snapshot, lbr_snapshot);
>> +
>>  static noinline int
>>  __intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries,
>>  				  unsigned int cnt, unsigned long flags,
>>  				  int pmu_enabled, int lbr_enabled)
>>  {
>> -	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> +	struct lbr_snapshot *bpf = this_cpu_ptr(&lbr_snapshot);
>>  
>> -	intel_pmu_lbr_read(&cpuc->lbr_stack,
>> -			   cpuc->lbr_entries, cpuc->lbr_counters);
>> -	cnt = min_t(unsigned int, cnt, cpuc->lbr_stack.nr);
>> +	intel_pmu_lbr_read(&bpf->stack, bpf->entries, bpf->counters);
>> +	cnt = min_t(unsigned int, cnt, bpf->stack.nr);
>>  
>> -	memcpy(entries, cpuc->lbr_entries,
>> +	memcpy(entries, bpf->entries,
>>  	       sizeof(struct perf_branch_entry) * cnt);
>>  
>>  	/*
>> -- 
>> 2.34.1
>>