Re: [PATCH v2 2/2] perf: Add Raspberry Pi 5 (BCM2712) AXI PMU support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect subsystem teardown ordering in both the exit and init error paths fails to cancel background tasks, leading to Use-After-Free.
- [Medium] Incomplete hiding of legacy BCM2835-specific sysfs event aliases on BCM2712.
- [Medium] Dropping the spinlock in the VPU work handler causes a TOCTOU race that corrupts hardware configurations.
- [Low] Invalid Kconfig dependency on non-existent symbol `ARCH_BCM2712`.
--

--- Patch [2]: [PATCH v2 2/2] perf: Add Raspberry Pi 5 (BCM2712) AXI PMU support ---
commit c6c37f9c3fb56c6bdbc0982c7918061dbe1d45cb
Author: Ian Rogers <irogers@google.com>

perf: Add Raspberry Pi 5 (BCM2712) AXI PMU support

Expand the Raspberry Pi AXI PMU driver to support Broadcom BCM2712
(Raspberry Pi 5) system monitors, PCIe RP1 Southbridge links, HEVC decoder,
and Cortex-A76 DSU L3 interconnects.

> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 97c8bf34f4633..b0c4eb0748dd3 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -327,11 +327,11 @@ config NVIDIA_TEGRA410_C2C_PMU
>  
>  config RASPBERRYPI_AXI_PMU
>  	tristate "Raspberry Pi AXI PMU support"
> -	depends on ARCH_BCM2835 || ARCH_BRCMSTB || COMPILE_TEST
> +	depends on ARCH_BCM2835 || ARCH_BCM2712 || ARCH_BRCMSTB || COMPILE_TEST

[Severity: Low]
Does this code add a dependency on an undefined symbol?

ARCH_BCM2712 does not appear to be defined anywhere in the kernel tree.

>  	select RASPBERRYPI_FIRMWARE if ARCH_BCM2835
>  	help
>  	  Support for the AXI Performance Monitoring Unit (PMU) found on Broadcom
> -	  SoCs used in Raspberry Pi platforms (BCM2835, BCM2836, BCM2837, BCM2711).
> +	  SoCs used in Raspberry Pi platforms (BCM2835, BCM2836, BCM2837, BCM2711, BCM2712).
>  
>  	  If compiled as a module, it will be called rpi_axi_pmu.
>  
> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> index cdf7d9a74f13e..b13632fa8f84c 100644
> --- a/drivers/perf/rpi_axi_pmu.c
> +++ b/drivers/perf/rpi_axi_pmu.c
[ ... ]
> @@ -867,12 +1043,94 @@ static struct attribute *rpi_axi_pmu_events_attrs[] = {
[ ... ]
> +static umode_t rpi_axi_pmu_events_is_visible(struct kobject *kobj,
> +					      struct attribute *attr, int unused)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct rpi_axi_pmu *pmu = dev_get_drvdata(dev);
> +	struct perf_pmu_events_attr *pmu_attr;
> +
> +	pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr.attr);
> +
> +	if (pmu->chip == CHIP_BCM2712) {
> +		/*
> +		 * On RPi 5 (BCM2712), hide legacy VPU Mailbox IPC events (monitor=1)
> +		 * and RPi 1-4 aliases
> +		 */
> +		if (strstr(pmu_attr->event_str, "monitor=1") ||
> +		    strstr(attr->name, "cpu0_") ||
> +		    strstr(attr->name, "cpu1_") ||
> +		    strstr(attr->name, "dma0_") ||
> +		    strstr(attr->name, "dma1_") ||
> +		    strstr(attr->name, "v3d0_") ||
> +		    strstr(attr->name, "v3d1_") ||
> +		    strstr(attr->name, "hvs_system") ||
> +		    strstr(attr->name, "isp_system") ||
> +		    strstr(attr->name, "usb_system"))
> +			return 0;

[Severity: Medium]
Are there missing legacy aliases that should be hidden here?

This hides VPU IPC events on RPi 5, but it looks like several legacy System
Monitor aliases for diverged buses (such as ccp2tx, mphi_rx, h264, v3d, 
peripheral, cpu_uc, cpu_l2) are not included in this check.

Opening them might measure the wrong hardware unit or fail with -EINVAL since 
their hardcoded bus indices exceed BCM2712_SB__MAX.

> +	} else {
> +		/* On RPi 1-4 (BCM2835-BCM2711), hide RPi 5 specific events */
> +		if (strstr(attr->name, "pcie_rp1") ||
> +		    strstr(attr->name, "a76_") ||
> +		    strstr(attr->name, "v3d7_") ||
> +		    strstr(attr->name, "hevc_dec") ||
> +		    strstr(attr->name, "arm_") ||
> +		    strstr(attr->name, "pcie0_") ||
> +		    strstr(attr->name, "hvs_filtered") ||
> +		    strstr(attr->name, "emmc0_"))
> +			return 0;
> +	}
> +
> +	return attr->mode;
> +}
[ ... ]
> @@ -1549,7 +1807,7 @@ static int rpi_axi_pmu__init(struct rpi_axi_pmu *pmu, struct platform_device *pd
>  	spin_lock_init(&pmu->lock);
>  	mutex_init(&pmu->vpu_mutex);
>  
> -	pmu->chip = CHIP_BCM2835;
> +	pmu->chip = (enum rpi_axi_chip)(uintptr_t)of_device_get_match_data(dev);
>  
>  	pmu->pmu = (struct pmu) {
>  		.module = THIS_MODULE,

[Severity: High]
Can the error paths in rpi_axi_pmu__init() and the teardown in 
rpi_axi_pmu__exit() lead to a use-after-free?

If cpuhp_state_add_instance() fails in rpi_axi_pmu__init(), the driver calls 
perf_pmu_unregister() but doesn't cancel the background tasks:

rpi_axi_pmu__init() {
    ...
err_pmu_unregister:
	perf_pmu_unregister(&pmu->pmu);
err_firmware_put:
	if (pmu->firmware)
		rpi_firmware_put(pmu->firmware);
	return ret;
}

Since PMU memory is freed via devres upon returning the error, an active timer
or workqueue (started by an event opened during the registration window) could 
fire and access freed memory. 

Similarly, in rpi_axi_pmu__exit(), the background tasks are cancelled before 
unregistering the PMU subsystem:

rpi_axi_pmu__exit() {
	cpuhp_state_remove_instance(rpi_axi_pmu_cpuhp_state, &pmu->cpuhp_node);
	hrtimer_cancel(&pmu->hrtimer);
	cancel_work_sync(&pmu->vpu_work);
    ...
	perf_pmu_unregister(&pmu->pmu);
}

A concurrent userspace action could start a new event in this window,
restarting the timer and causing a use-after-free when the memory is
subsequently freed.

[Severity: Medium]
Is there a time-of-check to time-of-use race condition in 
rpi_axi_pmu_vpu_work_handler()?

When the lock is dropped, a concurrent event deletion and addition could 
potentially misconfigure the PMU watcher:

rpi_axi_pmu_vpu_work_handler() {
    ...
		if (!pmu->monitor[MON__VPU].hw_events.enabled[idx]) {
			int bus = pmu->monitor[MON__VPU].hw_events.monitored_bus[idx];
			int filter = pmu->monitor[MON__VPU].hw_events.filter[idx];

			spin_unlock_irq(&pmu->lock);
			rpi_axi_pmu_enable_bus_watcher(pmu, MON__VPU, idx, bus, filter);
			spin_lock_irq(&pmu->lock);
		}
    ...
}

Concurrently, rpi_axi_pmu_del() can delete the event and free the idx slot, 
which rpi_axi_pmu_add() can then reuse for a new event with different 
configurations. 

Because the old values were cached before dropping the lock, they might be 
forcefully written to the hardware, and enabled[idx] is modified outside the 
lock:

rpi_axi_pmu_enable_bus_watcher() {
    ...
	pmu->monitor[mon].hw_events.enabled[idx] = true;
    ...
}

Could this silently misconfigure the PMU watcher for the new event?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811235112.2267241-1-irogers@google.com?part=2