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

[Vlastimil Babka <vbabka@suse.cz>]

On 10/17/25 12:02, Hao Ge wrote:
> 
> 
>> On Oct 17, 2025, at 16:22, Vlastimil Babka <vbabka@suse.cz> wrote:
>> 
>> 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.
> 
> Aha, sorry I misunderstood what you meant.
> 
>>> 
>>>> 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.
> 
> 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.

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