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

[Kemeng Shi]

在 2026/7/21 10:15:40, Youngjun Park 写道:
> On Tue, Jul 21, 2026 at 09:49:53AM +0800, Kemeng Shi wrote:
>> 在 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!
> 
> Hello!
> 
> I don't quite understand why we need to introduce a new helper function.
> Is it because the table assignment must be done strictly inside the lock?
> In successful cases, wouldn't it be fine to assign the table without holding
> the lock? (Please let me know if I am missing something here.)
> 
> If we just add error handling to free the allocated table inside
> swap_cluster_alloc_table(), swap_cluster_free_table() will properly free
> memcg_table and zero_bitmap as long as ci->table is not assigned yet.
> I don't think this will cause any issues.
> 
> My intention is something like this:
> 
> -       rcu_assign_pointer(ci->table, table);     /* Remove this */
> ...
> 
> #ifdef CONFIG_MEMCG
>         if (!mem_cgroup_disabled()) {
>                 VM_WARN_ON_ONCE(ci->memcg_table);
>                 ci->memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
>                 if (!ci->memcg_table) {
> 			+ free table
>                         swap_cluster_free_table(ci);
>                         return -ENOMEM;
>                 }
>         }
> #endif
> 
> #if !SWAP_TABLE_HAS_ZEROFLAG
>         VM_WARN_ON_ONCE(ci->zero_bitmap);
>         ci->zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
>         if (!ci->zero_bitmap) {
>                 + free table
>                 swap_cluster_free_table(ci);
>                 return -ENOMEM;
>         }
> #endif
> 
> ...
>         /* Assign here! */
>         rcu_assign_pointer(ci->table, table);
> 
> 	return
> }
> If it becomes visible at the very end, wouldn't it be safe from any issues?
Sorry, I misunderstand what you mean. I think this is a better direction!
As we rely on memory ordering, I'm checking if we need add memory barrier
betwwen access ci->table and zero_bitmap or betwwen ci->table and memcg_table.

On write side:
rcu_assign_pointer(ci->table, table) will offer release to ensure
zero_bitmap and memcg_table visible before ci->table.

On read side:
folio_alloc_swap
 swap_alloc_fast/swap_alloc_slow
  /* access ci->table under cluster lock */
  swap_cluster_lock
  cluster_is_usable
  ...
  __swap_table_set
  ...
  swap_cluster_unlock
  
 mem_cgroup_try_charge_swap
  ...
  /* cluster lock will ensure order betwwen ci->table and memcg_table */
  swap_cluster_get_and_lock
  __swap_cgroup_set
  swap_cluster_unlock

swap_writeout
 swap_zeromap_folio_set
  /* cluster lock will ensure order betwwen ci->table and zero_bitmap */
  swap_cluster_get_and_lock
  __swap_table_set_zero
  swap_cluster_unlock

So I think we can fix the issue in this way and I will do it in the next
version. Thanks for this great idea.

Kemeng Shi