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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJuCfpEWGujJe3JOjmiKLOUr49Hw_3smT6iatY7kaRBPPCWpNg@mail.gmail.com>
Date: Fri, 17 Oct 2025 14:52:58 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Hao Ge <hao.ge@...ux.dev>, 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>, 
	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 Fri, Oct 17, 2025 at 3:40 AM Vlastimil Babka <vbabka@...e.cz> wrote:
>
> On 10/17/25 12:02, Hao Ge wrote:
> >
> >
> >> 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?
>
> 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;
         }

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