RE: Subject: [PATCH v2] slab: kmalloc_size_roundup() must not return 0 for non-zero size

[David Laight]

From: Vlastimil Babka
> Sent: 20 September 2023 10:58
> 
> On 9/11/23 18:38, David Laight wrote:
> >> >> So perhaps the best would be to return size for c == NULL, but also do a
> >> >> WARN_ONCE?
> >> >
> >> > That would add a real function call to an otherwise leaf function
> >> > and almost certainly require the compiler create a stack frame.
> >>
> >> Hm I thought WARN is done by tripping on undefined instruction like BUG
> >> these days. Also any code that accepts the call to kmalloc_size_roundup
> >> probably could accept that too.
> >
> > It's probably just worth removing the c == NULL check and
> > assuming there won't be any fallout.
> > The NULL pointer deref is an easy to debug as anything else.
> >
> > If it gets called in any early init code it'll soon show up.
> 
> Good point, early crash should be ok.
> So how about this with my tweaks, looks ok?

Is that just/mainly the change to assume that kmalloc_slab() doesn't fail?
You can also remove 'c'.
	return kmalloc_slab(size, GFP_KERNEL, 0)->object_size;
isn't too long.

I also did a grep for callers.
Nothing in early code, IIRC mainly 'net'.

	David

> I'll put it in -next and
> send with another hotfix to 6.6 next week.
> ----8<----
> From f5de1ee7b35d7ab35c21c79dd13cea49fbe239b7 Mon Sep 17 00:00:00 2001
> From: David Laight <david.laight@xxxxxxxxxx>
> Date: Thu, 7 Sep 2023 12:42:20 +0000
> Subject: [PATCH] Subject: [PATCH v2] slab: kmalloc_size_roundup() must not
>  return 0 for non-zero size
> 
> The typical use of kmalloc_size_roundup() is:
> 
> 	ptr = kmalloc(sz = kmalloc_size_roundup(size), ...);
> 	if (!ptr) return -ENOMEM.
> 
> This means it is vitally important that the returned value isn't less
> than the argument even if the argument is insane.
> In particular if kmalloc_slab() fails or the value is above
> (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return
> its single zero-length buffer ZERO_SIZE_PTR.
> 
> Fix this by returning the input size if the size exceeds
> KMALLOC_MAX_SIZE. kmalloc() will then return NULL as the size really is
> too big.
> 
> kmalloc_slab() should not normally return NULL, unless called too early.
> Again, returning zero is not the correct action as it can be in some
> usage scenarios stored to a variable and only later cause kmalloc()
> return ZERO_SIZE_PTR and subsequent crashes on access. Instead we can
> simply stop checking the kmalloc_slab() result completely, as calling
> kmalloc_size_roundup() too early would then result in an immediate crash
> during boot and the developer noticing an issue in their code.
> 
> [vbabka@xxxxxxx: remove kmalloc_slab() result check, tweak comments and
>  commit log]
> Signed-off-by: David Laight <david.laight@xxxxxxxxxx>
> Fixes: 05a940656e1e ("slab: Introduce kmalloc_size_roundup()")
> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
> ---
>  mm/slab_common.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index e99e821065c3..1dc108224bd1 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -747,22 +747,25 @@ size_t kmalloc_size_roundup(size_t size)
>  {
>  	struct kmem_cache *c;
> 
> -	/* Short-circuit the 0 size case. */
> -	if (unlikely(size == 0))
> -		return 0;
> -	/* Short-circuit saturated "too-large" case. */
> -	if (unlikely(size == SIZE_MAX))
> -		return SIZE_MAX;
> +	if (size && size <= KMALLOC_MAX_CACHE_SIZE) {
> +		/*
> +		 * The flags don't matter since size_index is common to all.
> +		 * Neither does the caller for just getting ->object_size.
> +		 */
> +		c = kmalloc_slab(size, GFP_KERNEL, 0);
> +		return c->object_size;
> +	}
> +
>  	/* Above the smaller buckets, size is a multiple of page size. */
> -	if (size > KMALLOC_MAX_CACHE_SIZE)
> +	if (size && size <= KMALLOC_MAX_SIZE)
>  		return PAGE_SIZE << get_order(size);
> 
>  	/*
> -	 * The flags don't matter since size_index is common to all.
> -	 * Neither does the caller for just getting ->object_size.
> +	 * Return 'size' for 0 - kmalloc() returns ZERO_SIZE_PTR
> +	 * and very large size - kmalloc() may fail.
>  	 */
> -	c = kmalloc_slab(size, GFP_KERNEL, 0);
> -	return c ? c->object_size : 0;
> +	return size;
> +
>  }
>  EXPORT_SYMBOL(kmalloc_size_roundup);
> 
> --
> 2.42.0
> 
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)