Re: [PATCH 15/15] perf/x86/intel: Allow Topdown metrics with a non-leader slots event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] perf: lockless sibling traversal and modification in intel_pmu_hw_config()
- [Medium] perf: state leak on active slots event if metric event allocation fails
--

commit 35318b4bb32158e6a24c6de2f1ef286a1ed4b438
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
Subject: perf/x86/intel: Allow Topdown metrics with a non-leader slots event

This commit relaxes the requirement for the slots event to be the group leader
for Topdown metrics, allowing metric events to validate constraints against a
valid slots sibling instead. It incrementally validates groups and sets
reliance capabilities so detaching a non-leader slots event triggers dependent
metric events into an error state.

> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 6cc1462c9c82d..9025f29f625d4 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
[ ... ]
> @@ -5300,30 +5300,53 @@ static int intel_pmu_hw_config(struct perf_event *event)
>  
>  		if (is_available_metric_event(event)) {
>  			struct perf_event *leader = event->group_leader;
> +			struct perf_event *slots = NULL;
> +			struct perf_event *sibling;
>  
>  			/* The metric events don't support sampling. */
>  			if (is_sampling_event(event))
>  				return -EINVAL;
>  
> -			/* The metric events require a slots group leader. */
> -			if (!is_slots_event(leader))
> +			/*
> +			 * intel_pmu_hw_config() cannot tell whether the current
> +			 * event is the last one in the group. Require the slots
> +			 * event to appear before all metric events.
> +			 */
> +			if (is_slots_event(leader)) {
> +				slots = leader;
> +			} else if (leader->nr_siblings) {
> +				for_each_sibling_event(sibling, leader) {

[Severity: High]
When a user calls perf_event_open() to add a metric event to an already
active hardware group, this is executed without holding the group leader's
ctx->mutex.

Does traversing the sibling_list without the context lock here cause a data
race? A concurrent thread closing a sibling file descriptor triggers
perf_group_detach(), which mutates the list and could lead to list
corruption or a use-after-free.

Additionally, since for_each_sibling_event() invokes
lockdep_assert_event_ctx(), will this trigger a WARN_ON_ONCE because the
required lock is not held?

> +					if (is_slots_event(sibling)) {
> +						slots = sibling;
> +						break;
> +					}
> +				}
> +			}
[ ... ]
> +			if (slots == leader) {
> +				event->event_caps |= PERF_EV_CAP_SIBLING;
> +			} else {
> +				slots->event_caps |= PERF_EV_CAP_RELIED_ON;
> +				event->event_caps |= PERF_EV_CAP_RELIANT;
> +			}
>  			/*
>  			 * Only once we have a METRICs sibling do we
>  			 * need TopDown magic.
>  			 */
> -			leader->hw.flags |= PERF_X86_EVENT_TOPDOWN;
> -			event->hw.flags  |= PERF_X86_EVENT_TOPDOWN;
> +			slots->hw.flags |= PERF_X86_EVENT_TOPDOWN;

[Severity: High]
Does locklessly modifying the active slots event's event_caps and hw.flags
cause issues for running events? Setting PERF_X86_EVENT_TOPDOWN on a
running event changes its NMI read path to intel_update_topdown_event.
Because its saved_slots was uninitialized (0), this could subtract 0 from
the raw MSR value, resulting in double-counting the event.

[Severity: Medium]
What happens to these modifications if the new metric event allocation
fails later on (for instance, in perf_allow_cpu(),
security_perf_event_alloc(), or perf_cgroup_connect())?

Since there is no rollback mechanism in the error path to restore the slots
event, it appears to permanently leak state. Being incorrectly flagged with
PERF_EV_CAP_RELIED_ON and PERF_X86_EVENT_TOPDOWN permanently alters its
NMI read path and breaks its sampling capabilities, even though the
dependent metric event was never successfully created.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928074309.898043-1-dapeng1.mi@linux.intel.com?part=15