linux-kernel - Re: [PATCH] kernel/fork.c : remove local 'oldmm' and retval

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:	Tue, 26 Nov 2013 19:24:11 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Daeseok Youn <daeseok.youn@...il.com>
Cc:	akpm@...ux-foundation.org, oleg@...hat.com,
	viro@...iv.linux.org.uk, luto@...capital.net,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] kernel/fork.c : remove local 'oldmm' and retval

Daeseok Youn <daeseok.youn@...il.com> writes:

> From cec2f201f0dc99a33a58d9d1e0452140bb0993a1 Mon Sep 17 00:00:00 2001
> From: Daeseok Youn <daeseok.youn@....com>
> Date: Wed, 27 Nov 2013 09:54:41 +0900
> Subject: [PATCH] kernel/fork.c : remove local 'oldmm' and retval
>
>  Local oldmm is used only for increaing mm_users field
>  in current->mm. When clone_flags have a CLONE_VM flag,
>  current->mm is assigning to local 'mm'.
>  Local retval is used only for returning -ENOMEM value.
>  When dup_mm() is failed, just return -ENOMEM.

You are making the generated code worse, and the source less
comprehensible.

You are adding additional exit points making it harder to analyze the
function.

You are introducing races and expense by not caching current->mm in
oldmm.

This looks like code churn for no good reason, and that will result in
worse code.

ick.

Eric

>  Signed-off-by: Daeseok Youn <daeseok.youn@...il.com>
> ---
>  kernel/fork.c |   16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 728d5be..022a0af 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -857,8 +857,7 @@ fail_nocontext:
>  
>  static int copy_mm(unsigned long clone_flags, struct task_struct *tsk)
>  {
> -	struct mm_struct *mm, *oldmm;
> -	int retval;
> +	struct mm_struct *mm;
>  
>  	tsk->min_flt = tsk->maj_flt = 0;
>  	tsk->nvcsw = tsk->nivcsw = 0;
> @@ -874,28 +873,23 @@ static int copy_mm(unsigned long clone_flags, struct task_struct *tsk)
>  	 *
>  	 * We need to steal a active VM for that..
>  	 */
> -	oldmm = current->mm;
> -	if (!oldmm)
> +	if (!current->mm)
>  		return 0;
>  
>  	if (clone_flags & CLONE_VM) {
> -		atomic_inc(&oldmm->mm_users);
> -		mm = oldmm;
> +		mm = current->mm;
> +		atomic_inc(&mm->mm_users);
>  		goto good_mm;
>  	}
>  
> -	retval = -ENOMEM;
>  	mm = dup_mm(tsk);
>  	if (!mm)
> -		goto fail_nomem;
> +		return -ENOMEM;
>  
>  good_mm:
>  	tsk->mm = mm;
>  	tsk->active_mm = mm;
>  	return 0;
> -
> -fail_nomem:
> -	return retval;
>  }
>  
>  static int copy_fs(unsigned long clone_flags, struct task_struct *tsk)
--
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/