Re: [PATCH v3 3/4] perf/arm64: Add BRBE support for bpf_get_branch_snapshot()

On Mon, Apr 13, 2026 at 9:31 PM <[email protected]> wrote:
>
> Sashiko AI review found 4 potential issue(s):
> - [High] arm_brbe_snapshot_branch_stack executes BRBE system register reads without validating CPU support, leading to an Undefined Instruction kernel panic on asymmetric systems.
> - [High] arm_brbe_snapshot_branch_stack destructively clears the hardware branch buffer, corrupting data for concurrent perf sessions.
> - [High] A TOCTOU race can unintentionally re-enable BRBE after it was explicitly disabled by a PMU interrupt, corrupting hardware state.
> - [Medium] Missing isb() context synchronization after unpausing BRBE causes missed branch records.
> --
>
> commit ce06705327b522bc73c923ff4219c198fe71f3b4
> Author: Puranjay Mohan <[email protected]>
> Subject: perf/arm64: Add BRBE support for bpf_get_branch_snapshot()
>
> This commit enables bpf_get_branch_snapshot() on ARM64 by implementing the
> perf_snapshot_branch_stack static call for BRBE. It pauses BRBE, masks
> exceptions, reads branch records, and then invalidates the buffer.
>
> > diff --git a/drivers/perf/arm_brbe.c b/drivers/perf/arm_brbe.c
> > index ba554e0c846c4..fd62019ddc839 100644
> > --- a/drivers/perf/arm_brbe.c
> > +++ b/drivers/perf/arm_brbe.c
>
> [ ... ]
>
> > +/*
> > + * Best-effort BRBE snapshot for BPF tracing. Pause BRBE to avoid
> > + * self-recording and return 0 if the snapshot state appears disturbed.
> > + */
> > +int arm_brbe_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
> > +{
> > +     unsigned long flags;
> > +     int nr_hw, nr_copied = 0;
> > +     u64 brbfcr, brbcr;
> > +
> > +     if (!cnt)
> > +             return 0;
> > +
> > +     /*
> > +      * Pause BRBE first to avoid recording our own branches. The
> > +      * sysreg read/write and ISB are branchless, so pausing before
> > +      * checking BRBCR avoids polluting the buffer with our own
> > +      * conditional branches.
> > +      */
> > +     brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
>
> On asymmetric systems (like big.LITTLE) where only a subset of CPU cores
> implement BRBE, the perf_snapshot_branch_stack static call is enabled
> globally. If a BPF program invokes this snapshot on a CPU lacking BRBE,
> will this unconditionally execute read_sysreg_s(SYS_BRBFCR_EL1) and
> trigger an Undefined Instruction exception kernel panic?
>
> Does this need a local CPU feature check before accessing these registers?

I had this in the previous version, but Rob explained that this
doesn't happen in practice so I removed it.

>
> > +     brbcr = read_sysreg_s(SYS_BRBCR_EL1);
> > +     write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
> > +     isb();
> > +
> > +     /* Bail out if BRBE is not enabled (BRBCR_EL1 == 0). */
> > +     if (!brbcr) {
> > +             write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
> > +             return 0;
> > +     }
> > +
> > +     /* Block local exception delivery while reading the buffer. */
> > +     flags = local_daif_save();
> > +
> > +     /*
> > +      * A PMU overflow before local_daif_save() could have re-enabled
> > +      * BRBE, clearing the PAUSED bit. The overflow handler already
> > +      * restored BRBE to its correct state, so just bail out.
> > +      */
> > +     if (!(read_sysreg_s(SYS_BRBFCR_EL1) & BRBFCR_EL1_PAUSED)) {
>
> Can a TOCTOU race unintentionally re-enable BRBE if a PMU interrupt fires
> before exceptions are masked?
>
> If the PMU overflow interrupt decides to stop the event, it calls
> brbe_disable(), which sets SYS_BRBCR_EL1 to 0 and sets the PAUSED bit in
> SYS_BRBFCR_EL1.
>
> Upon returning from the interrupt, the PAUSED bit would still be set, so
> this check evaluates to false and proceeds. At the end of the function,
> the cached brbcr (which was > 0) and brbfcr are written back, which would
> unintentionally re-enable BRBE in hardware without an active perf event.
>
> > +             local_daif_restore(flags);
> > +             return 0;
> > +     }
> > +
> > +     nr_hw = FIELD_GET(BRBIDR0_EL1_NUMREC_MASK,
> > +                       read_sysreg_s(SYS_BRBIDR0_EL1));
> > +
> > +     for_each_brbe_entry(i, nr_hw) {
> > +             if (nr_copied >= cnt)
> > +                     break;
> > +
> > +             if (!perf_entry_from_brbe_regset(i, &entries[nr_copied], NULL))
> > +                     break;
> > +
> > +             nr_copied++;
> > +     }
> > +
> > +     brbe_invalidate();
>
> Does calling brbe_invalidate() here destructively clear the hardware branch
> buffer?
>
> Since BPF branch snapshots are intended to be non-destructive observations,
> wiping out the hardware branch buffer could permanently destroy the branch
> history for a concurrently active perf session.
>
> > +
> > +     /* Restore BRBCR before unpausing via BRBFCR, matching brbe_enable(). */
> > +     write_sysreg_s(brbcr, SYS_BRBCR_EL1);
> > +     isb();
> > +     write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
>
> Is an explicit isb() missing after writing to SYS_BRBFCR_EL1?
>
> Since MSR instructions to system registers are not context synchronizing,
> the processor may delay unpausing BRBE. Could this result in missed
> branch records until the next context synchronization event naturally occurs?
>
> > +     local_daif_restore(flags);
> > +
> > +     return nr_copied;
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3