Re: [PATCH] slab: Avoid race on slab->obj_exts in alloc_slab_obj_exts
From: Hao Ge
Date: Sun Oct 19 2025 - 22:02:25 EST
On 2025/10/18 05:52, Suren Baghdasaryan wrote:
On Fri, Oct 17, 2025 at 3:40 AM Vlastimil Babka <vbabka@xxxxxxx> wrote:
On 10/17/25 12:02, Hao Ge wrote:
Yes.
On Oct 17, 2025, at 16:22, Vlastimil Babka <vbabka@xxxxxxx> wrote:Aha, sorry I misunderstood what you meant.
On 10/17/25 09:40, Harry Yoo wrote:
On Fri, Oct 17, 2025 at 02:42:56PM +0800, Hao Ge wrote:I don't think this makes sense.
Hi Harry
Thank you for your quick response.
On 2025/10/17 14:05, Harry Yoo wrote:
On Fri, Oct 17, 2025 at 12:57:49PM +0800, Hao Ge wrote:Oh sorry, I missed this.
From: Hao Ge <gehao@xxxxxxxxxx>I think we should add Cc: stable as well?
In the alloc_slab_obj_exts function, there is a race condition
between the successful allocation of slab->obj_exts and its
setting to OBJEXTS_ALLOC_FAIL due to allocation failure.
When two threads are both allocating objects from the same slab,
they both end up entering the alloc_slab_obj_exts function because
the slab has no obj_exts (allocated yet).
And One call succeeds in allocation, but the racing one overwrites
our obj_ext with OBJEXTS_ALLOC_FAIL. The threads that successfully
allocated will have prepare_slab_obj_exts_hook() return
slab_obj_exts(slab) + obj_to_index(s, slab, p), where slab_obj_exts(slab)
already sees OBJEXTS_ALLOC_FAIL and thus it returns an offset based
on the zero address.
And then it will call alloc_tag_add, where the member codetag_ref *ref
of obj_exts will be referenced.Thus, a NULL pointer dereference occurs,
leading to a panic.
In order to avoid that, for the case of allocation failure where
OBJEXTS_ALLOC_FAIL is assigned, we use cmpxchg to handle this assignment.
Thanks for Vlastimil and Suren's help with debugging.
Fixes: f7381b911640 ("slab: mark slab->obj_exts allocation failures unconditionally")
We need an explicit Cc: stable to backport mm patches to -stable.
Great point.Signed-off-by: Hao Ge <gehao@xxxxxxxxxx>A silly question:
---
mm/slub.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/slub.c b/mm/slub.c
index 2e4340c75be2..9e6361796e34 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2054,7 +2054,7 @@ static inline void mark_objexts_empty(struct slabobj_ext *obj_exts)
static inline void mark_failed_objexts_alloc(struct slab *slab)
{
- slab->obj_exts = OBJEXTS_ALLOC_FAIL;
+ cmpxchg(&slab->obj_exts, 0, OBJEXTS_ALLOC_FAIL);
}
If mark_failed_objexts_alloc() succeeds and a concurrent
alloc_slab_obj_exts() loses, should we retry cmpxchg() in
alloc_slab_obj_exts()?
We could modify it like this, perhaps?
static inline void mark_failed_objexts_alloc(struct slab *slab)
{
+ unsigned long old_exts = READ_ONCE(slab->obj_exts);
+ if( old_exts == 0 )
+ cmpxchg(&slab->obj_exts, 0, OBJEXTS_ALLOC_FAIL);
}
cmpxchg() fails anyway if old_exts != 0.
From my understanding, it only affected the obj_ext associated with this allocation, which was subsequently deallocated, leading to the loss of this count. Is this correct?Yeah, but either we make it a full loop or we don't care.Do you have any better suggestions on your end?I meant something like this.
But someone might argue that this is not necessary anyway
if there's a severe memory pressure :)
diff --git a/mm/slub.c b/mm/slub.c
index a585d0ac45d4..4354ae68b0e1 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2139,6 +2139,11 @@ int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
slab->obj_exts = new_exts;
} else if ((old_exts & ~OBJEXTS_FLAGS_MASK) ||
cmpxchg(&slab->obj_exts, old_exts, new_exts) != old_exts) {
+
+ old_exts = READ_ONCE(slab->obj_exts);
+ if (old_exts == OBJEXTS_ALLOC_FAIL &&
+ cmpxchg(&slab->obj_exts, old_exts, new_exts) == old_exts)
+ goto out;
Maybe we could care because even without a severe memory pressure, one side
might be using kmalloc_nolock() and fail more easily. I'd bet it's what's
making this reproducible actually.
In that case, we may really need to handle this situation and require a full loop.
In theory, this scenario could occur:
Thread1 Thead2
alloc_slab_obj_exts alloc_slab_obj_exts
old_exts = READ_ONCE(slab->obj_exts) = 0
mark_failed_objexts_alloc(slab);
cmpxchg(&slab->obj_exts, old_exts, new_exts) != old_exts
kfree and return 0;
alloc_tag_add---->a panic occurs
Alternatively, is there any code logic I might have overlooked?
I think retrying like this should work:
+retry:
old_exts = READ_ONCE(slab->obj_exts);
handle_failed_objexts_alloc(old_exts, vec, objects);
if (new_slab) {
@@ -2145,8 +2146,7 @@ int alloc_slab_obj_exts(struct slab *slab,
struct kmem_cache *s,
* be simply assigned.
*/
slab->obj_exts = new_exts;
- } else if ((old_exts & ~OBJEXTS_FLAGS_MASK) ||
- cmpxchg(&slab->obj_exts, old_exts, new_exts) != old_exts) {
+ } else if (old_exts & ~OBJEXTS_FLAGS_MASK) {
/*
* If the slab is already in use, somebody can allocate and
* assign slabobj_exts in parallel. In this case the existing
@@ -2158,6 +2158,8 @@ int alloc_slab_obj_exts(struct slab *slab,
struct kmem_cache *s,
else
kfree(vec);
return 0;
+ } else if (cmpxchg(&slab->obj_exts, old_exts, new_exts) != old_exts) {
+ goto retry;
}
Agree with this. If there are no issues with my comment above,
I will send V2 based on Suren's suggestion.
Additionally, I believe the "Fixes" field should be written as follows:
Fixes: 09c46563ff6d ("codetag: debug: introduce OBJEXTS_ALLOC_FAIL to mark failed slab_ext allocations")
Am I wrong?
/*
* If the slab is already in use, somebody can allocate and
* assign slabobj_exts in parallel. In this case the existing
@@ -2152,6 +2157,7 @@ int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
return 0;
}
+out:
kmemleak_not_leak(vec);
return 0;
}
--
2.25.1