Re: [PATCH] slab: Avoid race on slab->obj_exts in alloc_slab_obj_exts

[Harry Yoo <harry.yoo@oracle.com>]

On Fri, Oct 17, 2025 at 02:42:56PM +0800, Hao Ge wrote:
> 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:
> > > From: Hao Ge <gehao@kylinos.cn>
> > > 
> > > 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")
> > 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.
> > 
> > > Signed-off-by: Hao Ge <gehao@kylinos.cn>
> > > ---
> > >   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);
> > >   }
> > 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()?
> 
> 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);
>  }

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

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

-- 
Cheers,
Harry / Hyeonggon