Re: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] x86/resctrl: MBM counter leak and hardware misconfiguration for pseudo-locked groups
--

commit b647b3c26df799d4d9b84cff6fe64eee2efa36ad
Author: Babu Moger <babu.moger@amd.com>
fs/resctrl: Assign counters to existing groups when enabling mbm_event

When switching to mbm_event counter assignment mode, existing monitoring
groups are not automatically assigned counters, leaving them unassigned.
This commit walks every existing group and assigns available counters to
match the auto-assignment behavior that occurs for new groups.

Closes: https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com

> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index a932a1fea8182..84d422da1bace 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c

[ ... ]

> @@ -1272,6 +1275,22 @@ void rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp)
>  					   &mon_event_all[QOS_L3_MBM_LOCAL_EVENT_ID]);
>  }
>  
> +/*
> + * resctrl_assign_cntrs_allrdtgrp() - Assign counters to the MBM events of
> + *				      every existing group.
> + */
> +static void resctrl_assign_cntrs_allrdtgrp(void)
> +{
> +	struct rdtgroup *prgrp, *crgrp;
> +
> +	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> +		rdtgroup_assign_cntrs(prgrp);

[Severity: High]
Does this code inadvertently leak hardware counters to pseudo-locked groups?

When a user creates a control group and transitions it to pseudo-locked mode,
its rmid and closid are explicitly freed. If 'mbm_event' assignment mode is
then enabled globally, this traversal loop captures those pseudo-locked groups
because they are retained in rdt_all_groups.

Since rdtgroup_assign_cntrs() lacks a check for
rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED, the counter allocation proceeds.

Could this cause rdtgroup_alloc_assign_cntr() to program the hardware using
the stale, freed rmid and closid of the pseudo-locked group? This appears to
permanently leak scarce hardware counters and potentially corrupt the hardware
monitoring state of other active groups that have been re-allocated those IDs.

> +
> +		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list)
> +			rdtgroup_assign_cntrs(crgrp);
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791589412.git.babu.moger@amd.com?part=2