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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] TOCTOU race in `arm_brbe_snapshot_branch_stack` leads to hardware state corruption.
- [High] Unconditional calls to `brbe_invalidate()` destructively clear hardware branch records, breaking concurrent perf events.
- [High] `bpf_get_branch_snapshot()` unconditionally fails when invoked from a PMU NMI handler.
--

commit 4683208c7439e46b2ca26814f22b69f16e4ad0c2
Author: Puranjay Mohan <puranjay@kernel.org>

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, reads the
records into a buffer, invalidates the buffer, and unpauses BRBE.

> diff --git a/drivers/perf/arm_brbe.c b/drivers/perf/arm_brbe.c
> index ba554e0c846c4..aede95e277842 100644
> --- a/drivers/perf/arm_brbe.c
> +++ b/drivers/perf/arm_brbe.c
[ ... ]
> @@ -774,32 +795,96 @@ void brbe_read_filtered_entries(struct perf_branch_stack *branch_stack,
[ ... ]
> +/*
> + * 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;
> +
> +	/* Guard against running on a CPU without BRBE (e.g. big.LITTLE). */
> +	if (!cpu_has_brbe())
> +		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);
> +	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;
> +	}

[Severity: High]
Is this check incompatible with PMU NMI handlers?

If a BPF program attaches to a perf event overflow, armv8pmu_handle_irq()
calls armv8pmu_stop() before processing overflows, which calls brbe_disable()
and sets SYS_BRBCR_EL1 to 0.

Would this cause bpf_get_branch_snapshot() to unconditionally fail and return
0 when triggered from these PMU events?

> +
> +	/* Block local exception delivery while reading the buffer. */
> +	flags = local_daif_save();

[Severity: High]
Is there a TOCTOU race window here that could corrupt hardware state?

The function reads and caches SYS_BRBFCR_EL1 and SYS_BRBCR_EL1 before
disabling local exceptions via local_daif_save().

If an interrupt or NMI arrives in this window (such as a perf_event_disable
IPI) and modifies the BRBE hardware configuration, the function later writes
back the stale cached brbcr and brbfcr values at the end of the function,
overwriting the PMU subsystem's intentional changes.

Does the mitigation check below catch this?

	if (!(read_sysreg_s(SYS_BRBFCR_EL1) & BRBFCR_EL1_PAUSED)) {

It looks like it might fail to catch this because brbe_disable() leaves the
PAUSED bit set, leading to the function silently re-enabling BRBE and
corrupting the hardware state when restoring the cached registers.

> +
> +	/*
> +	 * 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)) {
> +		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();

[Severity: High]
Does unconditionally clearing hardware branch records here break concurrent
perf events?

Since bpf_get_branch_snapshot() can be invoked asynchronously at any time
(e.g., from a BPF kprobe or tracepoint program), if another perf event is
actively profiling, would this snapshot silently wipe the hardware BRBE
buffer?

When the active perf event later overflows, it would read an empty or severely
truncated branch stack, potentially corrupting the profiling data. Should
snapshots be read-only?

> +
> +	/* Restore BRBCR before unpausing via BRBFCR, matching brbe_enable(). */
> +	write_sysreg_s(brbcr, SYS_BRBCR_EL1);
> +	isb();
> +	write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
> +	/* Ensure BRBE is unpaused before returning to the caller. */
> +	isb();
> +	local_daif_restore(flags);
> +
> +	return nr_copied;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527121207.2312181-1-puranjay@kernel.org?part=3