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

[Kairui Song <ryncsn@gmail.com>]

On Mon, Jul 20, 2026 at 03:13:39PM +0800, Kemeng Shi wrote:
> 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:
> 
> swap_alloc_fast         swap_cluster_populate()
>                          /* Try a sleep allocation */
>                          spin_unlock(&ci->lock);
>                          swap_cluster_alloc_table()
>                           rcu_assign_pointer(ci->table, table);
> 
>  ci = swap_cluster_lock(si, offset)
>  cluster_is_usable(ci, order）
>   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()
>                             rcu_assign_pointer(ci->table, NULL);
> 
>     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.

Thanks, that's a really tricky one, good catch.

> 
> Fixes: 2fe7a6f5024b8 ("mm/memcg, swap: store cgroup id in cluster table directly")

This commit doesn't exist, you mean b197d41462c2 right?

> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  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)

Nit: The name is a bit misleading, it not copying the table content,
just filled the d_ci with s_ci. And do we really need a helper for this?

> +{
> +	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;
>  
>  	/*
> @@ -552,7 +567,8 @@ swap_cluster_populate(struct swap_info_struct *si,
>  		spin_unlock(&si->global_cluster_lock);
>  	local_unlock(&percpu_swap_cluster.lock);
>  
> -	ret = swap_cluster_alloc_table(ci, __GFP_HIGH | __GFP_NOMEMALLOC |
> +	tmp_ci = *ci;
> +	ret = swap_cluster_alloc_table(&tmp_ci, __GFP_HIGH | __GFP_NOMEMALLOC |
>  					   GFP_KERNEL);

This allocation could be largely simplified with mempool, the reason I didn't
use that is due to lack of RCU support, which can be done later to clean
this up.

Please fix the Fixes tags, other parts are mostly fine, thanks agian!