Re: [PATCH v15 2/3] perf: Add Raspberry Pi BCM2835 AXI PMU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] perf: rpi_axi_pmu: NULL ctx deref in rpi_axi_pmu__validate_group()
- [Low] perf: rpi_axi_pmu: enforce RPI_AXI_MAX_EVENTS limit in group validation
- [Low] perf: rpi_axi_pmu: Kconfig depends on undefined symbol RPI_AXIPERF
--

commit 3a239421611725bee7d8e283cbdeacf0ec2bf701
Author: Ian Rogers <irogers@google.com>

perf: Add Raspberry Pi BCM2835 AXI PMU driver

This commit adds a perf PMU driver for the Raspberry Pi System and VideoCore
AXI performance monitors on Broadcom BCM2835 and BCM2711 platforms.

> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 245e7bb763b99..c1d7d64caba28 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -325,4 +325,18 @@ config NVIDIA_TEGRA410_C2C_PMU
>  	  Enable perf support for counters in NVIDIA C2C interface of NVIDIA
>  	  Tegra410 SoC.
>  
> +config RPI_AXI_PMU
> +	tristate "Raspberry Pi AXI Performance Monitor"
> +	depends on ARCH_BCM2835 || COMPILE_TEST
> +	depends on !RPI_AXIPERF

[Severity: Low]
Should this dependency on !RPI_AXIPERF be removed from the patch before it is
applied?

RPI_AXIPERF is not defined in the mainline kernel tree, and although the note
below the --- separator asks maintainers to delete this line when applying,
tools like git am and b4 strip the note and commit the undefined Kconfig
symbol directly, which triggers scripts/checkkconfigsymbols.py warnings.

[ ... ]

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> new file mode 100644
> index 0000000000000..f194b8633e434
> --- /dev/null
> +++ b/drivers/perf/rpi_axi_pmu.c

[ ... ]

> +static bool rpi_axi_pmu__validate_group(struct perf_event *event)
> +{
> +	struct perf_event *sibling, *leader = event->group_leader;
> +	struct rpi_axi_hw_events fake_hw_events[MON_MAX];
> +
> +	rpi_axi_hw_events__init(&fake_hw_events[MON_SYSTEM]);
> +	rpi_axi_hw_events__init(&fake_hw_events[MON_VPU]);
> +	if (!rpi_axi_pmu__validate_event(event->pmu, fake_hw_events, leader))
> +		return false;
> +	for_each_sibling_event(sibling, leader) {
> +		if (!rpi_axi_pmu__validate_event(event->pmu, fake_hw_events, sibling))
> +			return false;
> +	}
> +	return rpi_axi_pmu__validate_event(event->pmu, fake_hw_events, event);
> +}

[Severity: High]
Can calling for_each_sibling_event(sibling, leader) when leader == event
cause a NULL pointer dereference under CONFIG_PROVE_LOCKING=y, and also
validate the group leader twice?

When userspace opens a standalone event or group leader via perf_event_open()
with group_fd == -1, perf_event_alloc() sets event->group_leader = event and
calls perf_init_event() -> perf_try_init_event() while event->ctx is still
NULL:

kernel/events/core.c:perf_try_init_event() {
	if (event->group_leader != event && pmu->task_ctx_nr != perf_sw_context) {
		ctx = perf_event_ctx_lock_nested(event->group_leader,
						 SINGLE_DEPTH_NESTING);
		BUG_ON(!ctx);
	}

	event->pmu = pmu;
	ret = pmu->event_init(event);
}

Because rpi_axi_pmu_event_init() -> rpi_axi_pmu__validate_group() does not
return early when leader == event, it proceeds to for_each_sibling_event():

include/linux/perf_event.h:
#define for_each_sibling_event(sibling, event)			\
	lockdep_assert_event_ctx(event);			\
	if ((event)->group_leader == (event))			\
		list_for_each_entry((sibling), &(event)->sibling_list, sibling_list)

With CONFIG_PROVE_LOCKING=y, lockdep_assert_event_ctx(leader) evaluates
lockdep_is_held(&leader->ctx->mutex) while leader->ctx is NULL, dereferencing
a near-NULL address inside lockdep. In addition, when leader == event, the
final rpi_axi_pmu__validate_event(event->pmu, fake_hw_events, event) call
validates the leader a second time.

Should rpi_axi_pmu__validate_group() return true immediately after validating
leader when leader == event (or skip calling rpi_axi_pmu__validate_group() in
rpi_axi_pmu_event_init() when event->group_leader == event)?

[ ... ]

> +static int rpi_axi_pmu_add(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);
> +	struct rpi_axi_hw_events *hw_events = &pmu->monitor[mon].hw_events;
> +	unsigned long spinflags;
> +	int idx, slot = -1;
> +
> +	raw_spin_lock_irqsave(&pmu->lock, spinflags);
> +	idx = rpi_axi_hw_events__get_alloc_event_idx(hw_events, event);
> +	if (idx < 0) {
> +		raw_spin_unlock_irqrestore(&pmu->lock, spinflags);
> +		return -EAGAIN;
> +	}
> +
> +	for (int i = 0; i < RPI_AXI_MAX_EVENTS; i++) {
> +		if (!pmu->events[i]) {
> +			slot = i;
> +			pmu->events[i] = event;
> +			break;
> +		}
> +	}
> +
> +	if (slot < 0) {

[Severity: Low]
What happens if userspace creates an event group with more than
RPI_AXI_MAX_EVENTS (66) hardware events targeting this PMU that share at most
3 (bus, filter) pairs per monitor?

In rpi_axi_pmu_event_init() -> rpi_axi_pmu__validate_group() ->
rpi_axi_pmu__validate_event() -> rpi_axi_hw_events__get_alloc_event_idx(),
events targeting the same (monitor, bus, filter) increment
fake_hw_events[mon].refcount[i] without checking the total number of PMU
events against RPI_AXI_MAX_EVENTS, so rpi_axi_pmu_event_init() succeeds.

However, when group_sched_in() attempts to schedule the group via
rpi_axi_pmu_add(), the 67th event will find no free slot in
pmu->events[RPI_AXI_MAX_EVENTS] and fail with -ENOSPC, causing
group_sched_in() to roll back all 66 previously added events on every
multiplexing rotation tick.

Should rpi_axi_pmu__validate_group() also reject groups whose total number of
events on this PMU exceeds RPI_AXI_MAX_EVENTS?

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