[PATCH] mm: fix oom roll-back of __vmalloc_area_node

[PATCH] mm: fix oom roll-back of __vmalloc_area_node

[Jan Kiszka]

__vunmap must not rely on area->nr_pages when picking the
release methode for area->pages. It may be too small when
__vmalloc_area_node failed early due to lacking memory.
Instead, use a flag in vmstruct to differentiate.

Applies against 2.6.18-rc1, but sighted, developed, and
tested on a 2.6.16 kernel.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>

---
 include/linux/vmalloc.h |    1 +
 mm/vmalloc.c            |    7 ++++---
 2 files changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/vmalloc.h
===================================================================
--- linux-2.6.orig/include/linux/vmalloc.h
+++ linux-2.6/include/linux/vmalloc.h
@@ -11,6 +11,7 @@ struct vm_area_struct;
 #define VM_ALLOC	0x00000002	/* vmalloc() */
 #define VM_MAP		0x00000004	/* vmap()ed pages */
 #define VM_USERMAP	0x00000008	/* suitable for remap_vmalloc_range */
+#define VM_VPAGES	0x00000010	/* buffer for pages was vmalloc'ed */
 /* bits [20..32] reserved for arch specific ioremap internals */
 
 /*
Index: linux-2.6/mm/vmalloc.c
===================================================================
--- linux-2.6.orig/mm/vmalloc.c
+++ linux-2.6/mm/vmalloc.c
@@ -340,7 +340,7 @@ void __vunmap(void *addr, int deallocate
 			__free_page(area->pages[i]);
 		}
 
-		if (area->nr_pages > PAGE_SIZE/sizeof(struct page *))
+		if (area->flags & VM_VPAGES)
 			vfree(area->pages);
 		else
 			kfree(area->pages);
@@ -427,9 +427,10 @@ void *__vmalloc_area_node(struct vm_stru
 
 	area->nr_pages = nr_pages;
 	/* Please note that the recursion is strictly bounded. */
-	if (array_size > PAGE_SIZE)
+	if (array_size > PAGE_SIZE) {
 		pages = __vmalloc_node(array_size, gfp_mask, PAGE_KERNEL, node);
-	else
+		area->flags |= VM_VPAGES;
+	} else
 		pages = kmalloc_node(array_size, (gfp_mask & ~__GFP_HIGHMEM), node);
 	area->pages = pages;
 	if (!area->pages) {

Re: [PATCH] mm: fix oom roll-back of __vmalloc_area_node

[Andrew Morton]

On Tue, 11 Jul 2006 21:37:08 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> __vunmap must not rely on area->nr_pages when picking the
> release methode for area->pages. It may be too small when
> __vmalloc_area_node failed early due to lacking memory.
> Instead, use a flag in vmstruct to differentiate.

So you mean that when this:

		if (unlikely(!area->pages[i])) {
			/* Successfully allocated i pages, free them in __vunmap() */
			area->nr_pages = i;
			goto fail;

happens, it could be that i <= PAGE_SIZE/sizeof(struct page *) and __vunmap
kfree()s something which it should have vfree()d, yes?

That sounds like a dead box, or worse.

I think the change would be a good one even if it didn't fix a bug, thanks.

Re: [PATCH] mm: fix oom roll-back of __vmalloc_area_node

[Jan Kiszka]

Andrew Morton wrote:

> On Tue, 11 Jul 2006 21:37:08 +0200
> Jan Kiszka <jan.kiszka@web.de> wrote:
>
>   
>> __vunmap must not rely on area->nr_pages when picking the
>> release methode for area->pages. It may be too small when
>> __vmalloc_area_node failed early due to lacking memory.
>> Instead, use a flag in vmstruct to differentiate.
>>     
>
> So you mean that when this:
>
> 		if (unlikely(!area->pages[i])) {
> 			/* Successfully allocated i pages, free them in __vunmap() */
> 			area->nr_pages = i;
> 			goto fail;
>
> happens, it could be that i <= PAGE_SIZE/sizeof(struct page *) and __vunmap
> kfree()s something which it should have vfree()d, yes?
>
>   

Yes, exactly. It then causes some BUG in kfree during unroll.


> That sounds like a dead box, or worse.
>
>   
Someone triggered a too large vmalloc request, that was the scenario here.

> I think the change would be a good one even if it didn't fix a bug, thanks.
>
>   
Meanwhile I thought about an even simpler solution:


__vunmap must not rely on area->nr_pages when picking the
release methode for area->pages. It may be too small when
__vmalloc_area_node failed due to lacking memory. Check
for the vmalloc address range instead.

Signed-off by: Jan Kiszka <jan.kiszka@web.de>

Index: linux-2.6/mm/vmalloc.c
===================================================================
--- linux-2.6.orig/mm/vmalloc.c
+++ linux-2.6/mm/vmalloc.c
@@ -340,7 +340,7 @@ void __vunmap(void *addr, int deallocate
 			__free_page(area->pages[i]);
 		}
 
-		if (area->nr_pages > PAGE_SIZE/sizeof(struct page *))
+		if (area->pages >= VMALLOC_START && area->pages < VMALLOC_END)
 			vfree(area->pages);
 		else
 			kfree(area->pages);

Re: [PATCH] mm: fix oom roll-back of __vmalloc_area_node

[Andrew Morton]

On Thu, 13 Jul 2006 09:38:38 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> > I think the change would be a good one even if it didn't fix a bug, thanks.
> >
> >   
> Meanwhile I thought about an even simpler solution:
> 
> 
> __vunmap must not rely on area->nr_pages when picking the
> release methode for area->pages. It may be too small when
> __vmalloc_area_node failed due to lacking memory. Check
> for the vmalloc address range instead.
> 
> Signed-off by: Jan Kiszka <jan.kiszka@web.de>
> 
> Index: linux-2.6/mm/vmalloc.c
> ===================================================================
> --- linux-2.6.orig/mm/vmalloc.c
> +++ linux-2.6/mm/vmalloc.c
> @@ -340,7 +340,7 @@ void __vunmap(void *addr, int deallocate
>  			__free_page(area->pages[i]);
>  		}
>  
> -		if (area->nr_pages > PAGE_SIZE/sizeof(struct page *))
> +		if (area->pages >= VMALLOC_START && area->pages < VMALLOC_END)
>  			vfree(area->pages);
>  		else
>  			kfree(area->pages);

Nah, your first patch was better.  Very clear, direct, explicit.

It even had a comment.