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

[bot+bpf-ci]

> 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:

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