Re: [PATCH v2 2/2] fs/resctrl: Do not allocate a monitor context when releasing limbo RMIDs

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

Hi Jinjie,

On 10/30/26 2:21 AM, Jinjie Ruan wrote:
> Busy RMIDs of a monitor domain are normally released by the limbo
> handler once their cache occupancy drops below the threshold. When a
> domain goes offline, its busy RMIDs are released without checking
> occupancy, since the domain is going away and the RMIDs must not be
> left behind.

nit: "left behind" is vague. How about changing "RMIDs must not be
left behind" to "RMIDs would otherwise never return to the free list."

> 
> On the monitor domain offline path, __check_limbo() allocates
> the monitor context that an occupancy check needs before it scans
> the busy RMIDs, but without using it, and returns early when that
> allocation fails. Then the caller destroys the domain's monitoring state,
> including the bitmap that records which RMIDs are busy, so nothing can
> clear those bits afterwards and the busy counts never reach zero. Those
> RMIDs never return to the free list and, where the RMID depends on
> the CLOSID, the dirty count of their CLOSID never decreases. The allocation
> can also wait for a monitor to become free, which stalls the CPU offline
> path on a context that is not going to be used.
> 
> Only MPAM systems are affected. An architecture that can always read an
> RMID allocates nothing for the monitor context, so the allocation can
> neither fail nor wait there.
> 
> So move the release of busy RMIDs into a separate function that does

nit: Please drop "So" to stick with imperative. Just start with "Move the release ..."

> not take a monitor context and leave the occupancy check to
> __check_limbo().
> 
> This is not a revert of the fixed commit. It preserves the original fix's
> intent of not reading a counter in the offline path.

More on this below but after reconsidering the fallout I think above paragraph
needs to be dropped.

For entire changelog, could you please adjust the line wraps after edit to not
jump around as much?

> 
> Fixes: fc16126cc11d ("x86,fs/resctrl: Prevent out-of-bounds access while offlining CPU when SNC enabled")

Taking a closer look, the issue as described is present before fc16126cc11d. 
At fc16126cc11d^, __check_limbo() already returns early on allocation failure,
and the force-release caller already falls through to domain_destroy_l3_mon_state(d),
so the RMIDs leak there too. 

The issue seems to originate from feature that added the allocation and early
release, which is:
e557999f80a5 ("x86/resctrl: Allow arch to allocate memory needed in resctrl_arch_rmid_read()")

Even so, as with patch #1, since the impact is only on MPAM this need not
be backported as far. I thus think the tags should be:
	Fixes: e557999f80a5 ("x86/resctrl: Allow arch to allocate memory needed in resctrl_arch_rmid_read()")
and
	Cc: <stable@vger.kernel.org> # 7.1.x

This fix does depend on fc16126cc11d to apply cleanly but fortunately it has been
backported to 7.1.

> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Reinette Chatre <reinette.chatre@intel.com>
> Cc: <stable@vger.kernel.org>
> ---

...

>  /*
>   * 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)
> +static void __check_limbo(struct rdt_l3_mon_domain *d)
>  {
>  	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>  	struct rmid_entry *entry;
> -	bool rmid_dirty = true;
> +	bool rmid_dirty;
>  	u32 idx, cur_idx = 1;
>  	void *arch_mon_ctx;
>  	void *arch_priv;

Another difference between MPAM and tip style is that tip prefers variable
declarations to be in reverse fir order. For reference, see "Variable declarations" in 
Documentation/process/maintainer-tip.rst.

Please re-order the declarations to maintain reverse fir.

Everything else looks good.

Thank you very much.

Reinette