[PATCH] x86: Fix memory leak of init_vdso_vars()

[PATCH] x86: Fix memory leak of init_vdso_vars()

[wzt]

Fix memory leak of init_vdso_vars().

Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>

---
 arch/x86/vdso/vma.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/x86/vdso/vma.c b/arch/x86/vdso/vma.c
index 4b5d26f..c6b0308 100644
--- a/arch/x86/vdso/vma.c
+++ b/arch/x86/vdso/vma.c
@@ -44,19 +44,19 @@ static int __init init_vdso_vars(void)
 	vdso_size = npages << PAGE_SHIFT;
 	vdso_pages = kmalloc(sizeof(struct page *) * npages, GFP_KERNEL);
 	if (!vdso_pages)
-		goto oom;
+		goto oom1;
 	for (i = 0; i < npages; i++) {
 		struct page *p;
 		p = alloc_page(GFP_KERNEL);
 		if (!p)
-			goto oom;
+			goto oom2;
 		vdso_pages[i] = p;
 		copy_page(page_address(p), vdso_start + i*PAGE_SIZE);
 	}
 
 	vbase = vmap(vdso_pages, npages, 0, PAGE_KERNEL);
 	if (!vbase)
-		goto oom;
+		goto oom2;
 
 	if (memcmp(vbase, "\177ELF", 4)) {
 		printk("VDSO: I'm broken; not ELF\n");
@@ -70,7 +70,13 @@ static int __init init_vdso_vars(void)
 	vunmap(vbase);
 	return 0;
 
- oom:
+oom2:
+	i--;
+	for (; i >= 0; i--)
+		__free_page(vdso_pages[i]);
+	__free_page(vdso_pages);
+
+oom1:
 	printk("Cannot allocate vdso\n");
 	vdso_enabled = 0;
 	return -ENOMEM;
-- 
1.7.4.1

Re: [PATCH] x86: Fix memory leak of init_vdso_vars()

[Michal Hocko]

On Tue 05-07-11 14:21:48, wzt wrote:
> Fix memory leak of init_vdso_vars().
> 
> Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>

The patch looks correct but it could be cleaned up a bit IMO.
See bellow.

> 
> ---
>  arch/x86/vdso/vma.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/vdso/vma.c b/arch/x86/vdso/vma.c
> index 4b5d26f..c6b0308 100644
> --- a/arch/x86/vdso/vma.c
> +++ b/arch/x86/vdso/vma.c
> @@ -44,19 +44,19 @@ static int __init init_vdso_vars(void)
>  	vdso_size = npages << PAGE_SHIFT;
>  	vdso_pages = kmalloc(sizeof(struct page *) * npages, GFP_KERNEL);
>  	if (!vdso_pages)
> -		goto oom;
> +		goto oom1;

I wouldn't rename the label. It just makes the patch bigger for no good
reason. The new naming is not anyhow useful.

>  	for (i = 0; i < npages; i++) {
>  		struct page *p;
>  		p = alloc_page(GFP_KERNEL);
>  		if (!p)
> -			goto oom;
> +			goto oom2;

Waht about goto oom_free_pages instead?

>  		vdso_pages[i] = p;
>  		copy_page(page_address(p), vdso_start + i*PAGE_SIZE);
>  	}
>  
>  	vbase = vmap(vdso_pages, npages, 0, PAGE_KERNEL);
>  	if (!vbase)
> -		goto oom;
> +		goto oom2;
>  
>  	if (memcmp(vbase, "\177ELF", 4)) {
>  		printk("VDSO: I'm broken; not ELF\n");
> @@ -70,7 +70,13 @@ static int __init init_vdso_vars(void)
>  	vunmap(vbase);
>  	return 0;
>  
> - oom:
> +oom2:
> +	i--;
> +	for (; i >= 0; i--)
> +		__free_page(vdso_pages[i]);

Why not 
	for (; i > 0; i--)
		__free_page(vdso_pages[i-1]);

> +	__free_page(vdso_pages);
> +
> +oom1:
>  	printk("Cannot allocate vdso\n");
>  	vdso_enabled = 0;
>  	return -ENOMEM;

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] x86: Fix memory leak of init_vdso_vars()

[Andi Kleen]

On 7/7/2011 6:14 AM, Michal Hocko wrote:
> On Tue 05-07-11 14:21:48, wzt wrote:
>> Fix memory leak of init_vdso_vars().
>>
>> Signed-off-by: Zhitong Wang<zhitong.wangzt@alibaba-inc.com>
> The patch looks correct but it could be cleaned up a bit IMO.
> See bellow.

When you run out of memory at early system boot (this is early
system boot only) the system is toast anyways. There's no way
to recover.

I would just add a few GFP_PANICs and then drop the error paths.

-Andi

Re: [PATCH] x86: Fix memory leak of init_vdso_vars()

[wzt wzt]

maybe, we can just panic() when kmalloc failed instead of add a new GFP_PANIC.

new patch will coming soon.

On Fri, Jul 8, 2011 at 2:33 AM, Andi Kleen <ak@linux.intel.com> wrote:
> When you run out of memory at early system boot (this is early
> system boot only) the system is toast anyways. There's no way
> to recover.
>
> I would just add a few GFP_PANICs and then drop the error paths.
>
> -Andi
>
>

Re: [PATCH] x86: Fix memory leak of init_vdso_vars()

[Ingo Molnar]

* wzt wzt <wzt.wzt@gmail.com> wrote:

> maybe, we can just panic() when kmalloc failed instead of add a new 
> GFP_PANIC.
> 
> new patch will coming soon.
> 
> On Fri, Jul 8, 2011 at 2:33 AM, Andi Kleen <ak@linux.intel.com> wrote:
> > When you run out of memory at early system boot (this is early
> > system boot only) the system is toast anyways. There's no way
> > to recover.
> >
> > I would just add a few GFP_PANICs and then drop the error paths.

That's a very sloppy way of doing it: should anyone ever want to 
reuse this function *not* in an init path (which is a future 
possibility with the vdso) it will have a nasty panic() embedded in 
it ...

Thanks,

	Ingo

[PATCH] x86: Fix memory leak of init_vdso_vars()

[Andy Lutomirski]

From: Zhitong Wang <wzt.wzt@gmail.com>

If init_vdso_vars ran out of memory (not very likely), then it would
leak a few pages as well.

Also rename init_vdso_vars to just init_vdso, since initializing
vvars is just about the only thing this function doesn't do.

Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>
[Rebased and slightly simplified from the original.]
Signed-off-by: Andy Lutomirski <luto@mit.edu>
---
 arch/x86/vdso/vma.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/vdso/vma.c b/arch/x86/vdso/vma.c
index c39938d..cabaf0e 100644
--- a/arch/x86/vdso/vma.c
+++ b/arch/x86/vdso/vma.c
@@ -54,7 +54,7 @@ found:
 	apply_alternatives(alt_data, alt_data + alt_sec->sh_size);
 }
 
-static int __init init_vdso_vars(void)
+static int __init init_vdso(void)
 {
 	int npages = (vdso_end - vdso_start + PAGE_SIZE - 1) / PAGE_SIZE;
 	int i;
@@ -69,19 +69,24 @@ static int __init init_vdso_vars(void)
 		struct page *p;
 		p = alloc_page(GFP_KERNEL);
 		if (!p)
-			goto oom;
+			goto oom_free;
 		vdso_pages[i] = p;
 		copy_page(page_address(p), vdso_start + i*PAGE_SIZE);
 	}
 
 	return 0;
 
- oom:
+oom_free:
+	for(i--; i >= 0; i--)
+		__free_page(vdso_pages[i]);
+	__free_page(vdso_pages);
+
+oom:
 	printk("Cannot allocate vdso\n");
 	vdso_enabled = 0;
 	return -ENOMEM;
 }
-subsys_initcall(init_vdso_vars);
+subsys_initcall(init_vdso);
 
 struct linux_binprm;
 
-- 
1.7.6

Re: [PATCH] x86: Fix memory leak of init_vdso_vars()

[Andi Kleen]

On Thu, Jul 21, 2011 at 10:33:14AM -0400, Andy Lutomirski wrote:
> From: Zhitong Wang <wzt.wzt@gmail.com>
> 
> If init_vdso_vars ran out of memory (not very likely), then it would
> leak a few pages as well.
> 
> Also rename init_vdso_vars to just init_vdso, since initializing
> vvars is just about the only thing this function doesn't do.

Just add a GFP_PANIC, there's no way to recover from this.
Your system will not work without a vdso.

-Andi

Re: [PATCH] x86: Fix memory leak of init_vdso_vars()

[Andrew Lutomirski]

On Thu, Jul 21, 2011 at 1:08 PM, Andi Kleen <ak@linux.intel.com> wrote:
> On Thu, Jul 21, 2011 at 10:33:14AM -0400, Andy Lutomirski wrote:
>> From: Zhitong Wang <wzt.wzt@gmail.com>
>>
>> If init_vdso_vars ran out of memory (not very likely), then it would
>> leak a few pages as well.
>>
>> Also rename init_vdso_vars to just init_vdso, since initializing
>> vvars is just about the only thing this function doesn't do.
>
> Just add a GFP_PANIC, there's no way to recover from this.
> Your system will not work without a vdso.

Ingo objected to this before, although I'm not convinved.  Calling
init_vdso_vars more than once will cause major problems (like
double-patching of alternatives).  If there's too little memory for it
to work, then presumably there's also too little memory to start init.
 (Also, I bet that no one ever audited whether the ELF loader works
right if the vDSO failed to load.)

Ingo?

--Andy

>
> -Andi
>

Re: [PATCH] x86: Fix memory leak of init_vdso_vars()

[Ingo Molnar]

* Andrew Lutomirski <luto@mit.edu> wrote:

> On Thu, Jul 21, 2011 at 1:08 PM, Andi Kleen <ak@linux.intel.com> wrote:
> > On Thu, Jul 21, 2011 at 10:33:14AM -0400, Andy Lutomirski wrote:
> >> From: Zhitong Wang <wzt.wzt@gmail.com>
> >>
> >> If init_vdso_vars ran out of memory (not very likely), then it would
> >> leak a few pages as well.
> >>
> >> Also rename init_vdso_vars to just init_vdso, since initializing
> >> vvars is just about the only thing this function doesn't do.
> >
> > Just add a GFP_PANIC, there's no way to recover from this.
> > Your system will not work without a vdso.
>
> Ingo objected to this before, although I'm not convinved.  Calling 
> init_vdso_vars more than once will cause major problems (like 
> double-patching of alternatives).  If there's too little memory for 
> it to work, then presumably there's also too little memory to start 
> init.
>
>  (Also, I bet that no one ever audited whether the ELF loader works 
> right if the vDSO failed to load.)
> 
> Ingo?

This assumes that the system actually needs an ELF loader - if a 
static binary is booted via a init= boot parameter it might not be 
needed.

Memory failure injection code will also cause this to panic early 
during bootup spuriously.

Really, we should cleanly tear down what we built up and fail cleanly 
as well, no need to be sloppy since we already have the patch. That 
some other code down the boot chain might be sloppy is no excuse to 
be sloppy here.

Would be nice to stick a WARN_ON() into the oom branch though, as 
it's clearly an anomalous condition.

(btw., there's no GFP_PANIC, we never had any such flag for the page 
allocator.)

Thanks,

	Ingo

Re: [PATCH] x86: Fix memory leak of init_vdso_vars()

[Andrew Lutomirski]

On Thu, Jul 21, 2011 at 2:38 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Andrew Lutomirski <luto@mit.edu> wrote:
>
>> On Thu, Jul 21, 2011 at 1:08 PM, Andi Kleen <ak@linux.intel.com> wrote:
>> > On Thu, Jul 21, 2011 at 10:33:14AM -0400, Andy Lutomirski wrote:
>> >> From: Zhitong Wang <wzt.wzt@gmail.com>
>> >>
>> >> If init_vdso_vars ran out of memory (not very likely), then it would
>> >> leak a few pages as well.
>> >>
>> >> Also rename init_vdso_vars to just init_vdso, since initializing
>> >> vvars is just about the only thing this function doesn't do.
>> >
>> > Just add a GFP_PANIC, there's no way to recover from this.
>> > Your system will not work without a vdso.
>>
>> Ingo objected to this before, although I'm not convinved.  Calling
>> init_vdso_vars more than once will cause major problems (like
>> double-patching of alternatives).  If there's too little memory for
>> it to work, then presumably there's also too little memory to start
>> init.
>>
>>  (Also, I bet that no one ever audited whether the ELF loader works
>> right if the vDSO failed to load.)
>>
>> Ingo?
>
> This assumes that the system actually needs an ELF loader - if a
> static binary is booted via a init= boot parameter it might not be
> needed.

I actually meant the kernel's loaded.  But I just looked and it appears correct.

But I think this whole thing is silly, because I can't see any good
reason that the vdso needs to allocate memory in the first place.
I'll send a patch.

--Andy

Re: [PATCH] x86: Fix memory leak of init_vdso_vars()

[Ingo Molnar]

* Andrew Lutomirski <luto@mit.edu> wrote:

> On Thu, Jul 21, 2011 at 2:38 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Andrew Lutomirski <luto@mit.edu> wrote:
> >
> >> On Thu, Jul 21, 2011 at 1:08 PM, Andi Kleen <ak@linux.intel.com> wrote:
> >> > On Thu, Jul 21, 2011 at 10:33:14AM -0400, Andy Lutomirski wrote:
> >> >> From: Zhitong Wang <wzt.wzt@gmail.com>
> >> >>
> >> >> If init_vdso_vars ran out of memory (not very likely), then it would
> >> >> leak a few pages as well.
> >> >>
> >> >> Also rename init_vdso_vars to just init_vdso, since initializing
> >> >> vvars is just about the only thing this function doesn't do.
> >> >
> >> > Just add a GFP_PANIC, there's no way to recover from this.
> >> > Your system will not work without a vdso.
> >>
> >> Ingo objected to this before, although I'm not convinved.  Calling
> >> init_vdso_vars more than once will cause major problems (like
> >> double-patching of alternatives).  If there's too little memory for
> >> it to work, then presumably there's also too little memory to start
> >> init.
> >>
> >>  (Also, I bet that no one ever audited whether the ELF loader works
> >> right if the vDSO failed to load.)
> >>
> >> Ingo?
> >
> > This assumes that the system actually needs an ELF loader - if a
> > static binary is booted via a init= boot parameter it might not be
> > needed.
> 
> I actually meant the kernel's loaded.  But I just looked and it appears correct.
> 
> But I think this whole thing is silly, because I can't see any good
> reason that the vdso needs to allocate memory in the first place.
> I'll send a patch.

that's the perfect solution indeed :)

Thanks,

	Ingo

[PATCH] x86-64: Do not allocate memory for the vDSO

[Andy Lutomirski]

We can map the vDSO straight from kernel data, saving a few page
allocations.  As an added bonus, the deleted code contained a memory
leak.

Signed-off-by: Andy Lutomirski <luto@mit.edu>
---
 arch/x86/vdso/vdso.S |   15 +++++++++++++--
 arch/x86/vdso/vma.c  |   25 ++++++-------------------
 2 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/arch/x86/vdso/vdso.S b/arch/x86/vdso/vdso.S
index 1d3aa6b..1b979c1 100644
--- a/arch/x86/vdso/vdso.S
+++ b/arch/x86/vdso/vdso.S
@@ -1,10 +1,21 @@
+#include <asm/page_types.h>
+#include <linux/linkage.h>
 #include <linux/init.h>
 
-__INITDATA
+__PAGE_ALIGNED_DATA
 
 	.globl vdso_start, vdso_end
+	.align PAGE_SIZE
 vdso_start:
 	.incbin "arch/x86/vdso/vdso.so"
 vdso_end:
 
-__FINIT
+.previous
+
+	.globl vdso_pages
+	.bss
+	.align 8
+	.type vdso_pages, @object
+vdso_pages:
+	.zero (vdso_end - vdso_start + PAGE_SIZE - 1) / PAGE_SIZE * 8
+	.size vdso_pages, .-vdso_pages
diff --git a/arch/x86/vdso/vma.c b/arch/x86/vdso/vma.c
index ba92244..40e4cb0 100644
--- a/arch/x86/vdso/vma.c
+++ b/arch/x86/vdso/vma.c
@@ -14,13 +14,14 @@
 #include <asm/vgtod.h>
 #include <asm/proto.h>
 #include <asm/vdso.h>
+#include <asm/page.h>
 
 unsigned int __read_mostly vdso_enabled = 1;
 
 extern char vdso_start[], vdso_end[];
 extern unsigned short vdso_sync_cpuid;
 
-static struct page **vdso_pages;
+extern struct page *vdso_pages[];
 static unsigned vdso_size;
 
 static void patch_vdso(void *vdso, size_t len)
@@ -51,7 +52,7 @@ found:
 	apply_alternatives(alt_data, alt_data + alt_sec->sh_size);
 }
 
-static int __init init_vdso_vars(void)
+static int __init init_vdso(void)
 {
 	int npages = (vdso_end - vdso_start + PAGE_SIZE - 1) / PAGE_SIZE;
 	int i;
@@ -59,26 +60,12 @@ static int __init init_vdso_vars(void)
 	patch_vdso(vdso_start, vdso_end - vdso_start);
 
 	vdso_size = npages << PAGE_SHIFT;
-	vdso_pages = kmalloc(sizeof(struct page *) * npages, GFP_KERNEL);
-	if (!vdso_pages)
-		goto oom;
-	for (i = 0; i < npages; i++) {
-		struct page *p;
-		p = alloc_page(GFP_KERNEL);
-		if (!p)
-			goto oom;
-		vdso_pages[i] = p;
-		copy_page(page_address(p), vdso_start + i*PAGE_SIZE);
-	}
+	for (i = 0; i < npages; i++)
+		vdso_pages[i] = virt_to_page(vdso_start + i*PAGE_SIZE);
 
 	return 0;
-
- oom:
-	printk("Cannot allocate vdso\n");
-	vdso_enabled = 0;
-	return -ENOMEM;
 }
-subsys_initcall(init_vdso_vars);
+subsys_initcall(init_vdso);
 
 struct linux_binprm;
 
-- 
1.7.6

[tip:x86/vdso] x86-64, vdso: Do not allocate memory for the vDSO

[tip-bot for Andy Lutomirski]

Commit-ID:  aafade242ff24fac3aabf61c7861dfa44a3c2445
Gitweb:     http://git.kernel.org/tip/aafade242ff24fac3aabf61c7861dfa44a3c2445
Author:     Andy Lutomirski <luto@mit.edu>
AuthorDate: Thu, 21 Jul 2011 15:47:10 -0400
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Thu, 21 Jul 2011 13:41:53 -0700

x86-64, vdso: Do not allocate memory for the vDSO

We can map the vDSO straight from kernel data, saving a few page
allocations.  As an added bonus, the deleted code contained a memory
leak.

Signed-off-by: Andy Lutomirski <luto@mit.edu>
Link: http://lkml.kernel.org/r/2c4ed5c2c2e93603790229e0c3403ae506ccc0cb.1311277573.git.luto@mit.edu
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/vdso/vdso.S |   15 +++++++++++++--
 arch/x86/vdso/vma.c  |   25 ++++++-------------------
 2 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/arch/x86/vdso/vdso.S b/arch/x86/vdso/vdso.S
index 1d3aa6b..1b979c1 100644
--- a/arch/x86/vdso/vdso.S
+++ b/arch/x86/vdso/vdso.S
@@ -1,10 +1,21 @@
+#include <asm/page_types.h>
+#include <linux/linkage.h>
 #include <linux/init.h>
 
-__INITDATA
+__PAGE_ALIGNED_DATA
 
 	.globl vdso_start, vdso_end
+	.align PAGE_SIZE
 vdso_start:
 	.incbin "arch/x86/vdso/vdso.so"
 vdso_end:
 
-__FINIT
+.previous
+
+	.globl vdso_pages
+	.bss
+	.align 8
+	.type vdso_pages, @object
+vdso_pages:
+	.zero (vdso_end - vdso_start + PAGE_SIZE - 1) / PAGE_SIZE * 8
+	.size vdso_pages, .-vdso_pages
diff --git a/arch/x86/vdso/vma.c b/arch/x86/vdso/vma.c
index c39938d..316fbca 100644
--- a/arch/x86/vdso/vma.c
+++ b/arch/x86/vdso/vma.c
@@ -14,13 +14,14 @@
 #include <asm/vgtod.h>
 #include <asm/proto.h>
 #include <asm/vdso.h>
+#include <asm/page.h>
 
 unsigned int __read_mostly vdso_enabled = 1;
 
 extern char vdso_start[], vdso_end[];
 extern unsigned short vdso_sync_cpuid;
 
-static struct page **vdso_pages;
+extern struct page *vdso_pages[];
 static unsigned vdso_size;
 
 static void __init patch_vdso(void *vdso, size_t len)
@@ -54,7 +55,7 @@ found:
 	apply_alternatives(alt_data, alt_data + alt_sec->sh_size);
 }
 
-static int __init init_vdso_vars(void)
+static int __init init_vdso(void)
 {
 	int npages = (vdso_end - vdso_start + PAGE_SIZE - 1) / PAGE_SIZE;
 	int i;
@@ -62,26 +63,12 @@ static int __init init_vdso_vars(void)
 	patch_vdso(vdso_start, vdso_end - vdso_start);
 
 	vdso_size = npages << PAGE_SHIFT;
-	vdso_pages = kmalloc(sizeof(struct page *) * npages, GFP_KERNEL);
-	if (!vdso_pages)
-		goto oom;
-	for (i = 0; i < npages; i++) {
-		struct page *p;
-		p = alloc_page(GFP_KERNEL);
-		if (!p)
-			goto oom;
-		vdso_pages[i] = p;
-		copy_page(page_address(p), vdso_start + i*PAGE_SIZE);
-	}
+	for (i = 0; i < npages; i++)
+		vdso_pages[i] = virt_to_page(vdso_start + i*PAGE_SIZE);
 
 	return 0;
-
- oom:
-	printk("Cannot allocate vdso\n");
-	vdso_enabled = 0;
-	return -ENOMEM;
 }
-subsys_initcall(init_vdso_vars);
+subsys_initcall(init_vdso);
 
 struct linux_binprm;