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

[Hailong Liu <hailong.liu@xxxxxxxx>]

On Thu, 18. Jul 11:00, Barry Song wrote:
> From: Barry Song <v-songbaohua@xxxxxxxx>
>
> 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?
>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Cc: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> Cc: Christoph Lameter <cl@xxxxxxxxx>
> Cc: Pekka Enberg <penberg@xxxxxxxxxx>
> Cc: David Rientjes <rientjes@xxxxxxxxxx>
> Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> Cc: Vlastimil Babka <vbabka@xxxxxxx>
> Cc: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> Cc: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
> Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
> ---
>  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);
Hi Barry:

IMO, using __GFP_NOFAIL guarantees success if and only if the parameters are *correct*.
Maybe we can add here to help callers to find the reason as in mm/page_alloc.c

```
	if (gfp_mask & __GFP_NOFAIL) {
		/*
		 * All existing users of the __GFP_NOFAIL are blockable, so warn
		 * of any new users that actually require GFP_NOWAIT
		 */
		if (WARN_ON_ONCE_GFP(!can_direct_reclaim, gfp_mask))
			goto fail;

		/*
		 * PF_MEMALLOC request from this context is rather bizarre
		 * because we cannot reclaim anything and only can loop waiting
		 * for somebody to do a work for us
		 */
		WARN_ON_ONCE_GFP(current->flags & PF_MEMALLOC, gfp_mask);

		/*
		 * non failing costly orders are a hard requirement which we
		 * are not prepared for much so let's warn about these users
		 * so that we can identify them and convert them to something
		 * else.
		 */
		WARN_ON_ONCE_GFP(costly_order, gfp_mask);
```

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

--
help you, help me,
Hailong.