Re: SLAB_NO_CMPXCHG was:: [PATCH 6/6] slab: Introduce kmalloc_nolock() and kfree_nolock().

[Harry Yoo <harry.yoo@oracle.com>]

On Tue, Jun 24, 2025 at 10:13:49AM -0700, Alexei Starovoitov wrote:
> On Thu, May 8, 2025 at 6:03 PM Harry Yoo <harry.yoo@oracle.com> wrote:
> > > +     s = kmalloc_slab(size, NULL, alloc_gfp, _RET_IP_);
> > > +
> > > +     if (!(s->flags & __CMPXCHG_DOUBLE))
> > > +             /*
> > > +              * kmalloc_nolock() is not supported on architectures that
> > > +              * don't implement cmpxchg16b.
> > > +              */
> > > +             return NULL;
> >
> > Hmm when someone uses slab debugging flags (e.g., passing boot
> > parameter slab_debug=FPZ as a hardening option on production [1], or
> > just for debugging), __CMPXCHG_DOUBLE is not set even when the arch
> > supports it.
> >
> > Is it okay to fail all kmalloc_nolock() calls in such cases?
> 
> I studied the code and the git history.
> Looks like slub doesn't have to disable cmpxchg mode when slab_debug is on.

A slight correction; Debug caches do not use cmpxchg mode at all by
design. If a future change enables cmpxchg mode for them, it will cause
the same consistency issue.

> The commit 41bec7c33f37 ("mm/slub: remove slab_lock() usage for debug
> operations")
> removed slab_lock from debug validation checks.

An excellent point!

Yes, SLUB does not maintain cpu slab and percpu partial slabs on
debug caches. Alloc/free is done under n->list_lock, so no need for
cmpxchg double and slab_lock() at all :)

> So right now slab_lock() only serializes slab->freelist/counter update.
> It's still necessary on arch-s that don't have cmpxchg, but that's it.
> Only __update_freelist_slow() is using it.

Yes.

> The comment next to SLAB_NO_CMPXCHG is obsolete as well.
> It's been there since days that slab_lock() was taken during
> consistency checks.

Yes.

> I think the following diff is appropriate:
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 044e43ee3373..9d615cfd1b6f 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -286,14 +286,6 @@ static inline bool
> kmem_cache_has_cpu_partial(struct kmem_cache *s)
>  #define DEBUG_DEFAULT_FLAGS (SLAB_CONSISTENCY_CHECKS | SLAB_RED_ZONE | \
>                                 SLAB_POISON | SLAB_STORE_USER)
> 
> -/*
> - * These debug flags cannot use CMPXCHG because there might be consistency
> - * issues when checking or reading debug information
> - */
> -#define SLAB_NO_CMPXCHG (SLAB_CONSISTENCY_CHECKS | SLAB_STORE_USER | \
> -                               SLAB_TRACE)
> -
> -
>
>  /*
>   * Debugging flags that require metadata to be stored in the slab.  These get
>   * disabled when slab_debug=O is used and a cache's min order increases with
> @@ -6654,7 +6646,7 @@ int do_kmem_cache_create(struct kmem_cache *s,
> const char *name,
>         }
> 
>  #ifdef system_has_freelist_aba
> -       if (system_has_freelist_aba() && !(s->flags & SLAB_NO_CMPXCHG)) {
> +       if (system_has_freelist_aba()) {
>                 /* Enable fast mode */
>                 s->flags |= __CMPXCHG_DOUBLE;
>         }
> 
> It survived my stress tests.
> Thoughts?

Perhaps it's better to change the condition in
kmalloc_nolock_noprof() from;

    if (!(s->flags & __CMPXCHG_DOUBLE))
        return NULL;

to;

    if (!(s->flags & __CMPXCHG_DOUBLE) && !kmem_cache_debug(s))
        return NULL;

Because debug caches do not use cmpxchg double (and that's why
it survived your test), it is not accurate to set __CMPXCHG_DOUBLE.

And it'll get into trouble anyway if debug caches use cmpxchg double.

-- 
Cheers,
Harry / Hyeonggon