Re: [PATCH] mm/slub: preserve original size in _kmalloc_nolock_noprof retry path

[hu.shengming]

Harry wrote:
> On 6/3/26 10:10 PM, hu.shengming@xxxxxxxxxx wrote:
> > From: Shengming Hu <hu.shengming@xxxxxxxxxx>
> > 
> > _kmalloc_nolock_noprof() retries from the next kmalloc bucket when the
> > initial allocation fails. The retry currently reuses `size` as the
> > bucket selector and overwrites it with s->object_size + 1.
> > 
> > That value is later passed as the original allocation size to
> > __slab_alloc_node(), slab_post_alloc_hook() and kasan_kmalloc(). On a
> > successful retry this makes KASAN/slub-debug observe the retry bucket
> > selector rather than the caller requested size, potentially widening the
> > valid kmalloc range and hiding overflows.
> 
> Good catch!
> 

Hi Harry,

> > Keep a separate `bucket_size` for choosing the retry cache and preserve
> > `size`.
> > 
> > Fixes: <af92793e52c3> ("slab: Introduce kmalloc_nolock() and kfree_nolock()")
> > Signed-off-by: Shengming Hu <hu.shengming@xxxxxxxxxx>
> > ---
> 
> I want to note that this conflicts with Vlastimil's work-in-progress
> feature [1] where it separately stores orig_size in struct
> slab_alloc_context which naturally solves the problem.
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/linux.git/log/?h=b4/slab_alloc_flags
> 
> No strong opinion on whether to queue this patch and then rebase
> Vlastimil's work on top, or wait for Vlastimil's work to solve the
> problem instead...
> 

Thanks for the review and for pointing out Vlastimil's work-in-progress
series.

> >  mm/slub.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 67abbbf68fc1..6a2b3ade3611 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -5350,6 +5350,7 @@ EXPORT_SYMBOL(__kmalloc_noprof);
> >  void *_kmalloc_nolock_noprof(DECL_TOKEN_PARAMS(size, token), gfp_t gfp_flags, int node)
> >  {
> >      gfp_t alloc_gfp = __GFP_NOWARN | __GFP_NOMEMALLOC | gfp_flags;
> > +    size_t bucket_size = size;
> >      struct kmem_cache *s;
> >      bool can_retry = true;
> >      void *ret;
> 
> But in either way, I think it's more straightforward to introduce
> orig_size as a variable to keep the original size and pass it to
> __slab_alloc_node().
> 

I agree that introducing an explicit orig_size is more straightforward
than adding bucket_size.

I'll update the patch to keep:

	size_t orig_size = size;

and use orig_size for the allocation hooks/KASAN paths, while allowing
size to continue being advanced for the retry cache selection.

> > @@ -5372,9 +5373,9 @@ void *_kmalloc_nolock_noprof(DECL_TOKEN_PARAMS(size, token), gfp_t gfp_flags, in
> >          return NULL;
> > 
> >  retry:
> > -    if (unlikely(size > KMALLOC_MAX_CACHE_SIZE))
> > +    if (unlikely(bucket_size > KMALLOC_MAX_CACHE_SIZE))
> >          return NULL;
> > -    s = kmalloc_slab(size, NULL, alloc_gfp, PASS_TOKEN_PARAM(token));
> > +    s = kmalloc_slab(bucket_size, NULL, alloc_gfp, PASS_TOKEN_PARAM(token));
> > 
> >      if (!(s->flags & __CMPXCHG_DOUBLE) && !kmem_cache_debug(s))
> >          /*
> > @@ -5408,7 +5409,7 @@ void *_kmalloc_nolock_noprof(DECL_TOKEN_PARAMS(size, token), gfp_t gfp_flags, in
> >       */
> >      if (!ret && can_retry) {
> >          /* pick the next kmalloc bucket */
> > -        size = s->object_size + 1;
> > +        bucket_size = s->object_size + 1;
> >          /*
> >           * Another alternative is to
> >           * if (memcg) alloc_gfp &= ~__GFP_ACCOUNT;
> 
> -- 
> Cheers,
> Harry / Hyeonggon

--
With Best Regards,
Shengming