linux-kernel - Re: [PATCH v1 06/20] signal: drop else branch in do_signal

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, 29 May 2018 17:06:59 +0200
From:   Christian Brauner <christian@...uner.io>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     linux-kernel@...r.kernel.org, ebiederm@...ssion.com,
        gregkh@...uxfoundation.org, mingo@...nel.org,
        james.morris@...rosoft.com, keescook@...omium.org,
        peterz@...radead.org, sds@...ho.nsa.gov, viro@...iv.linux.org.uk,
        akpm@...ux-foundation.org
Subject: Re: [PATCH v1 06/20] signal: drop else branch in do_signal_stop()

On Tue, May 29, 2018 at 04:30:40PM +0200, Oleg Nesterov wrote:
> I am busy now, can't review, just picked a random patch from this series...
> 
> On 05/28, Christian Brauner wrote:
> > do_signal_stop() already returns in the if branch so there's no need to
> > keep the else branch around.
> 
> OK, but for what???

Yes to both.

> 
> Do you think this change makes the code more readable? more clean? or what?
> 
> I do not really care but to me these "if/else" branches make this code more
> symmetrical, so I don't understand the purpose.

The pattern where both the if and the else branch return is fairly
uncommon even in this file. Otherwise you would need to also argue that
functions like:

int send_sig_info(int sig, struct siginfo *info, struct task_struct *p)
{
        /*
         * Make sure legacy kernel users don't send in bad values
         * (normal paths check this in check_kill_permission).
         */
        if (!valid_signal(sig))
                return -EINVAL;

        return do_send_sig_info(sig, info, p, false);
}

should really be:

int send_sig_info(int sig, struct siginfo *info, struct task_struct *p)
{
        /*
         * Make sure legacy kernel users don't send in bad values
         * (normal paths check this in check_kill_permission).
         */
        if (!valid_signal(sig))
                return -EINVAL;
        else
                return do_send_sig_info(sig, info, p, false);
}

Which I find very uneasy on the eye. Even more so when there are
multiple lines in the else that returns.

Christian

> 
> 
> 
> > Signed-off-by: Christian Brauner <christian@...uner.io>
> > ---
> > v0->v1:
> > * patch unchanged
> > ---
> >  kernel/signal.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index a628b56415e6..d1914439f144 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -2214,14 +2214,14 @@ static bool do_signal_stop(int signr)
> >  		/* Now we don't run again until woken by SIGCONT or SIGKILL */
> >  		freezable_schedule();
> >  		return true;
> > -	} else {
> > -		/*
> > -		 * While ptraced, group stop is handled by STOP trap.
> > -		 * Schedule it and let the caller deal with it.
> > -		 */
> > -		task_set_jobctl_pending(current, JOBCTL_TRAP_STOP);
> > -		return false;
> >  	}
> > +
> > +	/*
> > +	 * While ptraced, group stop is handled by STOP trap.
> > +	 * Schedule it and let the caller deal with it.
> > +	 */
> > +	task_set_jobctl_pending(current, JOBCTL_TRAP_STOP);
> > +	return false;
> >  }
> >  
> >  /**
> > -- 
> > 2.17.0
> > 
>