Re: [PATCH 2/2] fs/resctrl: Skip monitor context allocation when force-freeing RMIDs

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Jinjie,

There is no "force-freeing" of RMIDs. How about
	fs/resctrl: Do not allocate a monitor context when releasing limbo RMIDs

On 10/29/26 1:15 AM, Jinjie Ruan wrote:
> __check_limbo() allocates the architecture monitor context before the
> scan loop, even when the domain is going offline and the RMIDs will
> be freed without reading their occupancy. So if the allocation fails,
> the offline path returns early and leaves the busy RMIDs in limbo
> which will be leaked instead of being released.

(please separate the context, problem, and solution descriptions).

"will be leaked" is a bit vague to gain confidence that there is an issue here.
Compare with, for example, "The caller goes on to destroy the domain's
monitoring state, freeing the bitmap that records which RMIDs are busy,
so nothing can clear those bits afterwards and the busy counts never reach zero."

I think that there is an argument for this change even in the successful
allocation path. If I understand correctly MPAM's resctrl_arch_mon_ctx_alloc()
may need to block as it waits for a monitor? This would cause the CPU-offline
path to block on a counter that will never be read.

> 
> Split out release_limbo_rmids() for the offline path, which doesn't need
> the monitor context.

Considering the shape of this fix and the "Fixes:" tag I think it may be
worth a mention that this is not a revert of the indicated commit. This change
maintains the intent of the original fix that avoids reading a counter in the
offline path.

> 
> Cc: James Morse <james.morse@arm.com>
> Cc: Reinette Chatre <reinette.chatre@intel.com>
> Fixes: fc16126cc11d ("x86,fs/resctrl: Prevent out-of-bounds access while offlining CPU when SNC enabled")

The changelog is missing a clear description of the impact of this issue.
Nowhere does it mention that this only impacts MPAM and how likely this issue is. 

This is important because this "Fixes:" commit has a "Cc: <stable@kernel.org>" so
we need to help maintainers determine the impact of this issue to 
guide where, if at all, it needs to be backported.

> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  fs/resctrl/internal.h |  3 +-
>  fs/resctrl/monitor.c  | 69 ++++++++++++++++++++++++++++---------------
>  fs/resctrl/rdtgroup.c |  2 +-
>  3 files changed, 48 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index e62a277dee85..a1837679b27a 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -405,7 +405,8 @@ void cqm_handle_limbo(struct work_struct *work);
>  
>  bool has_busy_rmid(struct rdt_l3_mon_domain *d);
>  
> -void __check_limbo(struct rdt_l3_mon_domain *d, bool force_free);
> +void __check_limbo(struct rdt_l3_mon_domain *d);

Looks like __check_limbo() can be static now?

> +void release_limbo_rmids(struct rdt_l3_mon_domain *d);
>  
>  void resctrl_file_fflags_init(const char *config, unsigned long fflags);
>  
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index b8f4e1ff4083..020140b794a7 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -124,13 +124,36 @@ static void limbo_release_entry(struct rmid_entry *entry)
>  		closid_num_dirty_rmid[entry->closid]--;
>  }
>  
> +/*
> + * Release the busy RMIDs of a domain that is going offline. Their occupancy
> + * can't be read any more, so they are freed without being checked.

Please document the function self, not the caller. Considering the comments in
resctrl_offline_mon_domain() the caller already does a good job at this.

The "Their occupancy can't be read any more" also does not look right. I think
this claim is based on x86's cpumask_empty() guard in resctrl_arch_rmid_read(), for
x86 that does not actually mean that the occupancy cannot be read anymore, just that
the CPU may already been removed from the domain at that point. This is really
a platform specific detail that resctrl fs cannot assume.

How about something like:

	/*
	 * Release the RMIDs that are marked as busy for this domain without
	 * checking their LLC occupancy. Cache lines may still be tagged with
	 * these RMIDs and will be accounted to the next user of each RMID.
	 */

I think such comment pairs well with the comment in the caller - it documents
the consequence that the caller documents it is willing to take.

> + */
> +void release_limbo_rmids(struct rdt_l3_mon_domain *d)
> +{
> +	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
> +	struct rmid_entry *entry;
> +	u32 idx, cur_idx = 1;
> +
> +	for (;;) {
> +		idx = find_next_bit(d->rmid_busy_llc, idx_limit, cur_idx);
> +		if (idx >= idx_limit)
> +			break;
> +
> +		entry = __rmid_entry(idx);
> +		clear_bit(idx, d->rmid_busy_llc);
> +		if (!--entry->busy)
> +			limbo_release_entry(entry);
> +		cur_idx = idx + 1;
> +	}
> +}
> +
>  /*
>   * Check the RMIDs that are marked as busy for this domain. If the
>   * reported LLC occupancy is below the threshold clear the busy bit and
>   * decrement the count. If the busy count gets to zero on an RMID, we
>   * free the RMID
>   */
> -void __check_limbo(struct rdt_l3_mon_domain *d, bool force_free)
> +void __check_limbo(struct rdt_l3_mon_domain *d)

Now __check_limbO() can be static?

>  {
>  	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
> @@ -161,30 +184,28 @@ void __check_limbo(struct rdt_l3_mon_domain *d, bool force_free)
>  			break;

To be true to the fix I think the rmid_dirty initializer added by fc16126cc11d
can also be removed?
	"bool rmid_dirty = true;" -> "bool rmid_dirty;"

>  
>  		entry = __rmid_entry(idx);
> -		if (!force_free) {
> -			if (resctrl_arch_rmid_read(r, &d->hdr, entry->closid,
> -						   entry->rmid, QOS_L3_OCCUP_EVENT_ID,
> -						   arch_priv, &val, arch_mon_ctx)) {
> -				rmid_dirty = true;
> -			} else {
> -				rmid_dirty = (val >= resctrl_rmid_realloc_threshold);
> -
> -				/*
> -				 * x86's CLOSID and RMID are independent numbers,
> -				 * so the entry's CLOSID is an empty CLOSID
> -				 * (X86_RESCTRL_EMPTY_CLOSID). On Arm the RMID
> -				 * (PMG) extends the CLOSID (PARTID) space with
> -				 * bits that aren't used to select the configuration.
> -				 * It is thus necessary to track both CLOSID and
> -				 * RMID because there may be dependencies between
> -				 * them on some architectures.
> -				 */
> -				trace_mon_llc_occupancy_limbo(entry->closid, entry->rmid,
> -							      d->hdr.id, val);
> -			}
> +		if (resctrl_arch_rmid_read(r, &d->hdr, entry->closid,
> +					   entry->rmid, QOS_L3_OCCUP_EVENT_ID,
> +					   arch_priv, &val, arch_mon_ctx)) {
> +			rmid_dirty = true;
> +		} else {
> +			rmid_dirty = (val >= resctrl_rmid_realloc_threshold);
> +
> +			/*
> +			 * x86's CLOSID and RMID are independent numbers,
> +			 * so the entry's CLOSID is an empty CLOSID
> +			 * (X86_RESCTRL_EMPTY_CLOSID). On Arm the RMID
> +			 * (PMG) extends the CLOSID (PARTID) space with
> +			 * bits that aren't used to select the configuration.
> +			 * It is thus necessary to track both CLOSID and
> +			 * RMID because there may be dependencies between
> +			 * them on some architectures.
> +			 */
> +			trace_mon_llc_occupancy_limbo(entry->closid, entry->rmid,
> +						      d->hdr.id, val);
>  		}
>  
> -		if (force_free || !rmid_dirty) {
> +		if (!rmid_dirty) {
>  			clear_bit(idx, d->rmid_busy_llc);
>  			if (!--entry->busy)
>  				limbo_release_entry(entry);
> @@ -843,7 +864,7 @@ void cqm_handle_limbo(struct work_struct *work)
>  	if (cpumask_empty(&d->hdr.cpu_mask))
>  		goto out_unlock;
>  
> -	__check_limbo(d, false);
> +	__check_limbo(d);
>  
>  	if (has_busy_rmid(d)) {
>  		d->cqm_work_cpu = cpumask_any_housekeeping(&d->hdr.cpu_mask,
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 57ab090072c2..5d0351453c2f 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -4590,7 +4590,7 @@ void resctrl_offline_mon_domain(struct rdt_resource *r, struct rdt_domain_hdr *h
>  		 * the RMID as busy creates RMID leaks if the
>  		 * package never comes back.
>  		 */
> -		__check_limbo(d, true);
> +		release_limbo_rmids(d);
>  	}
>  
>  	domain_destroy_l3_mon_state(d);

Thank you very much.

Reinette