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

[Kairui Song <ryncsn@gmail.com>]

On Thu, Aug 13, 2026 at 11:03:16PM +0800, Longlong Xia wrote:
> From: Longlong Xia <xialonglong@kylinos.cn>
> 
> 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.
> 
> 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: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6-sol
> Signed-off-by: Longlong Xia <xialonglong@kylinos.cn>
> ---
>  mm/swapfile.c | 167 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 117 insertions(+), 50 deletions(-)
> 

Hi Longlong, thanks for the patch and report.

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 78b49b0658ad..2ca947c540e9 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -410,6 +410,99 @@ static inline unsigned int cluster_offset(struct swap_info_struct *si,
>  	return cluster_index(si, ci) * SWAPFILE_CLUSTER;
>  }
>  
> +struct swap_cluster_tables {
> +	struct swap_table *table;
> +#ifdef CONFIG_MEMCG
> +	struct swap_memcg_table *memcg_table;
> +#endif
> +#if !SWAP_TABLE_HAS_ZEROFLAG
> +	unsigned long *zero_bitmap;
> +#endif
> +};
> +
> +static void swap_cluster_tables_free(struct swap_cluster_tables *tables)
> +{
> +#ifdef CONFIG_MEMCG
> +	kfree(tables->memcg_table);
> +	tables->memcg_table = NULL;
> +#endif
> +
> +#if !SWAP_TABLE_HAS_ZEROFLAG
> +	kfree(tables->zero_bitmap);
> +	tables->zero_bitmap = NULL;
> +#endif
> +
> +	if (!tables->table)
> +		return;
> +
> +	if (SWP_TABLE_USE_PAGE)
> +		folio_put(virt_to_folio(tables->table));
> +	else
> +		kmem_cache_free(swap_table_cachep, tables->table);
> +	tables->table = NULL;
> +}
> +
> +static int swap_cluster_tables_alloc(struct swap_cluster_tables *tables,
> +				     gfp_t gfp)
> +{
> +	struct folio *folio;
> +
> +	if (SWP_TABLE_USE_PAGE) {
> +		folio = folio_alloc(gfp | __GFP_ZERO, 0);
> +		if (folio)
> +			tables->table = folio_address(folio);
> +	} else {
> +		tables->table = kmem_cache_zalloc(swap_table_cachep, gfp);
> +	}
> +	if (!tables->table)
> +		return -ENOMEM;
> +
> +#ifdef CONFIG_MEMCG
> +	if (!mem_cgroup_disabled()) {
> +		tables->memcg_table = kzalloc_obj(*tables->memcg_table, gfp);
> +		if (!tables->memcg_table)
> +			goto free_tables;
> +	}
> +#endif
> +
> +#if !SWAP_TABLE_HAS_ZEROFLAG
> +	tables->zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
> +	if (!tables->zero_bitmap)
> +		goto free_tables;
> +#endif
> +
> +	return 0;
> +
> +#if defined(CONFIG_MEMCG) || !SWAP_TABLE_HAS_ZEROFLAG
> +free_tables:
> +	swap_cluster_tables_free(tables);
> +	return -ENOMEM;
> +#endif
> +}
> +
> +static void swap_cluster_tables_install(struct swap_cluster_info *ci,
> +					struct swap_cluster_tables *tables)
> +{
> +	lockdep_assert_held(&ci->lock);
> +	VM_WARN_ON_ONCE(ci->flags || !cluster_is_empty(ci));
> +	VM_WARN_ON_ONCE(rcu_access_pointer(ci->table));
> +
> +#ifdef CONFIG_MEMCG
> +	VM_WARN_ON_ONCE(ci->memcg_table);
> +	ci->memcg_table = tables->memcg_table;
> +	tables->memcg_table = NULL;
> +#endif
> +
> +#if !SWAP_TABLE_HAS_ZEROFLAG
> +	VM_WARN_ON_ONCE(ci->zero_bitmap);
> +	ci->zero_bitmap = tables->zero_bitmap;
> +	tables->zero_bitmap = NULL;
> +#endif
> +
> +	rcu_assign_pointer(ci->table, tables->table);
> +	tables->table = NULL;
> +}
> +

You don't need to shuffle all the code for a simple bug fix, you can use
forward declaration if some functions are needed earlier.

...

>  	/*
>  	 * Back to atomic context. We might have migrated to a new CPU with a
> @@ -568,11 +621,19 @@ swap_cluster_populate(struct swap_info_struct *si,
>  		spin_lock(&si->global_cluster_lock);
>  	spin_lock(&ci->lock);
>  
> +	/* Nothing except this helper should populate an isolated cluster. */
> +	if (WARN_ON_ONCE(cluster_table_is_alloced(ci))) {
> +		swap_cluster_tables_free(&tables);
> +		return ci;
> +	}
> +
>  	if (ret) {
>  		move_cluster(si, ci, &si->free_clusters, CLUSTER_FLAG_FREE);
>  		spin_unlock(&ci->lock);
>  		return NULL;
>  	}
> +
> +	swap_cluster_tables_install(ci, &tables);

It seems the same fix with Kemeng's patch? Youngjun have notice this too.
https://lore.kernel.org/linux-mm/20260720071342.50742-2-shikemeng@huaweicloud.com/

And I think you missed some Cc, maybe you can try tools like b4 which
automatically generate the Cc list for you.

Hello Kemeng, can you help check and see if an updated can be sent?