slab: Avoid race on slab->obj_exts in alloc_slab_obj_exts

PATCH Superseded Hao Ge 4 participants 10 messages Oct 17, 2025 – Oct 20, 2025

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Hao Ge

author

2025-10-17 04:58 UTC #0 lore raw

PATCH 1/1 slab: Avoid race on slab->obj_exts in alloc_slab_obj_exts

From: Hao Ge <[email protected]>

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")

Signed-off-by Hao Ge <[email protected]>

---
 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/slub.c +1 -1

index 2e4340c75be2..9e6361796e34 100644
@@ -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);
 }
 
 static inline void handle_failed_objexts_alloc(unsigned long obj_exts,

Signature · 1 line

2.25.1

Harry Yoo

2025-10-17 06:05 UTC #1 lore raw

Quoted 21 lines · Hao Ge

From: Hao Ge <[email protected]>
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.

Quoted 6 lines

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")

I think we should add Cc: stable as well?
We need an explicit Cc: stable to backport mm patches to -stable.

Quoted 10 lines

Signed-off-by Hao Ge <[email protected]>
---
 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/slub.c +0 -0
index 2e4340c75be2..9e6361796e34 100644

mm/slub.c static inline void mark_objexts_empty(struct slabobj_ext *obj_exts) · line 2058

Quoted 6 lines

mm/slub.c +1 -1

@@ -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);
 }

A silly question:

If mark_failed_objexts_alloc() succeeds and a concurrent alloc_slab_obj_exts() loses, should we retry cmpxchg() in alloc_slab_obj_exts()?

Quoted 2 lines

Signature · 1 line
2.25.1

Signature · 2 lines

Cheers,
Harry / Hyeonggon

Hao Ge

author

2025-10-17 06:43 UTC #2 lore raw

Hi Harry

Thank you for your quick response.

Quoted 22 lines · Harry Yoo

Quoted 21 lines · Hao Ge
From: Hao Ge <[email protected]>
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.

Quoted 8 lines

Quoted 6 lines
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")
I think we should add Cc: stable as well?
We need an explicit Cc: stable to backport mm patches to -stable.

Oh sorry, I missed this.

Quoted 19 lines

Quoted 11 lines
Signed-off-by Hao Ge <[email protected]>
---
  mm/slub.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
mm/slub.c +0 -0
index 2e4340c75be2..9e6361796e34 100644
mm/slub.c static inline void mark_objexts_empty(struct slabobj_ext *obj_exts) · line 2058
Quoted 6 lines
mm/slub.c +1 -1
@@ -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);
  }
A silly question:

Quoted 3 lines

If mark_failed_objexts_alloc() succeeds and a concurrent alloc_slab_obj_exts() loses, should we retry cmpxchg() in alloc_slab_obj_exts()?

Great point.

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); }

Do you have any better suggestions on your end?

Quoted 3 lines

Quoted 3 lines
Signature · 2 lines
-- 
2.25.1

Harry Yoo

2025-10-17 07:40 UTC #3 lore raw

Quoted 62 lines · Hao Ge

Hi Harry
Thank you for your quick response.
Quoted 22 lines · Harry Yoo
Quoted 21 lines · Hao Ge
From: Hao Ge <[email protected]>
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.
Quoted 8 lines
Quoted 6 lines
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")
I think we should add Cc: stable as well?
We need an explicit Cc: stable to backport mm patches to -stable.
Oh sorry, I missed this.
Quoted 18 lines
Quoted 11 lines
Signed-off-by Hao Ge <[email protected]>
---
  mm/slub.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
mm/slub.c +0 -0
index 2e4340c75be2..9e6361796e34 100644
mm/slub.c static inline void mark_objexts_empty(struct slabobj_ext *obj_exts) · line 2057
Quoted 5 lines
mm/slub.c +1 -1
@@ -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);
  }
A silly question:
Quoted 3 lines
If mark_failed_objexts_alloc() succeeds and a concurrent alloc_slab_obj_exts() loses, should we retry cmpxchg() in alloc_slab_obj_exts()?
Great point.

Quoted 8 lines

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); }

I don't think this makes sense.
cmpxchg() fails anyway if old_exts != 0.

Quoted 1 line

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 :)

mm/slub.c +6 -0

index a585d0ac45d4..4354ae68b0e1 100644
@@ -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;
 		/*
 		 * 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;
 }

Quoted 4 lines

Quoted 3 lines
Quoted 3 lines
Signature · 2 lines
-- 
2.25.1

Signature · 2 lines

Cheers,
Harry / Hyeonggon

Vlastimil Babka

2025-10-17 08:21 UTC #4 lore raw

Quoted 87 lines · Harry Yoo

Quoted 62 lines · Hao Ge
Hi Harry
Thank you for your quick response.
Quoted 22 lines · Harry Yoo
Quoted 21 lines · Hao Ge
From: Hao Ge <[email protected]>
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.
Quoted 8 lines
Quoted 6 lines
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")
I think we should add Cc: stable as well?
We need an explicit Cc: stable to backport mm patches to -stable.
Oh sorry, I missed this.
Quoted 18 lines
Quoted 11 lines
Signed-off-by Hao Ge <[email protected]>
---
  mm/slub.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
mm/slub.c +0 -0
index 2e4340c75be2..9e6361796e34 100644
mm/slub.c static inline void mark_objexts_empty(struct slabobj_ext *obj_exts) · line 2057
Quoted 5 lines
mm/slub.c +1 -1
@@ -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);
  }
A silly question:
Quoted 3 lines
If mark_failed_objexts_alloc() succeeds and a concurrent alloc_slab_obj_exts() loses, should we retry cmpxchg() in alloc_slab_obj_exts()?
Great point.
Quoted 8 lines
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); }
I don't think this makes sense.
cmpxchg() fails anyway if old_exts != 0.
Quoted 1 line
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 :)
mm/slub.c +0 -0
index a585d0ac45d4..4354ae68b0e1 100644

mm/slub.c int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s, · line 2146

Quoted 8 lines

mm/slub.c +5 -0

@@ -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;

Yeah, but either we make it a full loop or we don't care.
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.

Quoted 17 lines

mm/slub.c +1 -0

 		/*
 		 * 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;
 }

Quoted 5 lines

Quoted 4 lines
Quoted 4 lines
Signature · 2 lines
-- 
2.25.1

Hao Ge

author

2025-10-17 10:03 UTC #5 lore raw

Quoted 77 lines · Vlastimil Babka

Quoted 72 lines · Harry Yoo
Quoted 71 lines · Hao Ge
Hi Harry
Thank you for your quick response.
Quoted 22 lines · Harry Yoo
Quoted 21 lines · Hao Ge
From: Hao Ge <[email protected]>
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.
Quoted 8 lines
Quoted 6 lines
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")
I think we should add Cc: stable as well?
We need an explicit Cc: stable to backport mm patches to -stable.
Oh sorry, I missed this.
Quoted 18 lines
Quoted 11 lines
Signed-off-by Hao Ge <[email protected]>
---
 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/slub.c +0 -0
index 2e4340c75be2..9e6361796e34 100644
mm/slub.c static inline void mark_objexts_empty(struct slabobj_ext *obj_exts) · line 2057
Quoted 5 lines
mm/slub.c +1 -1
@@ -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);
 }
A silly question:
Quoted 3 lines
If mark_failed_objexts_alloc() succeeds and a concurrent alloc_slab_obj_exts() loses, should we retry cmpxchg() in alloc_slab_obj_exts()?
Great point.
We could modify it like this, perhaps?
static inline void mark_failed_objexts_alloc(struct slab *slab) {
(quoted diff) +3 -0
+       unsigned long old_exts = READ_ONCE(slab->obj_exts);
+       if( old_exts == 0 )
+               cmpxchg(&slab->obj_exts, 0, OBJEXTS_ALLOC_FAIL);
 }

Quoted 2 lines

Quoted 2 lines
I don't think this makes sense.
cmpxchg() fails anyway if old_exts != 0.

Aha, sorry I misunderstood what you meant.

Quoted 18 lines

Quoted 18 lines
Quoted 2 lines
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 :)
mm/slub.c +2 -0
index a585d0ac45d4..4354ae68b0e1 100644
@@ -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);

Quoted 8 lines

mm/slub.c int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s, · line 2146
Quoted 3 lines
mm/slub.c +3 -0
+        if (old_exts == OBJEXTS_ALLOC_FAIL &&
+            cmpxchg(&slab->obj_exts, old_exts, new_exts) == old_exts)
+            goto out;
Yeah, but either we make it a full loop or we don't care.
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.

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?

Quoted 19 lines

Quoted 18 lines

mm/slub.c +1 -0

       /*
        * 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;
 }

Quoted 5 lines

Quoted 4 lines
Quoted 4 lines
mm/slub.c +0 -1
--
2.25.1

Vlastimil Babka

2025-10-17 10:40 UTC #6 lore raw

Quoted 109 lines · Hao Ge

Quoted 77 lines · Vlastimil Babka
Quoted 72 lines · Harry Yoo
Quoted 71 lines · Hao Ge
Hi Harry
Thank you for your quick response.
Quoted 22 lines · Harry Yoo
Quoted 21 lines · Hao Ge
From: Hao Ge <[email protected]>
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.
Quoted 8 lines
Quoted 6 lines
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")
I think we should add Cc: stable as well?
We need an explicit Cc: stable to backport mm patches to -stable.
Oh sorry, I missed this.
Quoted 18 lines
Quoted 11 lines
Signed-off-by Hao Ge <[email protected]>
---
 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/slub.c +0 -0
index 2e4340c75be2..9e6361796e34 100644
mm/slub.c static inline void mark_objexts_empty(struct slabobj_ext *obj_exts) · line 2057
Quoted 5 lines
mm/slub.c +1 -1
@@ -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);
 }
A silly question:
Quoted 3 lines
If mark_failed_objexts_alloc() succeeds and a concurrent alloc_slab_obj_exts() loses, should we retry cmpxchg() in alloc_slab_obj_exts()?
Great point.
We could modify it like this, perhaps?
static inline void mark_failed_objexts_alloc(struct slab *slab) {
(quoted diff) +3 -0
+       unsigned long old_exts = READ_ONCE(slab->obj_exts);
+       if( old_exts == 0 )
+               cmpxchg(&slab->obj_exts, 0, OBJEXTS_ALLOC_FAIL);
 }
Quoted 2 lines
Quoted 2 lines
I don't think this makes sense.
cmpxchg() fails anyway if old_exts != 0.
Aha, sorry I misunderstood what you meant.
Quoted 18 lines
Quoted 18 lines
Quoted 2 lines
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 :)
mm/slub.c +2 -0
index a585d0ac45d4..4354ae68b0e1 100644
@@ -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);
Quoted 8 lines
mm/slub.c int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s, · line 2146
Quoted 3 lines
mm/slub.c +3 -0
+        if (old_exts == OBJEXTS_ALLOC_FAIL &&
+            cmpxchg(&slab->obj_exts, old_exts, new_exts) == old_exts)
+            goto out;
Yeah, but either we make it a full loop or we don't care.
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.

Quoted 1 line

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?

Yes.

Quoted 20 lines

Quoted 19 lines

mm/slub.c +1 -0

       /*
        * 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;
 }

Quoted 5 lines

Quoted 4 lines
Quoted 4 lines
mm/slub.c +0 -1
--
2.25.1

Suren Baghdasaryan

2025-10-17 21:53 UTC #7 lore raw

Quoted 111 lines · Vlastimil Babka

Quoted 109 lines · Hao Ge
Quoted 77 lines · Vlastimil Babka
Quoted 72 lines · Harry Yoo
Quoted 71 lines · Hao Ge
Hi Harry
Thank you for your quick response.
Quoted 22 lines · Harry Yoo
Quoted 21 lines · Hao Ge
From: Hao Ge <[email protected]>
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.
Quoted 8 lines
Quoted 6 lines
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")
I think we should add Cc: stable as well?
We need an explicit Cc: stable to backport mm patches to -stable.
Oh sorry, I missed this.
Quoted 18 lines
Quoted 11 lines
Signed-off-by Hao Ge <[email protected]>
---
 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/slub.c +0 -0
index 2e4340c75be2..9e6361796e34 100644
mm/slub.c static inline void mark_objexts_empty(struct slabobj_ext *obj_exts) · line 2057
Quoted 5 lines
mm/slub.c +1 -1
@@ -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);
 }
A silly question:
Quoted 3 lines
If mark_failed_objexts_alloc() succeeds and a concurrent alloc_slab_obj_exts() loses, should we retry cmpxchg() in alloc_slab_obj_exts()?
Great point.
We could modify it like this, perhaps?
static inline void mark_failed_objexts_alloc(struct slab *slab) {
(quoted diff) +3 -0
+       unsigned long old_exts = READ_ONCE(slab->obj_exts);
+       if( old_exts == 0 )
+               cmpxchg(&slab->obj_exts, 0, OBJEXTS_ALLOC_FAIL);
 }
Quoted 2 lines
Quoted 2 lines
I don't think this makes sense.
cmpxchg() fails anyway if old_exts != 0.
Aha, sorry I misunderstood what you meant.
Quoted 26 lines
Quoted 13 lines
Quoted 2 lines
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 :)
mm/slub.c +0 -0
index a585d0ac45d4..4354ae68b0e1 100644
mm/slub.c int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s, · line 2146
Quoted 8 lines
mm/slub.c +5 -0
@@ -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;
Yeah, but either we make it a full loop or we don't care.
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.

Quoted 3 lines

Quoted 1 line
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?
Yes.

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;
         }

Quoted 22 lines

Quoted 21 lines

Quoted 20 lines

mm/slub.c +1 -0

       /*
        * 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;
 }

Quoted 5 lines

Quoted 4 lines
Quoted 4 lines
mm/slub.c +0 -1
--
2.25.1

Hao Ge

author

2025-10-20 02:02 UTC #8 lore raw

Quoted 102 lines · Suren Baghdasaryan

Quoted 101 lines · Vlastimil Babka
Quoted 100 lines · Hao Ge
Quoted 67 lines · Vlastimil Babka
Quoted 63 lines · Harry Yoo
Quoted 63 lines · Hao Ge
Hi Harry
Thank you for your quick response.
Quoted 22 lines · Harry Yoo
Quoted 21 lines · Hao Ge
From: Hao Ge <[email protected]>
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.
Quoted 8 lines
Quoted 6 lines
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")
I think we should add Cc: stable as well?
We need an explicit Cc: stable to backport mm patches to -stable.
Oh sorry, I missed this.
Quoted 17 lines
Quoted 10 lines
Signed-off-by Hao Ge <[email protected]>
---
  mm/slub.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
mm/slub.c +0 -0
index 2e4340c75be2..9e6361796e34 100644
mm/slub.c static inline void mark_objexts_empty(struct slabobj_ext *obj_exts) · line 2057
Quoted 5 lines
mm/slub.c +1 -1
@@ -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);
  }
A silly question:
Quoted 3 lines
If mark_failed_objexts_alloc() succeeds and a concurrent alloc_slab_obj_exts() loses, should we retry cmpxchg() in alloc_slab_obj_exts()?
Great point.
We could modify it like this, perhaps?
Quoted 8 lines
Quoted 8 lines
Review
Quoted 6 lines
  static inline void mark_failed_objexts_alloc(struct slab *slab)
  {
(quoted diff) +3 -0
+       unsigned long old_exts = READ_ONCE(slab->obj_exts);
+       if( old_exts == 0 )
+               cmpxchg(&slab->obj_exts, 0, OBJEXTS_ALLOC_FAIL);
  }
I don't think this makes sense.
cmpxchg() fails anyway if old_exts != 0.
Aha, sorry I misunderstood what you meant.
Quoted 23 lines
Quoted 11 lines
Quoted 1 line
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 :)
mm/slub.c +0 -0
index a585d0ac45d4..4354ae68b0e1 100644
mm/slub.c int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s, · line 2146
Quoted 8 lines
mm/slub.c +5 -0
@@ -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;
Yeah, but either we make it a full loop or we don't care.
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.

Quoted 2 lines

Quoted 2 lines
Quoted 1 line
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?
Yes.

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?

Quoted 18 lines

I think retrying like this should work:

(quoted diff) +2 -2

+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

Review int alloc_slab_obj_exts(struct slab *slab, · line 2164

Quoted 7 lines

(quoted diff) +2 -0

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

Quoted 15 lines

mm/slub.c +1 -0

        /*
         * 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;
 }

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
mm/slub.c +0 -1
--
2.25.1

Vlastimil Babka

2025-10-20 10:21 UTC #9 lore raw

Quoted 29 lines · Hao Ge

Quoted 19 lines

I think retrying like this should work:

(quoted diff) +2 -2

+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

Review int alloc_slab_obj_exts(struct slab *slab, · line 2164

Quoted 7 lines

(quoted diff) +2 -0

@@ -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,

Quoted 1 line

I will send V2 based on Suren's suggestion.

Great.

Quoted 6 lines

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?

I think it was safe before this one:

f7381b911640 ("slab: mark slab->obj_exts allocation failures unconditionally")