linux-kernel - Re: [PATCH] x86: Fix memory leak of init_vdso

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 7 Jul 2011 15:14:55 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	wzt <wzt.wzt@...il.com>
Cc:	mingo@...hat.com, linux-kernel@...r.kernel.org, x86@...nel.org,
	ak@...ux.intel.com, tglx@...utronix.de, hpa@...or.com
Subject: Re: [PATCH] x86: Fix memory leak of init_vdso_vars()

On Tue 05-07-11 14:21:48, wzt wrote:
> Fix memory leak of init_vdso_vars().
> 
> Signed-off-by: Zhitong Wang <zhitong.wangzt@...baba-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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/