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

Date:	Wed, 27 Nov 2013 14:28:06 +0900
From:	DaeSeok Youn <daeseok.youn@...il.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, oleg <oleg@...hat.com>,
	viro <viro@...iv.linux.org.uk>, luto <luto@...capital.net>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	kernel-janitors <kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH] kernel/fork.c : remove local 'oldmm' and retval

Sorry.
my e-mail was rejected by vger.kernel.org server.
resend my e-mail.

2013/11/27 DaeSeok Youn <daeseok.youn@...il.com>
>
> Thank you for reviewing my patch.
>
> I am a newbie who want to know kernel source deeply. :)
>
> I agree your comments but I want to explain my patch why it is sent.
>
> I think "ENOMEM" is enough to explain to fail by dup_mm().
> So I replace the goto line to "return -ENOMEM" in if(!mm).
>
> And I think oldmm doesn't use any meaning control in copy_mm(),
> so re-use local 'mm' variable. If the source has less comprehensible,
> how about adding a comment like 'parent and child share address space'
> above "mm = current->mm" in my patch.
>
> just my opinion, let me know if i am wrong.
> Actually I need to learn about kernel source code.
>
> Thanks.
> Daeseok Youn.
>
>
>
> 2013/11/27 Eric W. Biederman <ebiederm@...ssion.com>
>
>> 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/