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

[Andrew Morton]

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

> From: Chen Gang <xili_gchen_5257@xxxxxxxxxxx>

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

This is peculiar.  I'm assuming that it should have been From:you@gmail 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/