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

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 9/28/2026 4:12 PM, sashiko-bot@kernel.org wrote:
> 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?

No, the ctx->mutex is held for sibling events. perf_event_ctx_lock_nested()
would acquire ctx->mutex before calling pmu->event_init().


>
>> +					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.

It's just a risk in theory, it should never really happen. slots event and
topdown metric events must be in a group and all events should be enabled
and scheduled at the same time, so it's impossible that we see a slots is
running but the topdown metric events in same group are still in creation.


>
> [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.

If an event member fails to pass the validation, but the other members in
the group still could success, we should not roll back the setting if a
member fails to pass the validation. 

Thanks.


>