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@xxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/