Re: [PATCH] mm/swap: publish cluster tables after full initialization

On Thu, Aug 13, 2026 at 11:03:16PM +0800, Longlong Xia wrote:

+Cc: Kemeng Shi <[email protected]>

Hello Longlong Xia

> From: Longlong Xia <[email protected]>
> 
> swap_cluster_populate() drops the local, global, and cluster locks
> before its sleeping allocation.  The allocation helper publishes ci->table
> before allocating the memcg table and, on some 32-bit configurations, the
> zero bitmap.
> 
> A stale per-CPU or global cluster cursor can reach the isolated cluster in
> that window.  Since CLUSTER_FLAG_NONE and a non-NULL table make the cluster
> appear usable, it can allocate a slot without the auxiliary state.  An
> auxiliary allocation failure can then tear down a table which is already in
> use.

I think this issue is addressed in Kemeng Shi's patch, which provides a
detailed reproduce scenario.
https://lore.kernel.org/linux-mm/[email protected]/

In short, referencing the table while auxiliary table allocation fails can
lead to a panic or something due to a NULL pointer dereference. That patch only fixed
the populate code rather than the entire allocation path.

However, as I mentioned in my previous review.
https://lore.kernel.org/linux-mm/al7WTNXanPVSELAH@yjaykim-PowerEdge-T330/
I suggested making it visible only when the ci->table installation succeeds,
even in the CLUSTER_FLAG_NONE case. If we do this, seeing the table in the
CLUSTER_FLAG_NONE case would not trigger the auxiliary allocation teardown
mentioned in the problem description.

> Allocate the complete set of tables into a private carrier.  Install the
> auxiliary pointers and publish the main table only while holding ci->lock;
> the slow path does this after reacquiring all allocator locks.  Allocation
> failures now free only unpublished resources.
> 
> Fixes: b197d41462c2 ("mm/memcg, swap: store cgroup id in cluster table directly")
> Cc: [email protected]

I see this commit is included in v7.2-rc1:
$ git describe --contains b197d41462c2
v7.2-rc1\~94^2\~147

Since it's in v7.2-rc1, is `Cc: [email protected]` necessary?

> @@ -788,6 +849,7 @@ static int swap_cluster_setup_bad_slot(struct swap_info_struct *si,
>  				       struct swap_cluster_info *cluster_info,
>  				       unsigned int offset, bool mask)
>  {
> +	struct swap_cluster_tables tables = {};
>  	unsigned int ci_off = offset % SWAPFILE_CLUSTER;
>  	unsigned long idx = offset / SWAPFILE_CLUSTER;
>  	struct swap_cluster_info *ci;
> @@ -812,9 +874,14 @@ static int swap_cluster_setup_bad_slot(struct swap_info_struct *si,
>  
>  	ci = cluster_info + idx;
>  	/* Need to allocate swap table first for initial bad slot marking. */
> -	if (!ci->count && swap_cluster_alloc_table(ci, GFP_KERNEL))
> -		return -ENOMEM;
> +	if (!ci->count) {
> +		ret = swap_cluster_tables_alloc(&tables, GFP_KERNEL);
> +		if (ret)
> +			return ret;
> +	}
>  	spin_lock(&ci->lock);
> +	if (tables.table)
> +		swap_cluster_tables_install(ci, &tables);
>  	/* Check for duplicated bad swap slots. */
>  	if (__swap_table_xchg(ci, ci_off, SWP_TB_BAD) != SWP_TB_NULL) {

As discussed before, this routine is set up before the swap device becomes
visible, so it doesn't actually benefit from this fix (although it inevitably
needs to be modified to align with the current direction).

Overall, I prefer the direction I suggested in my previous review. (If my assumtion is right)
https://lore.kernel.org/linux-mm/al7WTNXanPVSELAH@yjaykim-PowerEdge-T330/
or Kemeng's current suggestion.

(+ Also, if this needs to go into the 7.2 hotfix, Kemeng Shi or Longlong Xia
can submit it. Also, I can help to submit it based on my reviewed direction 
with both of your tags included.)

Thanks
Youngjun