2009-07-10 02:06:58

by Figo.zhang

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

__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 <[email protected]>
---
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);

2009-07-22 20:17:19

by Andrew Morton

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

On Fri, 10 Jul 2009 10:06:44 +0800
"Figo.zhang" <[email protected]> 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 <[email protected]>
> ---
> 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?