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

[Vlastimil Babka <vbabka@suse.cz>]

On 10/17/25 09:40, Harry Yoo wrote:
> 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;

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.

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