Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops

[Xiaoming Ni <nixiaoming@huawei.com>]

On 2019/9/23 11:50, Nicolas Pitre wrote:
> On Sat, 21 Sep 2019, Xiaoming Ni wrote:
> 
>> @ Nicolas Pitre
>> Can I make a v2 patch based on your advice ?
>> Or you will submit a patch for "GFP_WONTFAIL" yourself ?
> 
> Here's a patch implementing what I had in mind. This is compile tested 
> only.
> 
> ----- >8
> 
> Subject: [PATCH] mm: add __GFP_WONTFAIL and GFP_ONBOOT
> 
> Some memory allocations are very unlikely to fail during system boot.
> Because of that, the code often doesn't bother to check for allocation
> failure, but this gets reported anyway.
> 
> As an alternative to adding code to check for NULL that has almost no
> chance of ever being exercised, let's use a GFP flag to identify those
> cases and panic the kernel if allocation failure ever occurs.
> 
> Conversion of one such instance is also included.
> 
> Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
> 
.....
....

>  /**
> @@ -285,6 +293,9 @@ struct vm_area_struct;
>   * available and will not wake kswapd/kcompactd on failure. The _LIGHT
>   * version does not attempt reclaim/compaction at all and is by default used
>   * in page fault path, while the non-light is used by khugepaged.
> + *
> + * %GFP_ONBOOT is for relatively small allocations that are not expected
> + * to fail while the system is booting.
>   */
>  #define GFP_ATOMIC	(__GFP_HIGH|__GFP_ATOMIC|__GFP_KSWAPD_RECLAIM)
>  #define GFP_KERNEL	(__GFP_RECLAIM | __GFP_IO | __GFP_FS)
> @@ -300,6 +311,7 @@ struct vm_area_struct;
>  #define GFP_TRANSHUGE_LIGHT	((GFP_HIGHUSER_MOVABLE | __GFP_COMP | \
>  			 __GFP_NOMEMALLOC | __GFP_NOWARN) & ~__GFP_RECLAIM)
>  #define GFP_TRANSHUGE	(GFP_TRANSHUGE_LIGHT | __GFP_DIRECT_RECLAIM)
> +#define GFP_ONBOOT	(GFP_NOWAIT | __GFP_WONTFAIL)
>  

Isn't it better to bind GFP_ONBOOT and GFP_NOWAIT?
Can be not GFP_NOWAIT when applying for memory at boot time

>  /* Convert GFP flags to their corresponding migrate type */
>  #define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ff5484fdbd..36dee09f7f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4625,6 +4625,14 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  fail:
>  	warn_alloc(gfp_mask, ac->nodemask,
>  			"page allocation failure: order:%u", order);
> +	if (gfp_mask & __GFP_WONTFAIL) {

Is it more intuitive to use __GFP_DIE_IF_FAIL as the flag name?

> +		/*
> +		 * The assumption was wrong. This is never supposed to happen.
> +		 * Caller most likely won't check for a returned NULL either.
> +		 * So the only reasonable thing to do is to pannic.
> +		 */
> +		panic("Failed to allocate memory despite GFP_WONTFAIL\n");
> +	}
>  got_pg:
>  	return page;
>  }
> 
> .
> 

thanks
Niaoming Ni