Re: [PATCH] mm/slab: take n->list_lock for the list_add() in __refill_objects_node()

[Hao Li <hao.li@linux.dev>]

On Mon, Aug 31, 2026 at 02:55:06PM +0200, Vlastimil Babka (SUSE) wrote:
> On 8/30/26 16:35, Hao Li wrote:
> > On Sun, Aug 30, 2026 at 12:45:17PM +0000, Harry Yoo wrote:
> >> On Sun, Aug 30, 2026 at 04:25:45PM +0900, Hyunwoo Kim wrote:
> >> > In __refill_objects_node(), the list_add(&slab->slab_list, &pc.slabs) that
> >> > follows a successful __slab_try_return_freelist() is done without
> >> > n->list_lock.
> >> >
> >> > __slab_try_return_freelist() only succeeds while slab->freelist is NULL. The
> >> > slab we are refilling from is taken off pc.slabs by the list_del() at the
> >> > top of the loop, so at that point it is on no list.
> >> > 
> >> > If another CPU frees an object of that slab, __slab_free() sees the slab as
> >> > full and puts it back on n->partial. If a third CPU then takes that object
> >> > in get_from_partial_node(), the freelist becomes NULL again.
> >> 
> >> Ouch. Good catch, Hyunwoo.
> >> A classic ABA problem :)
> 
> Ouch, indeed.
> 
> >> > A slab that sits on n->partial with a NULL freelist only exists while
> >> > get_from_partial_node() holds n->list_lock, between its cmpxchg and its
> >> > remove_partial().
> >> > 
> >> > A list_add() in that window overwrites slab_list to point into pc.slabs.
> >> > The list_del() in remove_partial() then follows the overwritten links, so it
> >> > unlinks the slab from pc.slabs and poisons slab_list while leaving the
> >> > n->partial side alone. n->partial is left pointing at the poisoned slab.
> >> > 
> >> >            CPU0                        CPU1                  CPU2
> >> > 
> >> >   __refill_objects_node()
> >> >     get_partial_node_bulk()   // n->partial to pc.slabs
> >> >     list_del()                // on no list now
> >> >     get_freelist_nofreeze()   // freelist = NULL
> >> >                               __slab_free()
> >> >                                 add_partial()
> >> >                                 // back on n->partial
> >> > 	                        // freelist is not NULL
> >> > 
> >> >                                                     get_from_partial_node()
> >> >                                                       lock
> >> >                                                       cmpxchg
> >> >                                                       // freelist = NULL
> >> >     __slab_try_return_freelist()
> >> >     list_add(&pc.slabs)       // overwrites slab_list
> >> >                                                       remove_partial()
> >> >                                                         list_del()
> >> >                                                         // off pc.slabs
> >> >                                                         // slab_list = POISON
> >> > 
> >> > panic log:
> >> > 
> >> >   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
> >> > 
> >> > Do the list_add() under n->list_lock. Reattaching the freelist stays outside
> >> > the lock. Once it succeeds the freelist is no longer NULL, so __slab_free()
> >> > cannot put the slab back, and by the time the lock is taken remove_partial()
> >> > has finished and the slab is on no list.
> >> 
> >> Yeah, this should work correctly. 
> >> 
> >> > The lock is held until the block below that returns the remaining slabs to
> >> > the partial list. That block already took the same lock on this path, so no
> >> > lock/unlock pair is added.
> >> > 
> >> > The unlock is keyed on having taken the lock instead of on pc.slabs being
> >> > empty. With CONFIG_DEBUG_LIST or CONFIG_LIST_HARDENED, __list_add() returns
> >> > without linking anything if its check fails, which would leave pc.slabs
> >> > empty.
> >> 
> >> Well, if the check fails, it has a bug and should be fixed.
> >> We should not make the code less readable to handle a bug.
> >> 
> >> I think it's better to have (diff on top of the patch, not tested):
> >> 
> >> diff --git a/mm/slub.c b/mm/slub.c
> >> index 7a7f9935c711..eff96b992164 100644
> >> --- a/mm/slub.c
> >> +++ b/mm/slub.c
> >> @@ -7216,12 +7216,10 @@ __refill_objects_node(struct kmem_cache *s, void **p, gfp_t gfp, unsigned int mi
> >>  			break;
> >>  	}
> >>  
> >> -	if (!locked && !list_empty(&pc.slabs)) {
> >> -		spin_lock_irqsave(&n->list_lock, flags);
> >> -		locked = true;
> >> -	}
> >> +	if (!list_empty(&pc.slabs)) {
> >> +		if (!locked)
> >> +			spin_lock_irqsave(&n->list_lock, flags);
> >>  
> >> -	if (locked) {
> >>  		list_for_each_entry(slab, &pc.slabs, slab_list)
> >>  			set_node_partial_state(n, slab);
> > 
> > Since introducing a new variable seems unavoidable, what if we temporarily
> > stash this slab in a pointer like below, and then add it to pc.slabs once we
> > acquire the lock.
> > 
> > struct slab *leftover_slab = NULL;
> > 
> > ...
> > ...
> > 	if (__slab_try_return_freelist(s, slab, head, count)) {
> > 		leftover_slab = slab;
> > 		break;
> > 	}
> > 
> > ...
> > ...
> > if (!list_empty(&pc.slabs)) {
> > 	spin_lock_irqsave(&n->list_lock, flags);
> > 
> > 	if (leftover_slab)
> > 		list_add(&leftover_slab->slab_list, &pc.slabs);
> > 	...
> > 	...
> > }
> > 
> > PS: If I recall correctly, Vlastimil's initial patch was actually fine. It was
> > my suggestion to save an extra lock/unlock pair that accidentally led to this
> > trap...
> 
> Ah, thanks for the reminder. This [1] was the original attempt.
> 
> [1]
> https://lore.kernel.org/all/20260421-b4-refill-optimistic-return-v1-1-24f0bfc1acff@kernel.org/
> 
> I wonder if the fix should be to return to that approach and just have
> __slab_try_return_freelist() handle the list_lock. The code would be simpler
> with not "bool locked".
> 
> It should be really very rare that we would end up returning a partial list
> and also have additional slabs to return on pc.slabs? So I think there would
> be no noticeable performance downside to the simpler code potentially ending
> up taking the list_lock twice instead of once.

Yes, make sense, in fact, when I first looked at this patch, I did consider
that maybe the original locking approach was best for readability and safety :)

> 
> >> 
> >> > Fixes: ba7425312607 ("mm, slab: add an optimistic __slab_try_return_freelist()")
> >> > Cc: stable@vger.kernel.org
> >> > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> >> > ---
> >> >  mm/slub.c | 9 ++++++++-
> >> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >> > 
> >> > diff --git a/mm/slub.c b/mm/slub.c
> >> > index f9b56cb439e709..4f6d1a03a8ee46 100644
> >> > --- a/mm/slub.c
> >> > +++ b/mm/slub.c
> >> > @@ -7260,6 +7260,7 @@ __refill_objects_node(struct kmem_cache *s, void **p, gfp_t gfp, unsigned int mi
> >> >  	struct slab *slab, *slab2;
> >> >  	unsigned int refilled = 0;
> >> >  	unsigned long flags;
> >> > +	bool locked = false;
> >> >  	void *object;
> >> >  
> >> >  	pc.flags = gfp;
> >> 
> >> uh, I'm not a big fan of having a new variable to store 'locked' state,
> >> but okay, this seems unavoidable with current implementation.
> >> 
> >> > @@ -7297,7 +7298,10 @@ __refill_objects_node(struct kmem_cache *s, void **p, gfp_t gfp, unsigned int mi
> >> >  			void *tail;
> >> >  
> >> >  			if (__slab_try_return_freelist(s, slab, head, count)) {
> >> > +				/* get_from_partial_node() may be mid-removal of the slab */
> >> > +				spin_lock_irqsave(&n->list_lock, flags);
> >> >  				list_add(&slab->slab_list, &pc.slabs);
> >> > +				locked = true;
> >> >  				break;
> >> >  			}
> >> >  
> >> > @@ -7312,9 +7316,12 @@ __refill_objects_node(struct kmem_cache *s, void **p, gfp_t gfp, unsigned int mi
> >> >  			break;
> >> >  	}
> >> >  
> >> > -	if (!list_empty(&pc.slabs)) {
> >> > +	if (!locked && !list_empty(&pc.slabs)) {
> >> >  		spin_lock_irqsave(&n->list_lock, flags);
> >> > +		locked = true;
> >> > +	}
> >>  
> >> -- 
> >> Cheers,
> >> Harry / Hyeonggon
> >>  
> > 
>