Re: [PATCH slab/for-next-fixes v3 4/4] mm/slab: prevent unbounded recursion in free path with new kmalloc type

[Suren Baghdasaryan]

On Tue, Jul 14, 2026 at 2:12 AM Vlastimil Babka (SUSE)
<vbabka@xxxxxxxxxx> wrote:
>
> On 7/14/26 07:17, Harry Yoo wrote:
> >
> > On 7/14/26 2:08 AM, Suren Baghdasaryan wrote:
> >> On Mon, Jul 13, 2026 at 7:29 AM Harry Yoo (Oracle) <harry@xxxxxxxxxx> wrote:
> >>> @@ -386,12 +387,17 @@ static inline unsigned int size_index_elem(unsigned int bytes)
> >>>   * KMALLOC_MAX_CACHE_SIZE and the caller must check that.
> >>>   */
> >>>  static inline struct kmem_cache *
> >>> -kmalloc_slab(size_t size, kmem_buckets *b, gfp_t flags, kmalloc_token_t token)
> >>> +kmalloc_slab(size_t size, kmem_buckets *b, gfp_t flags, kmalloc_token_t token,
> >>> +            unsigned int alloc_flags)
> >>>  {
> >>>         unsigned int index;
> >>> +       enum kmalloc_cache_type type = kmalloc_type(flags, token);
> >>> +
> >>> +       if (alloc_flags & SLAB_ALLOC_NO_OBJ_EXT)
> >>> +               type = KMALLOC_NO_OBJ_EXT;
> >
> > Hi Suren, thanks for the reviews.
> > It's indeed helpful to have an eye for those bugfixes.
> >
> >> Why not let kmalloc_type() handle alloc_flags?
> >
> > Good point!
> >
> >> Other users (there are
> >> only 4 of them) can pass SLAB_ALLOC_DEFAULT. That seems cleaner to me
> >> and more robust.
> >
> > Hmm, there was a reason... *checks notes*, oh, there is no note.
> > IIRC I was afraid of exposing SLAB_ALLOC_* flags to arbitrary users.
>
> Hm good point, they are in mm/slab.h and kmalloc_type() needs to be in the
> public include/linux/slab.h. Given its usage in __builtin_constant_p(size)
> paths of kmalloc[_node] I would rather not complicate it (even if a
> hardcoded SLAB_ALLOC_DEFAULT should eliminate the extra code).
> So I'd rather keep KMALLOC_NO_OBJ_EXT handling hidden in mm/ as long as it's
> possible.
>
> > should probably fine as long as it's not used in
> > kmalloc/kmem_cache_alloc() APIs, not sure.
> >
> >>>         if (!b)
> >>> -               b = &kmalloc_caches[kmalloc_type(flags, token)];
> >>> +               b = &kmalloc_caches[type];
> >>>         if (size <= 192)
> >>>                 index = kmalloc_size_index[size_index_elem(size)];
> >>>         else
> >>> diff --git a/mm/slab_common.c b/mm/slab_common.c
> >>> index b6426d7ceec9..03ecac12cd86 100644
> >>> --- a/mm/slab_common.c
> >>> +++ b/mm/slab_common.c
> >>> @@ -957,6 +968,12 @@ new_kmalloc_cache(int idx, enum kmalloc_cache_type type)
> >>>                         return;
> >>>                 }
> >>>                 flags |= SLAB_ACCOUNT;
> >>> +       } else if (IS_ENABLED(CONFIG_SLAB_OBJ_EXT) && type == KMALLOC_NO_OBJ_EXT) {
> >>
> >> Hmm, you have to check IS_ENABLED(CONFIG_SLAB_OBJ_EXT) here because
> >> KMALLOC_NO_OBJ_EXT can be aliased with KMALLOC_NORMAL... Could we
> >> instead have a helper function like this (maybe with a better name):
> >
> > Hmm that's fine, but I think that bit should not be part of -stable
> > fixes at least. Here I tried to make it consistent with KMALLOC_RECLAIM
> > and KMALLOC_DMA :)
>
> Agreed.
>
> >> #ifdef CONFIG_SLAB_OBJ_EXT
> >> bool is_kmalloc_no_obj_ext_type(type) { return type == KMALLOC_NO_OBJ_EXT; }
> >> #else
> >> bool is_kmalloc_no_obj_ext_type(type) { return false; }
> >> #endif
> >> ?
> >
> > is_kmalloc_no_obj_ext_type(),
> > kmalloc_type_is_no_obj_ext(),
> > is_kmalloc_type_no_obj_ext(),
> > ...
> >
> > naming is hard, ugh :)
> >
> >>
> >>> +               if (!need_kmalloc_no_objext()) {
> >>> +                       kmalloc_caches[type][idx] = kmalloc_caches[KMALLOC_NORMAL][idx];
> >>
> >> Could kmalloc_caches[KMALLOC_NORMAL][idx] be NULL here?
> >
> > No. KMALLOC_NORMAL caches are created before all other kmalloc
> > caches.
> >
> > IIRC checking if kmalloc_caches[KMALLOC_NORMAL][idx] is NULL was added
> > by commit 963e84b0f262 ("mm/slab: limit kmalloc() minimum alignment
> > to dma_get_cache_alignment()") to avoid creating kmalloc caches of same
> > type and size due to minimum alignment.
> >
> >> In general why do we special-case and do an early exit here?
> >>
> >> Can we do instead:
> >>
> >> if (need_kmalloc_no_objext())
> >>     flags |= SLAB_NO_OBJ_EXT | SLAB_NO_MERGE;
> >>
> >> and use the common path?
> >
> > Hmm, but even without SLAB_NO_MERGE, we often end up not merging
> > kmalloc caches e.g.) because of non-zero s->usersize.
> >
> > I think that's why we do special-case and an early exit?
> >
> > We could probably do some refactoring to change that, but in general
> > I'm afraid of backporting refactoring work to -stable because I fear
> > introducing very subtle behavioral changes that nobody would notice.
>
> Agreed, let's keep hotfixes as minimal as possible and refactor later.

Sounds good. Feel free to add:

Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>

>
> >>> +                       return;
> >>> +               }
> >>> +               flags |= SLAB_NO_OBJ_EXT | SLAB_NO_MERGE;
> >>>         } else if (IS_ENABLED(CONFIG_ZONE_DMA) && (type == KMALLOC_DMA)) {
> >>>                 flags |= SLAB_CACHE_DMA;
> >>>         }
> >>> diff --git a/mm/slub.c b/mm/slub.c
> >>> index abe748b7dddb..a34f9b8770dc 100644
> >>> --- a/mm/slub.c
> >>> +++ b/mm/slub.c
> >>> @@ -2168,14 +2132,20 @@ int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
> >>>         unsigned long new_exts;
> >>>         unsigned long old_exts;
> >>>         struct slabobj_ext *vec;
> >>> -       size_t sz;
> >>> +       size_t sz = sizeof(struct slabobj_ext) * slab->objects;
> >>>
> >>>         gfp &= ~OBJCGS_CLEAR_MASK;
> >>> -       /* Prevent recursive extension vector allocation */
> >>> -       alloc_flags |= SLAB_ALLOC_NO_RECURSE;
> >>> -       alloc_flags &= ~SLAB_ALLOC_NEW_SLAB;
> >>> +       /*
> >>> +        * In most cases, obj_exts arrays are allocated from normal kmalloc.
> >>> +        * However, normal kmalloc caches must allocate them from
> >>> +        * KMALLOC_NO_OBJ_EXT caches to prevent recursion.
> >>
> >> For debugging it would have been convenient to allocate all obj_ext
> >> vectors from dedicated caches... Maybe we can do that for
> >> CONFIG_DEBUG_VM or someday when we add CONFIG_OBJ_EXT_DEBUG? Anyway,
> >> not really a complaint but a wish.
> >
> > Vlastimil and I had a conversation on always (even w/o debug options)
> > having dedicated caches (primarily to separate lifetime and for
> > simplicity), it would be interesting to explore.
> >
> > https://lore.kernel.org/all/2436707a-b6ab-45ec-98e5-538e18589462@xxxxxxxxxx
> >
>