Re: [PATCH v2 2/2] perf/x86/intel/lbr: Limit branch snapshots to valid entries

[Sandipan Das <sandipan.das@amd.com>]

On 16-10-2026 08:41, Mi, Dapeng wrote:
> 
> On 10/16/2026 8:21 AM, bot+bpf-ci@kernel.org wrote:
>>> perf/x86/intel/lbr: Limit branch snapshots to valid entries
>> TL;DR: Resetting cpuc->lbr_stack.nr in intel_pmu_lbr_read() may clobber an
>> in-flight adaptive PEBS sample if a BPF program calls
>> bpf_get_branch_snapshot(), possibly yielding a PERF_RECORD_SAMPLE without
>> branch records and with leftover bytes.
>>
>>> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
>>> index 22e2a06d5786..2fe858d99979 100644
>>> --- a/arch/x86/events/intel/lbr.c
>>> +++ b/arch/x86/events/intel/lbr.c
>> [ ... ]
>>
>>> @@ -1026,8 +1026,10 @@ void intel_pmu_lbr_read(void)
>>>  	 * but this simple approach seems to work for now.
>>>  	 */
>>>  	if (!cpuc->lbr_users || vlbr_exclude_host() ||
>>> -	    cpuc->lbr_users == cpuc->lbr_pebs_users)
>>> +	    cpuc->lbr_users == cpuc->lbr_pebs_users) {
>>> +		cpuc->lbr_stack.nr = 0;
>>                  ^^^^
>> Can this clobber the branch stack of a PEBS sample that is still being
>> built?
>>
>> Before this patch the early return had no side effects.  Now it writes to
>> cpuc->lbr_stack, which is shared with the adaptive PEBS path.
>>
>> If every LBR user on the CPU is an adaptive PEBS event, then
>> cpuc->lbr_users == cpuc->lbr_pebs_users, and the PEBS drain points the
>> sample at the per-CPU stack:
>>
>> setup_pebs_adaptive_sample_data()
>>     intel_pmu_store_pebs_lbrs(lbr);         /* cpuc->lbr_stack.nr = N */
>>     intel_pmu_lbr_save_brstack(data, cpuc, event);
>>         perf_sample_save_brstack(data, event, &cpuc->lbr_stack, ...);
>>             data->br_stack = brs;           /* points at cpuc->lbr_stack */
>>             data->dyn_size += ... + brs->nr * sizeof(struct perf_branch_entry);
>>
>> __intel_pmu_pebs_last_event() then calls perf_event_overflow(), and
>> __perf_event_overflow() runs bpf_overflow_handler() before the sample is
>> written.  A perf event BPF program attached to that event can call
>> bpf_get_branch_snapshot(), which is in bpf_base_func_proto:
> 
> Yeah, it's a possible risk. After thinking twice about this issue, the
> issue seems much more complex than I ever thought.
> 
> intel_pmu_lbr_read() could be called in two paths, one is from the perf PMI
> handler, the other is from the bpf_get_branch_snapshot() bpf helper. Which
> makes the thing more complicated is that bpf_get_branch_snapshot() helper
> could be called in the PMI handler as well, where is just before the
> default event overflow handler. In this case, intel_pmu_lbr_read() could be
> called twice in PMI handler.
> 
> So there could be 3 scenarios for the intel_pmu_lbr_read() calling.
> 
> 1. bpf_get_branch_snapshot() is not attached to any event, then
> bpf_get_branch_snapshot() won't be called in anywhere, current LBR stack
> update in PMI handler is good for both PEBS and non-PEBS events.
> 
> 2. bpf_get_branch_snapshot() is attached to a software tracepoint, then
> bpf_get_branch_snapshot() is called out of PMI handler,
> the intel_pmu_lbr_read() is called to get the current LBR stack data in
> bpf_get_branch_snapshot(), it's fine as well.
> 
> 3. bpf_get_branch_snapshot() is attached to a perf hardware event, then
> bpf_get_branch_snapshot() is called in the PMI handler, it leads to LBR
> stack could be updated twice in the PMI handler. In this scenario, since
> perf has already read the latest LBR stack data before calling
> bpf_get_branch_snapshot() helper, bpf_get_branch_snapshot() is unnecessary
> to read the LBR stack again, and worse it could overwrite the cache PEBS
> LBR stack data if PMI is triggered by PEBS event.
> 
> Roughly we could need below changes to thoroughly fix this issue.
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 442be0639414..d2cbb5ade003 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2869,8 +2869,8 @@ __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);
> +       intel_pmu_lbr_read(true);
> +       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);
> @@ -4090,7 +4090,7 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
> 
>         loops = 0;
>  again:
> -       intel_pmu_lbr_read();
> +       intel_pmu_lbr_read(false);
>         intel_pmu_ack_status(status);
>         if (++loops > 100) {
>                 static bool warned;
> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
> index 2218469be644..c651b94a893f 100644
> --- a/arch/x86/events/intel/lbr.c
> +++ b/arch/x86/events/intel/lbr.c
> @@ -1008,18 +1008,31 @@ static void intel_pmu_arch_lbr_read_xsave(struct
> cpu_hw_events *cpuc)
>         intel_pmu_store_lbr(cpuc, xsave->state.lbr.entries);
>  }
> 
> -void intel_pmu_lbr_read(void)
> +void intel_pmu_lbr_read(bool from_bpf)
>  {
>         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;
> +
> +       /*
> +        * Don't read LBRs from BPF context if we're in an PMI handler
> +        * since the LBR stack has been updated previously and new LBR
> +        * stack update could overwrite the cached PEBS LBR stack.
> +        */
> +       if (from_bpf && in_nmi())
>                 return;
> 
>         x86_pmu.lbr_read(cpuc);
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index 60038d329518..4738b40288a4 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -2006,7 +2006,7 @@ void intel_pmu_lbr_enable_all(bool pmi);
> 
>  void intel_pmu_lbr_disable_all(void);
> 
> -void intel_pmu_lbr_read(void);
> +void intel_pmu_lbr_read(bool from_bpf);
> 
>  void intel_pmu_lbr_read_32(struct cpu_hw_events *cpuc);
> 
> Sandipan, would you mind I take this series? I need to do fully validation
> for the Intel side changes. Once the validation passes, I would send a new
> patch series to fix the issue for both AMD and Intel side. Thanks.
> 

Sure, please go ahead.

> 
>>
>> intel_pmu_snapshot_branch_stack()
>>     __intel_pmu_snapshot_branch_stack()
>>         intel_pmu_lbr_read()
>>             /* lbr_users == lbr_pebs_users, early return taken */
>>             cpuc->lbr_stack.nr = 0;
>>
>> When the program returns, data->br_stack still points at cpuc->lbr_stack,
>> but nr is now 0.  A later bpf_read_branch_records() in the same program
>> reads ctx->data->br_stack->nr and returns 0 bytes.
>>
>> perf_output_sample() then writes nr = 0 and no entries, while header.size was
>> already computed by perf_prepare_header() from data->dyn_size, which counted
>> N entries.  Doesn't that leave the PERF_RECORD_SAMPLE without its branch
>> records and followed by N * sizeof(struct perf_branch_entry) bytes of
>> leftover ring buffer content?
>>
>> Before the patch, the early return left cpuc->lbr_stack untouched and this
>> sample came out intact.
>>
>> Would it be better to leave cpuc->lbr_stack alone in the snapshot path, for
>> example by checking the skip condition in __intel_pmu_snapshot_branch_stack()
>> and returning 0 there, instead of resetting the shared nr inside
>> intel_pmu_lbr_read() for every caller?
>>
>>>  		return;
>>> +	}
>>>
>>>  	x86_pmu.lbr_read(cpuc);
>>
>> ---
>> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
>> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>>
>> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/38827450009