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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Registering the PMU before completing all initialization steps creates a use-after-free race condition if probe fails.
- [Low] Usage of `#if LINUX_VERSION_CODE` shim for backward compatibility in an upstream patch.
--

--- Patch [1]: [PATCH v8 1/2] perf: Add Raspberry Pi BCM2835 AXI PMU driver ---
commit e754b178c7f623d2ebff89b932e852beed8622b9
Author: Ian Rogers <[email protected]>
Subject: perf: Add Raspberry Pi BCM2835 AXI PMU driver

This commit adds a new performance monitoring driver for the Raspberry Pi 
AXI bus (BCM2835/2711), exposing system-level and VideoCore PMU hardware to 
the Linux perf subsystem.

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> new file mode 100644
> index 0000000000000..676c2a3d5962e
> --- /dev/null
> +++ b/drivers/perf/rpi_axi_pmu.c
[ ... ]
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 13, 0)
> +static inline void rpi_hrtimer_setup(struct hrtimer *timer,
> +				     enum hrtimer_restart (*function)(struct hrtimer *),
> +				     clockid_t clock_id, enum hrtimer_mode mode)
> +{
> +	hrtimer_init(timer, clock_id, mode);
> +	timer->function = function;
> +}
> +#define hrtimer_setup rpi_hrtimer_setup
> +#endif

[Severity: Low]
This isn't a bug, but is this version compatibility shim intended for upstream 
inclusion? Upstream patches typically target the current API of the tree they 
are being merged into without including out-of-tree macro shims for backward 
compatibility with older kernels.

[ ... ]
> +	ret = perf_pmu_register(&pmu->pmu, PMU_NAME, /*type=*/-1);
> +	if (ret) {
> +		dev_err(dev, "PMU register failed %d\n", ret);
> +		goto err_teardown;
> +	}
> +
> +	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_pmu_unregister;
> +	}

[Severity: High]
Could this expose a use-after-free race condition if probe fails here?

When perf_pmu_register() completes, the PMU is exposed to userspace. If 
cpuhp_state_add_instance() fails immediately after, the error path goes to 
err_pmu_unregister and calls perf_pmu_unregister(). 

However, perf_pmu_unregister() does not revoke existing open events. If 
userspace opens a perf event on this PMU in the small window between 
registration and unregistration, the event will retain a pointer to the pmu 
struct. 

Because the pmu struct was allocated with devm_kzalloc(), it will be freed 
by the devres manager as soon as the probe function returns the error, leaving 
the open event with a dangling pointer that can corrupt memory upon subsequent 
accesses. Would it be safer to register the CPU hotplug instance before 
exposing the PMU to userspace?

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