| 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
| ||
|
Message-ID: <CAJuCfpGhzzQwS8q0t3RWygT924uSFRJUxmb5pSMnLrjrRnPuSg@mail.gmail.com>
Date: Mon, 20 Oct 2025 11:52:16 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Hao Ge <hao.ge@...ux.dev>
Cc: Vlastimil Babka <vbabka@...e.cz>, Andrew Morton <akpm@...ux-foundation.org>,
Christoph Lameter <cl@...two.org>, David Rientjes <rientjes@...gle.com>,
Roman Gushchin <roman.gushchin@...ux.dev>, Harry Yoo <harry.yoo@...cle.com>,
Shakeel Butt <shakeel.butt@...ux.dev>, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Hao Ge <gehao@...inos.cn>
Subject: Re: [PATCH v2] slab: Avoid race on slab->obj_exts in alloc_slab_obj_exts
On Mon, Oct 20, 2025 at 7:31 AM Hao Ge <hao.ge@...ux.dev> 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.
>
> Conversely, in a race condition, if mark_failed_objexts_alloc wins the
> race, the other process (that previously succeeded in allocation) will
> lose the race. A null pointer dereference may occur in the following
> scenario:
>
> Thread1 Thead2
>
> alloc_slab_obj_exts alloc_slab_obj_exts
>
> old_exts = READ_ONCE(slab->obj_exts) = 0
>
> mark_failed_objexts_alloc(slab);
>
> cmpxchg(&slab->obj_exts, old_exts, new_exts) != old_exts
>
> kfree and return 0;
>
> alloc_tag_add -> a panic occurs.
I appreciate the time and effort you put in this description but it
sounds overly-complicated IMHO. IIUC in both cases the issue happens
when a valid slab->obj_exts pointer is overwritten by
OBJEXTS_ALLOC_FAIL. I would simply say:
If two competing threads enter alloc_slab_obj_exts() and one of them
fails to allocate the object extension vector, it might override the
valid slab->obj_exts allocated by the other thread with
OBJEXTS_ALLOC_FAIL. This will cause the thread that lost this race and
expects a valid pointer to dereference a NULL pointer later on.
>
> To fix this, introduce a retry mechanism for the cmpxchg() operation:
> 1. Add a 'retry' label at the point where READ_ONCE(slab->obj_exts) is
> invoked, ensuring the latest value is fetched during subsequent retries.
> 2. if cmpxchg() fails (indicating a concurrent update), jump back to
> "retry" to re-read old_exts and recheck the validity of the obj_exts
> allocated in this operation.
The paragraph above explains "what" you do but we can use the code to
understand that. Changelog should describe "why" not "what" you do. I
would just say:
Update slab->obj_exts atomically using cmpxchg() to avoid
slab->obj_exts overrides by racing threads.
>
> Thanks for Vlastimil and Suren's help with debugging.
>
> Fixes: f7381b911640 ("slab: mark slab->obj_exts allocation failures unconditionally")
> Suggested-by: Suren Baghdasaryan <surenb@...gle.com>
> Signed-off-by: Hao Ge <gehao@...inos.cn>
> ---
> v2: Incorporate handling for the scenario where, if mark_failed_objexts_alloc wins the race,
> the other process (that previously succeeded in allocation) will lose the race, based on Suren's suggestion.
> Add Suggested-by: Suren Baghdasaryan <surenb@...gle.com>
> ---
> mm/slub.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 2e4340c75be2..fd1b5dda3863 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);
> }
>
> static inline void handle_failed_objexts_alloc(unsigned long obj_exts,
> @@ -2136,6 +2136,7 @@ int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
> #ifdef CONFIG_MEMCG
> new_exts |= MEMCG_DATA_OBJEXTS;
> #endif
> +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,20 @@ 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) {
> + /*
> + * There are some abnormal scenarios caused by race conditions:
> + *
> + * Thread1 Thead2
> + * alloc_slab_obj_exts alloc_slab_obj_exts
> + * old_exts = READ_ONCE(slab->obj_exts) = 0
> + * mark_failed_objexts_alloc(slab);
> + * cmpxchg(&slab->obj_exts, old_exts, new_exts) != old_exts
> + *
> + * We should retry to ensure the validity of the slab_ext
> + * allocated in this operation.
> + */
I don't think we need a diagram here. The race is quite trivial. Maybe
a simple comment like this?
/* Retry if a racing thread changed slab->obj_exts from under us. */
> + goto retry;
> }
>
> if (allow_spin)
> --
> 2.25.1
>
Powered by blists - more mailing lists