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

[Vlastimil Babka (SUSE)]

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.

>>> +                       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
>