Re: [PATCH 1/4] mm, swap: Fix potential NULL dereference when trying a sleep table allocation

[Youngjun Park]

On Mon, Jul 20, 2026 at 03:13:39PM +0800, Kemeng Shi wrote:

Hello Kemeng Shi,

Good catch!

it indeed looks like a possible race condition 
(though I haven't verified it at runtime either).

> The root cause of this issue is because multi-tables are updated in non
> atomic context. To be more specific, the issue could be triggerred as
> following:

Here the cluster is isolated (CLUSTER_FLAG_NONE).

> swap_alloc_fast         swap_cluster_populate()
>                          /* Try a sleep allocation */
>                          spin_unlock(&ci->lock);

And from here, the table becomes visible,

>                          swap_cluster_alloc_table()
>                           rcu_assign_pointer(ci->table, table);

All entry on this cluster is freed(e.g process dead) , 
so this might be the free cluster.

>  ci = swap_cluster_lock(si, offset)
>  cluster_is_usable(ci, order）

Since it's a normal cluster (CLUSTER_FLAG_NONE) and the table exists,
it passes here...

>   if (!cluster_table_is_alloced(ci)) // ok 
>  alloc_swap_scan_cluster()
>   cluster_scan_range()
>    __swap_table_get()
> 
>                           /* free table when more table allocation fails */
>                           ci->memcg_table = kzalloc_obj(*ci->memcg_table,
>                                                         gfp);
>                           if (!ci->memcg_table)
>                            swap_cluster_free_table()

Nullified

>                             rcu_assign_pointer(ci->table, NULL);

Now it happens.

>     table = rcu_dereference_check(ci->table, lockdep_is_held(&ci->lock));
>      atomic_long_read(&table[off]); // NULL dereference
> 
> Fix the issue by updating allocated tables in atomic context.
> 
> Fixes: 2fe7a6f5024b8 ("mm/memcg, swap: store cgroup id in cluster table directly")
> Signed-off-by: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx>
> ---
>  mm/swapfile.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 615d90867111..d29062d9c3cd 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -490,6 +490,20 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
>  	return 0;
>  }
>  
> +static void swap_cluster_copy_table(struct swap_cluster_info *d_ci,
> +				    struct swap_cluster_info *s_ci)
> +{
> +	rcu_assign_pointer(d_ci->table, rcu_access_pointer(s_ci->table));
> +
> +#ifdef CONFIG_MEMCG
> +	d_ci->memcg_table = s_ci->memcg_table;
> +#endif
> +
> +#if !SWAP_TABLE_HAS_ZEROFLAG
> +	d_ci->zero_bitmap = s_ci->zero_bitmap;
> +#endif
> +}
> +
>  /*
>   * Sanity check to ensure nothing leaked, and the specified range is empty.
>   * One special case is that bad slots can't be freed, so check the number of
> @@ -527,6 +541,7 @@ static struct swap_cluster_info *
>  swap_cluster_populate(struct swap_info_struct *si,
>  			 struct swap_cluster_info *ci)
>  {
> +	struct swap_cluster_info tmp_ci;
>  	int ret;

IMHO, How about resolving everything inside swap_cluster_alloc_table() instead?

We could consider the following two things.

- Assign the table at the very end inside swap_cluster_alloc_table().
- Handle the table freeing properly if subsequent allocations fail.

Rather than allocating a temp_ci (which adds special handling
for this case), wouldn't it be better to maintain the original intention
of the swap_cluster_alloc_table() function?

Although this is not a hot path and table allocation will usually succeed with
the ATOMIC allocator, this approach would also save the memcpy overhead
and stack memory usage.

(Another option might be checking memcg_table and zero_bitmap inside
cluster_table_is_alloced(). However, I personally dislike this idea as
it might introduce side effects regarding its coverage.)

Your current approach is also a good direction, but please review this
idea as well :)

Thanks,
Youngjun