Re: [PATCH v2] mm/slab: take n->list_lock in __slab_try_return_freelist() to avoid race

[Harry Yoo]

On Wed, Sep 02, 2026 at 06:57:53PM +0200, Vlastimil Babka (SUSE) wrote:
> On 9/2/26 18:41, Harry Yoo (Meta) wrote:
> > Commit ba7425312607 ("mm, slab: add an optimistic
> > __slab_try_return_freelist()") incorrectly assumed that nobody has freed
> > an object to the slab as long as slab->freelist is NULL and cmpxchg
> > succeeds.
> > 
> > However, as reported by Hyunwoo Kim [1], other CPUs might have freed
> > an object to the slab, insert the slab to the partial list, then
> > allocated an object from the slab, and be in the middle of removing
> > the slab from the list under n->list_lock.
> > 
> > Since __refill_objects_node() puts the slab back on pc.slabs
> > outside n->list_lock, it might insert the slab into that list while
> > the slab is concurrently being removed from n->partial.
> > This led to a list corruption, as reported by Hyunwoo Kim [1]:
> 
> "as reported by ..." was already said above, we could keep just the [1] or
> drop completely?

I didn't want to steal the credit for splat ;) but yeah keeping [1]
should be enough.

> >   list_add corruption. next->prev should be prev
> >   (ffff888100000248), but was dead000000000122.
> >   (next=ffffea000416e410).
> >   kernel BUG at lib/list_debug.c:29!
> >   Oops: invalid opcode: 0000 [#1] SMP NOPTI
> >   CPU: 1 UID: 65534 PID: 144 Comm: poc Not tainted
> >   7.2.0-16172-gcf72cbb39da8-dirty #1 PREEMPT(lazy)
> >   RIP: 0010:__list_add_valid_or_report+0x80/0xd0
> >   ...
> >   Call Trace:
> >    alloc_from_new_slab+0x183/0x300
> >    ___slab_alloc+0x31c/0x890
> >    __kmalloc_noprof+0x3d4/0x800
> >    lsm_blob_alloc+0x2d/0x50
> >    security_msg_msg_alloc+0x26/0x90
> >    load_msg+0x1aa/0x210
> >    do_msgsnd+0x91/0x800
> >    do_syscall_64+0x109/0x5d0
> >    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> >   ...
> >   Kernel panic - not syncing: Fatal exception
> > 
> > This is a classic ABA problem where cmpxchg succeeds but the state has
> > changed since __refill_objects_node() took the freelist from the slab.
> > 
> > As Vlastimil Babka mentioned [2], it should be rare to return more than
> > one slab (due to the racy read of slab->counters in
> > get_partial_node_bulk()). Therefore, instead of introducing additional
> > complexity, acquire and release n->list_lock twice in the worst case.
> > 
> > Return the slab directly to the partial list and hold n->list_lock
> > across the cmpxchg and add_partial(). This is similar to the initial
> > version of commit ba7425312607 [3]. This is enough to avoid the race as
> > the list manipulation is serialized by n->list_lock. While at it,
> > bring back unlikely() hint now that the condition is unlikely.
> 
> Ah nicely spotted.

Thanks ;-)

> > diff --git a/mm/slub.c b/mm/slub.c
> > index f9b56cb439e7..5cbbacb8ee32 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -5684,6 +5684,8 @@ static bool __slab_try_return_freelist(struct kmem_cache *s, struct slab *slab,
> >  				       void *head, int cnt)
> >  {
> >  	struct freelist_counters old, new;
> > +	struct kmem_cache_node *n;
> > +	unsigned long flags;
> >  
> >  	old.freelist = slab->freelist;
> >  	old.counters = slab->counters;
> > @@ -5695,9 +5697,16 @@ static bool __slab_try_return_freelist(struct kmem_cache *s, struct slab *slab,
> >  	new.counters = old.counters;
> >  	new.inuse -= cnt;
> >  
> > -	if (!slab_update_freelist(s, slab, &old, &new, "__slab_try_return_freelist"))
> > +	n = get_node(s, slab_nid(slab));
> 
> Wonder if we could just pass the 'n' to __slab_try_return_freelist() that we
> already have in the caller.

Sounds good, will do.

-- 
Cheers,
Harry / Hyeonggon