Re: [Patch v3 1/4] perf/x86/intel: Improve validation and configuration of ACR masks

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] The ACR feature incorrectly maps logical sibling indices from user space to physical array indices in `event_list`, which silently corrupts the configuration if any sibling is disabled.
- [High] The updated ACR hardware configuration (`hw.config1`) is not synchronized to the PMU hardware register if the event itself does not change counters, leaving the hardware state stale.
- [Low] The patch uses a complex and brittle hardware counter bitmask to verify group membership instead of simply checking array bounds.
--

commit 0bc290b3247e55dbfb3b7cee24f6926da6f5adfb
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
Subject: perf/x86/intel: Improve validation and configuration of ACR masks

This commit addresses several issues with user space ACR mask validation and
configuration. It calculates an index bitmap for each ACR group to drop invalid
user-space mask bits, avoids early returns on invalid bits, and explicitly
clears stale hardware ACR masks.

> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index d9488ade0f8e..dafa233dd757 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
[ ... ]
> @@ -3332,23 +3332,41 @@ static void intel_pmu_enable_event(struct perf_event *event)
>  static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc)
>  {
>  	struct perf_event *event, *leader;
> -	int i, j, idx;
> +	int i, j, k, bit, idx;
> +	u64 group_mask;
>  
>  	for (i = 0; i < cpuc->n_events; i++) {
>  		leader = cpuc->event_list[i];
>  		if (!is_acr_event_group(leader))
>  			continue;
>  
> -		/* The ACR events must be contiguous. */
> +		/* Find the last event of the ACR group. */
>  		for (j = i; j < cpuc->n_events; j++) {
>  			event = cpuc->event_list[j];
>  			if (event->group_leader != leader->group_leader)
>  				break;
> -			for_each_set_bit(idx, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
> -				if (i + idx >= cpuc->n_events ||
> -				    !is_acr_event_group(cpuc->event_list[i + idx]))
> -					return;
> -				__set_bit(cpuc->assign[i + idx], (unsigned long *)&event->hw.config1);
> +		}
> +
> +		/* Figure out the group indices bitmap. */
> +		group_mask = 0;
> +		for (k = i; k < j; k++)
> +			group_mask |= BIT_ULL(cpuc->assign[k]);

Since x86_schedule_events() guarantees strictly unique hardware counters for
all concurrently active core PMU events, a counter is present in group_mask if
and only if its index falls within [i, j).

Given that idx = i + bit guarantees idx >= i, is it possible to simplify this
by just checking if idx >= j instead of constructing and checking the
group_mask?

> +
> +		/*
> +		 * Translate the user-space ACR mask (attr.config2) into the physical
> +		 * counter bitmask (hw.config1) for each ACR event in the group.
> +		 * NOTE: ACR event contiguity is guaranteed by intel_pmu_hw_config().
> +		 */
> +		for (k = i; k < j; k++) {
> +			event = cpuc->event_list[k];
> +			event->hw.config1 = 0;

If Event A moves to a new counter but Event B itself does not move,
match_prev_assignment() would evaluate to true for Event B, and
x86_pmu_start() would be skipped.

Does this mean the updated hw.config1 for Event B is never written to the
physical hardware MSR, breaking the Auto Counter Reload functionality?

> +			for_each_set_bit(bit, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
> +				idx = i + bit;

User space provides the ACR mask (attr.config2) using absolute logical bit
indices (e.g., bit 0 = leader, bit 1 = first sibling). However,
collect_events() skips events that are in PERF_EVENT_STATE_OFF.

If an earlier sibling is disabled, it is not added to event_list, causing all
subsequent active siblings to shift down in the array.

Does idx point to the wrong event in the active array if any sibling is
disabled? Should the mask resolve against the logical sibling_list rather
than the compressed active array?

> +				if (idx >= cpuc->n_events ||
> +				    !(BIT_ULL(cpuc->assign[idx]) & group_mask) ||
> +				    !is_acr_event_group(cpuc->event_list[idx]))
> +					continue;
> +				__set_bit(cpuc->assign[idx], (unsigned long *)&event->hw.config1);
>  			}
>  		}

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