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

[Harry Yoo <harry@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 5461 bytes --]


On 7/14/26 2:08 AM, Suren Baghdasaryan wrote:
> On Mon, Jul 13, 2026 at 7:29 AM Harry Yoo (Oracle) <harry@kernel.org> 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.

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 :)

> #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.

>> +                       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@kernel.org

-- 
Cheers,
Harry / Hyeonggon

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]