* [PATCH] mm/slab: take n->list_lock for the list_add() in __refill_objects_node()
@ 2026-08-30 7:25 Hyunwoo Kim
2026-08-30 12:45 ` Harry Yoo
0 siblings, 1 reply; 3+ messages in thread
From: Hyunwoo Kim @ 2026-08-30 7:25 UTC (permalink / raw)
To: Vlastimil Babka, Harry Yoo, Andrew Morton
Cc: Hao Li, Christoph Lameter, David Rientjes, Roman Gushchin,
Suren Baghdasaryan, linux-mm, linux-kernel, imv4bel
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.
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.
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.
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;
@@ -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;
+ }
+ if (locked) {
list_for_each_entry(slab, &pc.slabs, slab_list)
set_node_partial_state(n, slab);
--
2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] mm/slab: take n->list_lock for the list_add() in __refill_objects_node()
2026-08-30 7:25 [PATCH] mm/slab: take n->list_lock for the list_add() in __refill_objects_node() Hyunwoo Kim
@ 2026-08-30 12:45 ` Harry Yoo
2026-08-30 14:35 ` Hao Li
0 siblings, 1 reply; 3+ messages in thread
From: Harry Yoo @ 2026-08-30 12:45 UTC (permalink / raw)
To: Hyunwoo Kim
Cc: Vlastimil Babka, Andrew Morton, Hao Li, Christoph Lameter,
David Rientjes, Roman Gushchin, Suren Baghdasaryan, linux-mm,
linux-kernel
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 :)
> 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);
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] mm/slab: take n->list_lock for the list_add() in __refill_objects_node()
2026-08-30 12:45 ` Harry Yoo
@ 2026-08-30 14:35 ` Hao Li
0 siblings, 0 replies; 3+ messages in thread
From: Hao Li @ 2026-08-30 14:35 UTC (permalink / raw)
To: Harry Yoo
Cc: Hyunwoo Kim, Vlastimil Babka, Andrew Morton, Christoph Lameter,
David Rientjes, Roman Gushchin, Suren Baghdasaryan, linux-mm,
linux-kernel
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 :)
>
> > 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...
>
> > 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
>
--
Thanks,
Hao
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-08-30 14:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-30 7:25 [PATCH] mm/slab: take n->list_lock for the list_add() in __refill_objects_node() Hyunwoo Kim
2026-08-30 12:45 ` Harry Yoo
2026-08-30 14:35 ` Hao Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®