Re: [PATCH v5 1/2] perf: Add Raspberry Pi BCM2835 AXI PMU driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-After-Free during CPU context migration because cpuhp instance is removed after PMU unregistration.
- [Medium] Hardware VPU bus watchers are never disabled due to dead code in the deferred teardown logic.
- [Low] Compilation error due to undeclared 'monitor_running' field in 'struct rpi_axi_hw_events'.
--

--- Patch [1]: [PATCH v5 1/2] perf: Add Raspberry Pi BCM2835 AXI PMU driver ---
commit 5c4597457ab7a1a4ae244f1e321c00bd28692a8a
Author: Ian Rogers <[email protected]>
Date:   Wed Aug 12 23:01:44 2026 +0000

    perf: Add Raspberry Pi BCM2835 AXI PMU driver

    This commit adds an uncore performance monitoring unit (PMU) driver
    for Broadcom BCM2835-BCM2711 AXI system monitors and VideoCore VPU firmware
    mailbox monitors found on Raspberry Pi 1 through 4.

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> new file mode 100644
> index 0000000000000..e5cc86fc78b25
> --- /dev/null
> +++ b/drivers/perf/rpi_axi_pmu.c
[ ... ]
> +struct rpi_axi_hw_events {
> +	int monitored_bus[NUM_BUS_WATCHERS_PER_MONITOR];
> +	int filter[NUM_BUS_WATCHERS_PER_MONITOR];
> +	int refcount[NUM_BUS_WATCHERS_PER_MONITOR];
> +	int num_monitored;
> +	int num_started;
> +	bool enabled[NUM_BUS_WATCHERS_PER_MONITOR];
> +};

[Severity: Low]
Does this code fail to compile? It looks like the monitor_running field is
accessed in rpi_axi_hw_events__init() and rpi_axi_pmu_enable_bus_watcher(),
but it is missing from the definition of struct rpi_axi_hw_events here.

> +
> +static void rpi_axi_hw_events__init(struct rpi_axi_hw_events *hw_events)
> +{
> +	hw_events->num_monitored = 0;
> +	hw_events->num_started = 0;
> +	hw_events->monitor_running = false;

[ ... ]
> +static void rpi_axi_pmu_del(struct perf_event *event, int flags)
> +{
> +	struct rpi_axi_pmu *pmu = pmu_to_rpi_axi_pmu(event->pmu);
> +	enum monitor mon = config_to_monitor(event->attr.config);
> +	unsigned long spinflags;
> +	int idx = event->hw.idx;
> +
> +	if (idx < 0)
> +		return;
> +
> +	rpi_axi_pmu_stop(event, PERF_EF_UPDATE);
> +
> +	spin_lock_irqsave(&pmu->lock, spinflags);
> +	for (int i = 0; i < RPI_AXI_MAX_EVENTS; i++) {
> +		if (pmu->events[i] == event) {
> +			pmu->events[i] = NULL;
> +			pmu->event_gen[i]++;
> +			break;
> +		}
> +	}
> +
> +	if (pmu->monitor[mon].hw_events.monitored_bus[idx] >= 0) {
> +		pmu->monitor[mon].hw_events.refcount[idx]--;
> +		if (pmu->monitor[mon].hw_events.refcount[idx] == 0) {
> +			if (mon == MON__SYSTEM)
> +				rpi_axi_pmu_disable_bus_watcher(pmu, mon, idx);
> +			pmu->monitor[mon].hw_events.monitored_bus[idx] = -1;
> +			pmu->monitor[mon].hw_events.filter[idx] = BCM2835_FLT__NONE;
> +			pmu->monitor[mon].hw_events.enabled[idx] = false;

[Severity: Medium]
Will this leave hardware VPU bus watchers running indefinitely?

When a VPU perf event is deleted, rpi_axi_pmu_del() synchronously sets
enabled[idx] = false. The background rpi_axi_pmu_vpu_work_handler() relies on
checking both refcount[idx] == 0 and enabled[idx] to perform the actual 
hardware teardown over the mailbox.

Because enabled[idx] is cleared here first, that condition becomes impossible
to satisfy in the work handler. Could this lead to the VPU monitor being left 
running when no events are active, wasting power and bus bandwidth?

> +			pmu->monitor[mon].hw_events.num_monitored--;
> +			if (mon == MON__SYSTEM && pmu->monitor[mon].hw_events.num_monitored == 0) {
> +				set_monitor_control(pmu, mon, GEN_CTL_RESET_BIT);
> +				pmu->monitor[mon].hw_events.monitor_running = false;
> +			}
> +		}
> +	}
[ ... ]
> +static int rpi_axi_pmu__init(struct rpi_axi_pmu *pmu, struct platform_device *pdev)
> +{
[ ... ]
> +	ret = cpuhp_state_add_instance(rpi_axi_pmu_cpuhp_state, &pmu->cpuhp_node);
> +	if (ret) {
> +		dev_err(dev, "Failed to add cpuhp instance %d\n", ret);
> +		goto err_teardown;
> +	}
> +
> +	ret = perf_pmu_register(&pmu->pmu, PMU_NAME, /*type=*/-1);
> +	if (ret) {
> +		dev_err(dev, "PMU register failed %d\n", ret);
> +		goto err_cpuhp_remove;
> +	}
> +
> +	return 0;
> +
> +err_cpuhp_remove:
> +	cpuhp_state_remove_instance(rpi_axi_pmu_cpuhp_state, &pmu->cpuhp_node);

[Severity: High]
Does this error path trigger a use-after-free or panic during CPU context
migration?

If perf_pmu_register() fails, calling cpuhp_state_remove_instance() will
trigger the rpi_axi_pmu_offline_cpu() callback for online CPUs. That
callback executes perf_pmu_migrate_context(), which will attempt to migrate
a PMU that was never successfully registered and has a NULL context pointer.

> +err_teardown:
> +	hrtimer_cancel(&pmu->hrtimer);
> +	cancel_work_sync(&pmu->vpu_work);
> +err_firmware_put:
> +	if (pmu->firmware)
> +		rpi_firmware_put(pmu->firmware);
> +	return ret;
> +}
> +
> +/**
> + * rpi_axi_pmu__exit() - Internal PMU teardown called during remove
> + * @pmu: Pointer to PMU context
> + */
> +static void rpi_axi_pmu__exit(struct rpi_axi_pmu *pmu)
> +{
> +	perf_pmu_unregister(&pmu->pmu);
> +	cpuhp_state_remove_instance(rpi_axi_pmu_cpuhp_state, &pmu->cpuhp_node);

[Severity: High]
Can unregistering the PMU before removing the cpuhp instance cause a
use-after-free?

perf_pmu_unregister() frees the PMU's CPU context allocations. The subsequent
call to cpuhp_state_remove_instance() triggers the rpi_axi_pmu_offline_cpu()
callback for online CPUs.

This callback will then call perf_pmu_migrate_context(), which dereferences
the now-freed cpu_pmu_context, leading to a UAF. Should
cpuhp_state_remove_instance() be called before perf_pmu_unregister() to
ensure the PMU context remains valid during migration?

> +	hrtimer_cancel(&pmu->hrtimer);
> +	cancel_work_sync(&pmu->vpu_work);
> +	if (pmu->firmware)
> +		rpi_firmware_put(pmu->firmware);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1