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

[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?

Youngjun