Re: [PATCH 2/3] perf: Add Broadcom BCM5301X AXI Trace Module counters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] perf: bcm5301x_axi_event_init() calls for_each_sibling_event() with NULL ctx
- [Low] docs: perf: title underline too short in bcm5301x-axi-pmu.rst
--

commit 98ec05a97143b2d8c2fd04e15999d3fc2b116134
Author: Florian Fainelli <florian.fainelli@broadcom.com>

perf: Add Broadcom BCM5301X AXI Trace Module counters

This commit adds a PMU driver and documentation for the Broadcom BCM5301X
AXI Trace Module counters, exposing 16 unfiltered fixed counters through
system-wide perf events with hrtimer-based 32-bit delta accounting.

> diff --git a/Documentation/admin-guide/perf/bcm5301x-axi-pmu.rst b/Documentation/admin-guide/perf/bcm5301x-axi-pmu.rst
> new file mode 100644
> index 0000000000000..720f042dfbd0b
> --- /dev/null
> +++ b/Documentation/admin-guide/perf/bcm5301x-axi-pmu.rst
> @@ -0,0 +1,92 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +Broadcom BCM5301X AXI Trace Module PMU
> +====================================

[Severity: Low]
In Documentation/admin-guide/perf/bcm5301x-axi-pmu.rst, the title underline is
36 characters while the title is 38 characters. Should the underline be
extended by two characters so make htmldocs does not emit a title underline
too short warning?

[ ... ]

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

[ ... ]

> +static int bcm5301x_axi_event_init(struct perf_event *event)
> +{
> +	struct bcm5301x_axi_pmu *axi = to_axi_pmu(event->pmu);
> +	struct perf_event *sibling, *leader = event->group_leader;
> +	unsigned long used = 0;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK ||
> +	    event->cpu < 0)
> +		return -EOPNOTSUPP;
> +	if (event->attr.config >= ATM_NR_COUNTERS || event->attr.config1 ||
> +	    event->attr.config2)
> +		return -EINVAL;
> +
> +	/* A group cannot allocate the same fixed counter twice. */
> +	if (!is_software_event(leader)) {
> +		if (leader->pmu != event->pmu)
> +			return -EINVAL;
> +		__set_bit(leader->attr.config, &used);
> +	}
> +	for_each_sibling_event(sibling, leader) {
> +		if (is_software_event(sibling))
> +			continue;
> +		if (sibling->pmu != event->pmu ||
> +		    test_and_set_bit(sibling->attr.config, &used))
> +			return -EINVAL;
> +	}
> +	if (event != leader && test_bit(event->attr.config, &used))
> +		return -EINVAL;

[Severity: High]
When userspace opens a standalone event or a group leader (where
event == leader), can calling for_each_sibling_event(sibling, leader) in
bcm5301x_axi_event_init() in drivers/perf/bcm5301x_axi_pmu.c trigger a
lockdep warning or dereference a near-NULL pointer?

In the perf_event_alloc() -> perf_init_event() -> perf_try_init_event() path
in kernel/events/core.c, leader->ctx->mutex is only locked when
event->group_leader != 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);

When event == leader, leader->ctx is still NULL (it is only assigned later in
perf_install_in_context()). Calling for_each_sibling_event(sibling, leader) in
include/linux/perf_event.h expands to lockdep_assert_event_ctx(leader):

#define lockdep_assert_event_ctx(event)			\
	WARN_ON_ONCE(__lockdep_enabled &&			\
		     (this_cpu_read(hardirqs_enabled) &&	\
		      lockdep_is_held(&(event)->ctx->mutex) != LOCK_STATE_HELD))

With CONFIG_PROVE_LOCKING enabled, this passes &(leader)->ctx->mutex (an
offset from NULL) to lockdep_is_held(), which triggers the WARN_ON_ONCE
assertion and can fault in match_held_lock() if any held lock in
current->held_locks has hlock->references != 0.

Should the group validation block in bcm5301x_axi_event_init() be guarded by
if (event != leader)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026211151.3212237-1-florian.fainelli@broadcom.com?part=2