[PATCH 0/8] signal: refactor some functions

[PATCH 0/8] signal: refactor some functions

[Christian Brauner]

Hey,

This series refactors a bunch of functions in signal.c to simplify parts
of the code.
The greatest single change is declaring the static do_sigpending()
helper as void which makes it possible to remove a bunch of unnecessary
checks in the syscalls later on.

Thanks!
Christian

Christian Brauner (8):
  signal: make force_sigsegv() void
  signal: make kill_as_cred_perm() return bool
  signal: make may_ptrace_stop() return bool
  signal: add copy_pending() helper
  signal: flatten do_send_sig_info()
  signal: drop else branch in do_signal_stop()
  signal: make do_sigpending() void
  signal: simplify rt_sigaction()

 include/linux/sched/signal.h |   2 +-
 kernel/signal.c              | 154 +++++++++++++++++------------------
 2 files changed, 78 insertions(+), 78 deletions(-)

-- 
2.17.0

[PATCH 1/8] signal: make force_sigsegv() void

[Christian Brauner]

force_sigsegv() returned 0 unconditionally so it doesn't make sense to have
it return at all. In addition, there are no callers that check
force_sigsegv()'s return value.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
 include/linux/sched/signal.h | 2 +-
 kernel/signal.c              | 7 ++-----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 113d1ad1ced7..e138ac16c650 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -314,7 +314,7 @@ int force_sig_pkuerr(void __user *addr, u32 pkey);
 int force_sig_ptrace_errno_trap(int errno, void __user *addr);
 
 extern int send_sig_info(int, struct siginfo *, struct task_struct *);
-extern int force_sigsegv(int, struct task_struct *);
+extern void force_sigsegv(int sig, struct task_struct *p);
 extern int force_sig_info(int, struct siginfo *, struct task_struct *);
 extern int __kill_pgrp_info(int sig, struct siginfo *info, struct pid *pgrp);
 extern int kill_pid_info(int sig, struct siginfo *info, struct pid *pid);
diff --git a/kernel/signal.c b/kernel/signal.c
index 9c33163a6165..c756008d589e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1468,8 +1468,7 @@ send_sig(int sig, struct task_struct *p, int priv)
 	return send_sig_info(sig, __si_special(priv), p);
 }
 
-void
-force_sig(int sig, struct task_struct *p)
+void force_sig(int sig, struct task_struct *p)
 {
 	force_sig_info(sig, SEND_SIG_PRIV, p);
 }
@@ -1480,8 +1479,7 @@ force_sig(int sig, struct task_struct *p)
  * the problem was already a SIGSEGV, we'll want to
  * make sure we don't even try to deliver the signal..
  */
-int
-force_sigsegv(int sig, struct task_struct *p)
+void force_sigsegv(int sig, struct task_struct *p)
 {
 	if (sig == SIGSEGV) {
 		unsigned long flags;
@@ -1490,7 +1488,6 @@ force_sigsegv(int sig, struct task_struct *p)
 		spin_unlock_irqrestore(&p->sighand->siglock, flags);
 	}
 	force_sig(SIGSEGV, p);
-	return 0;
 }
 
 int force_sig_fault(int sig, int code, void __user *addr
-- 
2.17.0

[PATCH 2/8] signal: make kill_as_cred_perm() return bool

[Christian Brauner]

kill_as_cred_perm() already behaves like a boolean function. Let's actually
declare it as such too.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
 kernel/signal.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index c756008d589e..81be01d193f4 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1349,14 +1349,15 @@ static int kill_proc_info(int sig, struct siginfo *info, pid_t pid)
 	return error;
 }
 
-static int kill_as_cred_perm(const struct cred *cred,
+static bool kill_as_cred_perm(const struct cred *cred,
 			     struct task_struct *target)
 {
 	const struct cred *pcred = __task_cred(target);
 	if (!uid_eq(cred->euid, pcred->suid) && !uid_eq(cred->euid, pcred->uid) &&
 	    !uid_eq(cred->uid,  pcred->suid) && !uid_eq(cred->uid,  pcred->uid))
-		return 0;
-	return 1;
+		return false;
+
+	return true;
 }
 
 /* like kill_pid_info(), but doesn't use uid/euid of "current" */
-- 
2.17.0

Re: [PATCH 2/8] signal: make kill_as_cred_perm() return bool

[Al Viro]

On Mon, May 28, 2018 at 03:49:10PM +0200, Christian Brauner wrote:
> kill_as_cred_perm() already behaves like a boolean function. Let's actually
> declare it as such too.
> 
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
>  kernel/signal.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index c756008d589e..81be01d193f4 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1349,14 +1349,15 @@ static int kill_proc_info(int sig, struct siginfo *info, pid_t pid)
>  	return error;
>  }
>  
> -static int kill_as_cred_perm(const struct cred *cred,
> +static bool kill_as_cred_perm(const struct cred *cred,
>  			     struct task_struct *target)
>  {
>  	const struct cred *pcred = __task_cred(target);
>  	if (!uid_eq(cred->euid, pcred->suid) && !uid_eq(cred->euid, pcred->uid) &&
>  	    !uid_eq(cred->uid,  pcred->suid) && !uid_eq(cred->uid,  pcred->uid))
> -		return 0;
> -	return 1;
> +		return false;
> +
> +	return true;

Ugh...
	if (!foo && !bar && !baz && !quux)
		return false;
	return true;

is a bloody odd way to spell

	return foo || bar || baz || quux;

Re: [PATCH 2/8] signal: make kill_as_cred_perm() return bool

[Christian Brauner]

On Mon, May 28, 2018 at 03:07:49PM +0100, Al Viro wrote:
> On Mon, May 28, 2018 at 03:49:10PM +0200, Christian Brauner wrote:
> > kill_as_cred_perm() already behaves like a boolean function. Let's actually
> > declare it as such too.
> > 
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> >  kernel/signal.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index c756008d589e..81be01d193f4 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1349,14 +1349,15 @@ static int kill_proc_info(int sig, struct siginfo *info, pid_t pid)
> >  	return error;
> >  }
> >  
> > -static int kill_as_cred_perm(const struct cred *cred,
> > +static bool kill_as_cred_perm(const struct cred *cred,
> >  			     struct task_struct *target)
> >  {
> >  	const struct cred *pcred = __task_cred(target);
> >  	if (!uid_eq(cred->euid, pcred->suid) && !uid_eq(cred->euid, pcred->uid) &&
> >  	    !uid_eq(cred->uid,  pcred->suid) && !uid_eq(cred->uid,  pcred->uid))
> > -		return 0;
> > -	return 1;
> > +		return false;
> > +
> > +	return true;
> 
> Ugh...
> 	if (!foo && !bar && !baz && !quux)
> 		return false;
> 	return true;
> 
> is a bloody odd way to spell
> 
> 	return foo || bar || baz || quux;

Added in v1.
There's a bunch more functions that should return bool and that can
similarly simplified which I'll send out in the next round.

Thanks!
Christian

[PATCH 3/8] signal: make may_ptrace_stop() return bool

[Christian Brauner]

may_ptrace_stop() already behaves like a boolean function. Let's actually
declare it as such too.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
 kernel/signal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 81be01d193f4..6c2e7b45cba1 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1889,10 +1889,10 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
 	spin_unlock_irqrestore(&sighand->siglock, flags);
 }
 
-static inline int may_ptrace_stop(void)
+static inline bool may_ptrace_stop(void)
 {
 	if (!likely(current->ptrace))
-		return 0;
+		return false;
 	/*
 	 * Are we in the middle of do_coredump?
 	 * If so and our tracer is also part of the coredump stopping
@@ -1908,9 +1908,9 @@ static inline int may_ptrace_stop(void)
 	 */
 	if (unlikely(current->mm->core_state) &&
 	    unlikely(current->mm == current->parent->mm))
-		return 0;
+		return false;
 
-	return 1;
+	return true;
 }
 
 /*
-- 
2.17.0

Re: [PATCH 3/8] signal: make may_ptrace_stop() return bool

[Al Viro]

On Mon, May 28, 2018 at 03:49:11PM +0200, Christian Brauner wrote:
> may_ptrace_stop() already behaves like a boolean function. Let's actually
> declare it as such too.
> 
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
>  kernel/signal.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 81be01d193f4..6c2e7b45cba1 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1889,10 +1889,10 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>  	spin_unlock_irqrestore(&sighand->siglock, flags);
>  }
>  
> -static inline int may_ptrace_stop(void)
> +static inline bool may_ptrace_stop(void)
>  {
>  	if (!likely(current->ptrace))
> -		return 0;
> +		return false;
>  	/*
>  	 * Are we in the middle of do_coredump?
>  	 * If so and our tracer is also part of the coredump stopping
> @@ -1908,9 +1908,9 @@ static inline int may_ptrace_stop(void)
>  	 */
>  	if (unlikely(current->mm->core_state) &&
>  	    unlikely(current->mm == current->parent->mm))
> -		return 0;
> +		return false;
>  
> -	return 1;
> +	return true;

	return !current->mm->core_state || current->mm != current->parent->mm;

or, if it gives any measurably better code generation,

	return likely(!current->mm->core_state ||
			current->mm != current->parent->mm);

Re: [PATCH 3/8] signal: make may_ptrace_stop() return bool

[Christian Brauner]

On Mon, May 28, 2018 at 03:12:19PM +0100, Al Viro wrote:
> On Mon, May 28, 2018 at 03:49:11PM +0200, Christian Brauner wrote:
> > may_ptrace_stop() already behaves like a boolean function. Let's actually
> > declare it as such too.
> > 
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> >  kernel/signal.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 81be01d193f4..6c2e7b45cba1 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1889,10 +1889,10 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
> >  	spin_unlock_irqrestore(&sighand->siglock, flags);
> >  }
> >  
> > -static inline int may_ptrace_stop(void)
> > +static inline bool may_ptrace_stop(void)
> >  {
> >  	if (!likely(current->ptrace))
> > -		return 0;
> > +		return false;
> >  	/*
> >  	 * Are we in the middle of do_coredump?
> >  	 * If so and our tracer is also part of the coredump stopping
> > @@ -1908,9 +1908,9 @@ static inline int may_ptrace_stop(void)
> >  	 */
> >  	if (unlikely(current->mm->core_state) &&
> >  	    unlikely(current->mm == current->parent->mm))
> > -		return 0;
> > +		return false;
> >  
> > -	return 1;
> > +	return true;
> 
> 	return !current->mm->core_state || current->mm != current->parent->mm;
> 
> or, if it gives any measurably better code generation,
> 
> 	return likely(!current->mm->core_state ||
> 			current->mm != current->parent->mm);

Makes sense. I put this in v1.

Thanks!
Christian

[PATCH 4/8] signal: add copy_pending() helper

[Christian Brauner]

Instead of using a goto for this let's add a simple helper copy_pending()
which can be called in both places.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
 kernel/signal.c | 54 +++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 6c2e7b45cba1..821b54e3328b 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -515,6 +515,19 @@ int unhandled_signal(struct task_struct *tsk, int sig)
 	return !tsk->ptrace;
 }
 
+static void copy_pending(siginfo_t *info, struct sigqueue *first,
+			 bool *resched_timer)
+{
+	list_del_init(&first->list);
+	copy_siginfo(info, &first->info);
+
+	*resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
+			 (info->si_code == SI_TIMER) &&
+			 (info->si_sys_private);
+
+	__sigqueue_free(first);
+}
+
 static void collect_signal(int sig, struct sigpending *list, siginfo_t *info,
 			   bool *resched_timer)
 {
@@ -526,8 +539,10 @@ static void collect_signal(int sig, struct sigpending *list, siginfo_t *info,
 	*/
 	list_for_each_entry(q, &list->list, list) {
 		if (q->info.si_signo == sig) {
-			if (first)
-				goto still_pending;
+			if (first) {
+				copy_pending(info, first, resched_timer);
+				return;
+			}
 			first = q;
 		}
 	}
@@ -535,29 +550,20 @@ static void collect_signal(int sig, struct sigpending *list, siginfo_t *info,
 	sigdelset(&list->signal, sig);
 
 	if (first) {
-still_pending:
-		list_del_init(&first->list);
-		copy_siginfo(info, &first->info);
-
-		*resched_timer =
-			(first->flags & SIGQUEUE_PREALLOC) &&
-			(info->si_code == SI_TIMER) &&
-			(info->si_sys_private);
-
-		__sigqueue_free(first);
-	} else {
-		/*
-		 * Ok, it wasn't in the queue.  This must be
-		 * a fast-pathed signal or we must have been
-		 * out of queue space.  So zero out the info.
-		 */
-		clear_siginfo(info);
-		info->si_signo = sig;
-		info->si_errno = 0;
-		info->si_code = SI_USER;
-		info->si_pid = 0;
-		info->si_uid = 0;
+		copy_pending(info, first, resched_timer);
+		return;
 	}
+
+	/*
+	 * Ok, it wasn't in the queue. This must be a fast-pathed signal or we
+	 * must have been out of queue space. So zero out the info.
+	 */
+	clear_siginfo(info);
+	info->si_signo = sig;
+	info->si_errno = 0;
+	info->si_code = SI_USER;
+	info->si_pid = 0;
+	info->si_uid = 0;
 }
 
 static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
-- 
2.17.0

[PATCH 5/8] signal: flatten do_send_sig_info()

[Christian Brauner]

Let's return early when lock_task_sighand() fails and move send_signal()
and unlock_task_sighand() out of the if block.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
 kernel/signal.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 821b54e3328b..498b464fe862 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1167,16 +1167,16 @@ specific_send_sig_info(int sig, struct siginfo *info, struct task_struct *t)
 }
 
 int do_send_sig_info(int sig, struct siginfo *info, struct task_struct *p,
-			bool group)
+		     bool group)
 {
 	unsigned long flags;
 	int ret = -ESRCH;
 
-	if (lock_task_sighand(p, &flags)) {
-		ret = send_signal(sig, info, p, group);
-		unlock_task_sighand(p, &flags);
-	}
+	if (!lock_task_sighand(p, &flags))
+		return ret;
 
+	ret = send_signal(sig, info, p, group);
+	unlock_task_sighand(p, &flags);
 	return ret;
 }
 
-- 
2.17.0

Re: [PATCH 5/8] signal: flatten do_send_sig_info()

[Al Viro]

On Mon, May 28, 2018 at 03:49:13PM +0200, Christian Brauner wrote:
> Let's return early when lock_task_sighand() fails and move send_signal()
> and unlock_task_sighand() out of the if block.

Why is it an improvement?

Re: [PATCH 5/8] signal: flatten do_send_sig_info()

[Christian Brauner]

On Mon, May 28, 2018 at 03:15:37PM +0100, Al Viro wrote:
> On Mon, May 28, 2018 at 03:49:13PM +0200, Christian Brauner wrote:
> > Let's return early when lock_task_sighand() fails and move send_signal()
> > and unlock_task_sighand() out of the if block.
> 
> Why is it an improvement?

I prefer to return early if I know I can and have the bigger portion of
the code move out of the if-branch. But that's - let's say - an
opinionated improvement.

Christian

[PATCH 6/8] signal: drop else branch in do_signal_stop()

[Christian Brauner]

do_signal_stop() already returns in the if branch so there's no need to
keep the else branch around.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
 kernel/signal.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 498b464fe862..3b7712ec0746 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

[PATCH 7/8] signal: make do_sigpending() void

[Christian Brauner]

do_sigpending() returned 0 unconditionally so it doesn't make sense to have
it return at all. This allows us to simplify a bunch of syscall callers.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
 kernel/signal.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 3b7712ec0746..42a7b067a49c 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2773,7 +2773,7 @@ COMPAT_SYSCALL_DEFINE4(rt_sigprocmask, int, how, compat_sigset_t __user *, nset,
 }
 #endif
 
-static int do_sigpending(sigset_t *set)
+static void do_sigpending(sigset_t *set)
 {
 	spin_lock_irq(&current->sighand->siglock);
 	sigorsets(set, &current->pending.signal,
@@ -2782,7 +2782,6 @@ static int do_sigpending(sigset_t *set)
 
 	/* Outside the lock because only this thread touches it.  */
 	sigandsets(set, &current->blocked, set);
-	return 0;
 }
 
 /**
@@ -2794,15 +2793,16 @@ static int do_sigpending(sigset_t *set)
 SYSCALL_DEFINE2(rt_sigpending, sigset_t __user *, uset, size_t, sigsetsize)
 {
 	sigset_t set;
-	int err;
 
 	if (sigsetsize > sizeof(*uset))
 		return -EINVAL;
 
-	err = do_sigpending(&set);
-	if (!err && copy_to_user(uset, &set, sigsetsize))
-		err = -EFAULT;
-	return err;
+	do_sigpending(&set);
+
+	if (copy_to_user(uset, &set, sigsetsize))
+		return -EFAULT;
+
+	return 0;
 }
 
 #ifdef CONFIG_COMPAT
@@ -2810,15 +2810,13 @@ COMPAT_SYSCALL_DEFINE2(rt_sigpending, compat_sigset_t __user *, uset,
 		compat_size_t, sigsetsize)
 {
 	sigset_t set;
-	int err;
 
 	if (sigsetsize > sizeof(*uset))
 		return -EINVAL;
 
-	err = do_sigpending(&set);
-	if (!err)
-		err = put_compat_sigset(uset, &set, sigsetsize);
-	return err;
+	do_sigpending(&set);
+
+	return put_compat_sigset(uset, &set, sigsetsize);
 }
 #endif
 
@@ -3653,25 +3651,26 @@ int __compat_save_altstack(compat_stack_t __user *uss, unsigned long sp)
 SYSCALL_DEFINE1(sigpending, old_sigset_t __user *, uset)
 {
 	sigset_t set;
-	int err;
 
 	if (sizeof(old_sigset_t) > sizeof(*uset))
 		return -EINVAL;
 
-	err = do_sigpending(&set);
-	if (!err && copy_to_user(uset, &set, sizeof(old_sigset_t)))
-		err = -EFAULT;
-	return err;
+	do_sigpending(&set);
+
+	if (copy_to_user(uset, &set, sizeof(old_sigset_t)))
+		return -EFAULT;
+
+	return 0;
 }
 
 #ifdef CONFIG_COMPAT
 COMPAT_SYSCALL_DEFINE1(sigpending, compat_old_sigset_t __user *, set32)
 {
 	sigset_t set;
-	int err = do_sigpending(&set);
-	if (!err)
-		err = put_user(set.sig[0], set32);
-	return err;
+
+	do_sigpending(&set);
+
+	return put_user(set.sig[0], set32);
 }
 #endif
 
-- 
2.17.0

Re: [PATCH 7/8] signal: make do_sigpending() void

[Al Viro]

On Mon, May 28, 2018 at 03:49:15PM +0200, Christian Brauner wrote:
> do_sigpending() returned 0 unconditionally so it doesn't make sense to have
> it return at all. This allows us to simplify a bunch of syscall callers.

Yeah, that should've been done in "signal: lift sigset size check out of
do_sigpending()" last year; missed that on review back then.
Acked-by: Al Viro <viro@zeniv.linux.org.uk>

[PATCH 8/8] signal: simplify rt_sigaction()

[Christian Brauner]

The goto is not needed and does not add any clarity. Simply return -EINVAL
on unexpected sigset_t struct size directly.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
 kernel/signal.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 42a7b067a49c..96c58de43ddf 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3741,24 +3741,21 @@ SYSCALL_DEFINE4(rt_sigaction, int, sig,
 		size_t, sigsetsize)
 {
 	struct k_sigaction new_sa, old_sa;
-	int ret = -EINVAL;
+	int ret;
 
 	/* XXX: Don't preclude handling different sized sigset_t's.  */
 	if (sigsetsize != sizeof(sigset_t))
-		goto out;
+		return -EINVAL;
 
-	if (act) {
+	if (act)
 		if (copy_from_user(&new_sa.sa, act, sizeof(new_sa.sa)))
 			return -EFAULT;
-	}
 
 	ret = do_sigaction(sig, act ? &new_sa : NULL, oact ? &old_sa : NULL);
-
-	if (!ret && oact) {
+	if (!ret && oact)
 		if (copy_to_user(oact, &old_sa.sa, sizeof(old_sa.sa)))
 			return -EFAULT;
-	}
-out:
+
 	return ret;
 }
 #ifdef CONFIG_COMPAT
-- 
2.17.0

Re: [PATCH 8/8] signal: simplify rt_sigaction()

[Christoph Hellwig]

> +	if (act)
>  		if (copy_from_user(&new_sa.sa, act, sizeof(new_sa.sa)))
>  			return -EFAULT;

	if (act && copy_from_user(&new_sa.sa, act, sizeof(new_sa.sa)))
		return -EFAULT;

>  	ret = do_sigaction(sig, act ? &new_sa : NULL, oact ? &old_sa : NULL);
> -
> -	if (!ret && oact) {
> +	if (!ret && oact)
>  		if (copy_to_user(oact, &old_sa.sa, sizeof(old_sa.sa)))
>  			return -EFAULT;
> -	}

	ret = do_sigaction(sig, act ? &new_sa : NULL, oact ? &old_sa : NULL);
	if (!ret && oact && copy_to_user(oact, &old_sa.sa, sizeof(old_sa.sa)))
		return -EFAULT;

Althought I'd personaly write it as:

	ret = do_sigaction(sig, act ? &new_sa : NULL, oact ? &old_sa : NULL);
	if (ret)
		return ret;
	if (oact && copy_to_user(oact, &old_sa.sa, sizeof(old_sa.sa)))
		return -EFAULT;
	return 0;

Re: [PATCH 8/8] signal: simplify rt_sigaction()

[Christian Brauner]

On Mon, May 28, 2018 at 11:47:19PM -0700, Christoph Hellwig wrote:
> > +	if (act)
> >  		if (copy_from_user(&new_sa.sa, act, sizeof(new_sa.sa)))
> >  			return -EFAULT;
> 
> 	if (act && copy_from_user(&new_sa.sa, act, sizeof(new_sa.sa)))
> 		return -EFAULT;

Yes, I agree and should have been bolder in making that change. But I
already wasn't sure how ok people would be with:

if (act) {} --> if (act)

> 
> >  	ret = do_sigaction(sig, act ? &new_sa : NULL, oact ? &old_sa : NULL);
> > -
> > -	if (!ret && oact) {
> > +	if (!ret && oact)
> >  		if (copy_to_user(oact, &old_sa.sa, sizeof(old_sa.sa)))
> >  			return -EFAULT;
> > -	}
> 
> 	ret = do_sigaction(sig, act ? &new_sa : NULL, oact ? &old_sa : NULL);
> 	if (!ret && oact && copy_to_user(oact, &old_sa.sa, sizeof(old_sa.sa)))
> 		return -EFAULT;
> 
> Althought I'd personaly write it as:
> 
> 	ret = do_sigaction(sig, act ? &new_sa : NULL, oact ? &old_sa : NULL);
> 	if (ret)
> 		return ret;
> 	if (oact && copy_to_user(oact, &old_sa.sa, sizeof(old_sa.sa)))
> 		return -EFAULT;
> 	return 0;

Yeah, I can put that in v2.
I'd wait for a little until people have commmented/acked some of the
other changes so that I don't keep spamming inboxes. If you have a few
spare minutes it would be cool if you could take a quick look. It
shouldn't take long.

Thanks!
Christian