x86: Fix memory leak of init_vdso_vars()

Fix memory leak of init_vdso_vars().

Signed-off-by Zhitong Wang <[email protected]>

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

arch/x86/vdso/vma.c +0 -0

index 4b5d26f..c6b0308 100644

arch/x86/vdso/vma.c +1 -1

 		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");

arch/x86/vdso/vma.c +3 -0

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

Quoted 3 lines · wzt

Fix memory leak of init_vdso_vars().

Signed-off-by Zhitong Wang<[email protected]>

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

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

new patch will coming soon.

Quoted 5 lines · Andi Kleen

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.

From: Zhitong Wang <[email protected]>

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.

Quoted 7 lines · Andy Lutomirski

From: Zhitong Wang <[email protected]>

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

Quoted 11 lines · Andi Kleen

Quoted 7 lines · Andy Lutomirski

From: Zhitong Wang <[email protected]>

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?

Quoted 19 lines · Andrew Lutomirski

Quoted 11 lines · Andi Kleen

Quoted 7 lines · Andy Lutomirski

From: Zhitong Wang <[email protected]>

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.

Quoted 4 lines

 (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.

Quoted 22 lines · Ingo Molnar

Quoted 19 lines · Andrew Lutomirski

Quoted 11 lines · Andi Kleen

Quoted 7 lines · Andy Lutomirski

From: Zhitong Wang <[email protected]>

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.

Quoted 8 lines

Quoted 4 lines

 (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.