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

[Kemeng Shi]

在 2026/7/20 17:11:54, 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;
> 
Hello, Youngjun

Thanks for feedback.> 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.
Yes, it will be better to handle the issue inside the swap_cluster_alloc_table()
and I also consider this before. However, I didn't find a way to properly handle
the lock as swap_cluster_alloc_table() will be used for both ATOMIC allocation
and sleep allocation. So what about introduce a new helper like:
swap_cluster_alloc_table_unlocked()
{
	table = kmem_cache_zalloc()
#ifdef CONFIG_MEMCG
	memcg_table = kzalloc_obj()
#endif
#if !SWAP_TABLE_HAS_ZEROFLAG
	zero_bitmap = bitmap_zalloc()
#endif

	spin_lock(&ci->lock);
	/* update all tables in atomic */
	spin_unlock(&ci->lock);
}
In this way, there are some repeated code, so it is also a little ugly...
I will appreciate if you have any better idea!
> 
> (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.)
Yes, I also think we should try not to introduce any cost in hot path.> 
> Your current approach is also a good direction, but please review this
> idea as well :)
> 
> Thanks,
> Youngjun