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

[Kemeng Shi]

在 2026/7/20 17:11:58, Kairui Song 写道:
> 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?
The commit 2fe7a6f5024b8 and b197d41462c2 both exist in this tree:
https://kernel.googlesource.com/pub/scm/linux/kernel/git/akpm/mm.git.
I search in another tree and only your commit exsits, so I believe
your commit is correct. Will fix this in next version. Thanks for
correction.

> 
>> 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)
> 
> 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?
As swap_cluster_alloc_table() grows from only table allocation to table,
memcg_table and zero_bitmap allocation. So I think maybe a helper may
inform us not to forget to do copy when new member is added. But I also
think about open-coding it. So both ways look good to me. I will open-code
it if you prefer it. :)> 
>> +{
>> +	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!