Re: [PATCH RFC] mm: warn potential return NULL for kmalloc_array and kvmalloc_array with __GFP_NOFAIL

[Michal Hocko <mhocko@suse.com>]

On Thu 18-07-24 11:00:25, Barry Song wrote:
> From: Barry Song <v-songbaohua@oppo.com>
> 
> Overflow in this context is highly unlikely. However, allocations using
> GFP_NOFAIL are guaranteed to succeed, so checking the return value is
> unnecessary. One option to fix this is allowing memory allocation with
> an overflowed size, but it seems pointless. Let's at least issue a
> warning. Likely BUG_ON() seems better as anyway we can't fix it?

WARN_ON is effectively BUG_ON with panic_on_warn so if this happens to
be in a user triggerable path then you would have an easy way to panic
the whole machine. It is likely true that the kernel could oops just
right after the failure but that could be recoverable at least.

If anything I would just pr_warn with caller address or add dump_stack
to capture the full trace. That would give us the caller that needs
fixing without panicing the system with panic_on_warn.

Btw. what has led you to create this patch? Have you encountered a
misbehaving caller?

Realistically speaking k*malloc(GFP_NOFAIL) with large values (still
far from overflow) would be very dangerous as well. So I am not really
sure this is buying us much if anything at all.

> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Lorenzo Stoakes <lstoakes@gmail.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Roman Gushchin <roman.gushchin@linux.dev>
> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> ---
>  include/linux/slab.h | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index a332dd2fa6cd..c6aec311864f 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -692,8 +692,10 @@ static inline __alloc_size(1, 2) void *kmalloc_array_noprof(size_t n, size_t siz
>  {
>  	size_t bytes;
>  
> -	if (unlikely(check_mul_overflow(n, size, &bytes)))
> +	if (unlikely(check_mul_overflow(n, size, &bytes))) {
> +		WARN_ON(flags & __GFP_NOFAIL);
>  		return NULL;
> +	}
>  	if (__builtin_constant_p(n) && __builtin_constant_p(size))
>  		return kmalloc_noprof(bytes, flags);
>  	return kmalloc_noprof(bytes, flags);
> @@ -794,8 +796,10 @@ kvmalloc_array_node_noprof(size_t n, size_t size, gfp_t flags, int node)
>  {
>  	size_t bytes;
>  
> -	if (unlikely(check_mul_overflow(n, size, &bytes)))
> +	if (unlikely(check_mul_overflow(n, size, &bytes))) {
> +		WARN_ON(flags & __GFP_NOFAIL);
>  		return NULL;
> +	}
>  
>  	return kvmalloc_node_noprof(bytes, flags, node);
>  }
> -- 
> 2.34.1

-- 
Michal Hocko
SUSE Labs