linux-kernel - Re: [PATCH] slab: Avoid race on slab->obj_exts in alloc_slab_obj

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <8F4AE1E9-7412-40D6-B383-187021266174@linux.dev>
Date: Fri, 17 Oct 2025 18:02:57 +0800
From: Hao Ge <hao.ge@...ux.dev>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Harry Yoo <harry.yoo@...cle.com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Christoph Lameter <cl@...two.org>, David Rientjes <rientjes@...gle.com>,
 Roman Gushchin <roman.gushchin@...ux.dev>,
 Suren Baghdasaryan <surenb@...gle.com>,
 Shakeel Butt <shakeel.butt@...ux.dev>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, Hao Ge <gehao@...inos.cn>
Subject: Re: [PATCH] slab: Avoid race on slab->obj_exts in alloc_slab_obj_exts



> On Oct 17, 2025, at 16:22, Vlastimil Babka <vbabka@...e.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@...inos.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@...inos.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?


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