linux-kernel - Re: [PATCH] mm: mmap: Simplify the failure return working flow

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] [day] [month] [year] [list]

Message-Id: <20150818155708.8d10dac3736d547083c44500@linux-foundation.org>
Date:	Tue, 18 Aug 2015 15:57:08 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Chen Gang <xili_gchen_5257@...mail.com>
Cc:	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	riel@...hat.com, Michal Hocko <mhocko@...e.cz>,
	sasha.levin@...cle.com, linux-mm@...ck.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: mmap: Simplify the failure return working flow

On Wed, 19 Aug 2015 06:27:58 +0800 Chen Gang <xili_gchen_5257@...mail.com> wrote:

> From: Chen Gang <xili_gchen_5257@...mail.com>

As sent, this patch is From:you@...mail and Signed-off-by:you@...il.

This is peculiar.  I'm assuming that it should have been From:you@...il and
I have made that change to my copy of the patch.

You can do this yourself by putting an explicit From: line at the start
of the changelog.


> @@ -2958,23 +2957,23 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
>  		*need_rmap_locks = (new_vma->vm_pgoff <= vma->vm_pgoff);
>  	} else {
>  		new_vma = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
> -		if (new_vma) {
> -			*new_vma = *vma;
> -			new_vma->vm_start = addr;
> -			new_vma->vm_end = addr + len;
> -			new_vma->vm_pgoff = pgoff;
> -			if (vma_dup_policy(vma, new_vma))
> -				goto out_free_vma;
> -			INIT_LIST_HEAD(&new_vma->anon_vma_chain);
> -			if (anon_vma_clone(new_vma, vma))
> -				goto out_free_mempol;
> -			if (new_vma->vm_file)
> -				get_file(new_vma->vm_file);
> -			if (new_vma->vm_ops && new_vma->vm_ops->open)
> -				new_vma->vm_ops->open(new_vma);
> -			vma_link(mm, new_vma, prev, rb_link, rb_parent);
> -			*need_rmap_locks = false;
> -		}
> +		if (!new_vma)
> +			return NULL;
> +		*new_vma = *vma;
> +		new_vma->vm_start = addr;
> +		new_vma->vm_end = addr + len;
> +		new_vma->vm_pgoff = pgoff;
> +		if (vma_dup_policy(vma, new_vma))
> +			goto out_free_vma;
> +		INIT_LIST_HEAD(&new_vma->anon_vma_chain);
> +		if (anon_vma_clone(new_vma, vma))
> +			goto out_free_mempol;
> +		if (new_vma->vm_file)
> +			get_file(new_vma->vm_file);
> +		if (new_vma->vm_ops && new_vma->vm_ops->open)
> +			new_vma->vm_ops->open(new_vma);
> +		vma_link(mm, new_vma, prev, rb_link, rb_parent);
> +		*need_rmap_locks = false;
>  	}
>  	return new_vma;

Embedding a return deep inside the function isn't good.  It can lead to
resource leaks, locking leaks etc as the code evolves.  This is the
main reason why the kernel uses goto, IMO: single-entry, single-exit.

So,

--- a/mm/mmap.c~mm-mmap-simplify-the-failure-return-working-flow-fix
+++ a/mm/mmap.c
@@ -2952,7 +2952,7 @@ struct vm_area_struct *copy_vma(struct v
 	} else {
 		new_vma = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
 		if (!new_vma)
-			return NULL;
+			goto out;
 		*new_vma = *vma;
 		new_vma->vm_start = addr;
 		new_vma->vm_end = addr + len;
@@ -2971,10 +2971,11 @@ struct vm_area_struct *copy_vma(struct v
 	}
 	return new_vma;
 
- out_free_mempol:
+out_free_mempol:
 	mpol_put(vma_policy(new_vma));
- out_free_vma:
+out_free_vma:
 	kmem_cache_free(vm_area_cachep, new_vma);
+out:
 	return NULL;
 }
 
_

--
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/