Re: [PATCH] vmalloc.c: fix lose num_physpages checking

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 10 Jul 2009 10:06:44 +0800
"Figo.zhang" <figo1802@gmail.com> wrote:

> __get_vm_area_node() lose size (physpages limit) checking, it be called by
> __get_vm_area() that some drivers called it directly.
> 
> Signed-off-by: Figo.zhang  <figo1802@gmail.com>
> --- 
> mm/vmalloc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index f8189a4..99f3aea 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1144,7 +1144,7 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
>  	}
>  
>  	size = PAGE_ALIGN(size);
> -	if (unlikely(!size))
> +	if (unlikely(!size || (size >> PAGE_SHIFT) > num_physpages))
>  		return NULL;
>  
>  	area = kmalloc_node(sizeof(*area), gfp_mask & GFP_RECLAIM_MASK, node);

I question whether those num_physpages checks in vmalloc.c are useful.

a) the caller is doing something crazy

b) if the caller passed in size=num_physpages-1 then that test will
   succeed, but the vmalloc is surely going to fail.

c) a request for >num_physpages of vmalloc space should fail later
   on in the vmalloc code, making this test redundant.

d) the cheerily undocumented __get_vm_area() and
   __get_vm_area_node() don't actually allocate physical pages for the
   area, and those functions cannot assume that the caller will be fully
   populating the area with physical pages, so checking that there are
   enough physical pages to fill the area doesn't make sense.

No?