[PATCH v1 00/20] signal: refactor some functions

[PATCH v1 00/20] signal: refactor some functions

[Christian Brauner]

Hey,

This series refactors a bunch of functions in signal.c to simplify parts
of the code.
As requested, v2 drops all 3 commits that were pure coding-style
changes. The only other coding-style change is the simplification of
rt_sigaction which seemed to be worth keeping.
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 (17):
  signal: make force_sigsegv() void
  signal: make kill_as_cred_perm() return bool
  signal: make may_ptrace_stop() return bool
  signal: make do_sigpending() void
  signal: simplify rt_sigaction()
  signal: make kill_ok_by_cred() return bool
  signal: make sig_handler_ignored() return bool
  signal: make sig_task_ignored() return bool
  signal: make sig_ignored() return bool
  signal: make has_pending_signals() return bool
  signal: make recalc_sigpending_tsk() return bool
  signal: make unhandled_signal() return bool
  signal: make flush_sigqueue_mask() void
  signal: make wants_signal() return bool
  signal: make legacy_queue() return bool
  signal: make security_task_kill() return bool
  signal: make get_signal() return bool

 include/linux/sched/signal.h |   2 +-
 include/linux/signal.h       |   4 +-
 kernel/signal.c              | 200 +++++++++++++++++------------------
 3 files changed, 103 insertions(+), 103 deletions(-)

-- 
2.17.0

[PATCH v2 01/17] 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>
---
v1->v2:
* unchanged
v0->v1:
* unchanged
---
 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 v2 02/17] 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>
---
v1->v2:
* unchanged
v0->v1:
* simplify to use a single return
---
 kernel/signal.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index c756008d589e..6c71d6b8d2b8 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,
-			     struct task_struct *target)
+static inline 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 uid_eq(cred->euid, pcred->suid) ||
+	       uid_eq(cred->euid, pcred->uid) ||
+	       uid_eq(cred->uid, pcred->suid) ||
+	       uid_eq(cred->uid, pcred->uid);
 }
 
 /* like kill_pid_info(), but doesn't use uid/euid of "current" */
-- 
2.17.0

[PATCH v2 03/17] 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>
---
v1->v2:
* unchanged
v0->v1:
* unchanged
---
 kernel/signal.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 6c71d6b8d2b8..bc750fb4ddcc 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
@@ -1906,11 +1906,11 @@ static inline int may_ptrace_stop(void)
 	 * block in TASK_TRACED. But PTRACE_EVENT_EXIT can be reported
 	 * after SIGKILL was already dequeued.
 	 */
-	if (unlikely(current->mm->core_state) &&
-	    unlikely(current->mm == current->parent->mm))
-		return 0;
+	if (likely(!current->mm->core_state ||
+		   current->mm != current->parent->mm))
+		return false;
 
-	return 1;
+	return true;
 }
 
 /*
-- 
2.17.0

Re: [PATCH v2 03/17] signal: make may_ptrace_stop() return bool

[Oleg Nesterov]

On 06/01, Christian Brauner wrote:
>
> may_ptrace_stop() already behaves like a boolean function. Let's actually
> declare it as such too.

OK. Then probably this patch should only make it return bool and do nothing else?

> -	if (unlikely(current->mm->core_state) &&
> -	    unlikely(current->mm == current->parent->mm))
> -		return 0;
> +	if (likely(!current->mm->core_state ||
> +		   current->mm != current->parent->mm))
> +		return false;

this is wrong.

Oleg.

Re: [PATCH v2 03/17] signal: make may_ptrace_stop() return bool

[Christian Brauner]

On Fri, Jun 01, 2018 at 05:56:12PM +0200, Oleg Nesterov wrote:
> On 06/01, Christian Brauner wrote:
> >
> > may_ptrace_stop() already behaves like a boolean function. Let's actually
> > declare it as such too.
> 
> OK. Then probably this patch should only make it return bool and do nothing else?
> 
> > -	if (unlikely(current->mm->core_state) &&
> > -	    unlikely(current->mm == current->parent->mm))
> > -		return 0;
> > +	if (likely(!current->mm->core_state ||
> > +		   current->mm != current->parent->mm))
> > +		return false;
> 
> this is wrong.

This was a suggestion by Al. I just took it over and may have messed it
up while doing so. I'll remove this.

Christian

[PATCH v2 04/17] 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>
Acked-by: Al Viro <viro@zeniv.linux.org.uk>
---
v1->v2:
* unchanged
v0->v1:
* unchanged
---
 kernel/signal.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index bc750fb4ddcc..7c770556b085 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2767,7 +2767,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,
@@ -2776,7 +2776,6 @@ static int do_sigpending(sigset_t *set)
 
 	/* Outside the lock because only this thread touches it.  */
 	sigandsets(set, &current->blocked, set);
-	return 0;
 }
 
 /**
@@ -2788,15 +2787,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
@@ -2804,15 +2804,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
 
@@ -3647,25 +3645,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 v2 04/17] signal: make do_sigpending() void

[Oleg Nesterov]

On 06/01, 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.

Agreed, ACK

[PATCH v2 05/17] 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>
---
v1->v2:
* [Christoph Hellwig] additional cleanups
v0->v1:
* unchanged
---
 kernel/signal.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 7c770556b085..00ed69d33282 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3735,25 +3735,23 @@ 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 (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)
+		return ret;
 
-	if (!ret && oact) {
-		if (copy_to_user(oact, &old_sa.sa, sizeof(old_sa.sa)))
-			return -EFAULT;
-	}
-out:
-	return ret;
+	if (oact && copy_to_user(oact, &old_sa.sa, sizeof(old_sa.sa)))
+		return -EFAULT;
+
+	return 0;
 }
 #ifdef CONFIG_COMPAT
 COMPAT_SYSCALL_DEFINE4(rt_sigaction, int, sig,
-- 
2.17.0

Re: [PATCH v2 05/17] signal: simplify rt_sigaction()

[Oleg Nesterov]

On 06/01, Christian Brauner wrote:
>
> The goto is not needed and does not add any clarity. Simply return -EINVAL
> on unexpected sigset_t struct size directly.

Agreed, sys_rt_sigaction() looks just ugly and mixes goto/return on failure,

ACK

[PATCH v2 06/17] signal: make kill_ok_by_cred() return bool

[Christian Brauner]

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

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* unchanged
---
 kernel/signal.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 00ed69d33282..bbd15e984a84 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -717,21 +717,16 @@ static inline bool si_fromuser(const struct siginfo *info)
 /*
  * called with RCU read lock from check_kill_permission()
  */
-static int kill_ok_by_cred(struct task_struct *t)
+static bool kill_ok_by_cred(struct task_struct *t)
 {
 	const struct cred *cred = current_cred();
 	const struct cred *tcred = __task_cred(t);
 
-	if (uid_eq(cred->euid, tcred->suid) ||
-	    uid_eq(cred->euid, tcred->uid)  ||
-	    uid_eq(cred->uid,  tcred->suid) ||
-	    uid_eq(cred->uid,  tcred->uid))
-		return 1;
-
-	if (ns_capable(tcred->user_ns, CAP_KILL))
-		return 1;
-
-	return 0;
+	return uid_eq(cred->euid, tcred->suid) ||
+	       uid_eq(cred->euid, tcred->uid) ||
+	       uid_eq(cred->uid, tcred->suid) ||
+	       uid_eq(cred->uid, tcred->uid) ||
+	       ns_capable(tcred->user_ns, CAP_KILL);
 }
 
 /*
-- 
2.17.0

[PATCH v2 07/17] signal: make sig_handler_ignored() return bool

[Christian Brauner]

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

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* patch added
---
 kernel/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index bbd15e984a84..62e2586f961e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -65,11 +65,11 @@ static void __user *sig_handler(struct task_struct *t, int sig)
 	return t->sighand->action[sig - 1].sa.sa_handler;
 }
 
-static int sig_handler_ignored(void __user *handler, int sig)
+static inline bool sig_handler_ignored(void __user *handler, int sig)
 {
 	/* Is it explicitly or implicitly ignored? */
 	return handler == SIG_IGN ||
-		(handler == SIG_DFL && sig_kernel_ignore(sig));
+	       (handler == SIG_DFL && sig_kernel_ignore(sig));
 }
 
 static int sig_task_ignored(struct task_struct *t, int sig, bool force)
-- 
2.17.0

[PATCH v2 08/17] signal: make sig_task_ignored() return bool

[Christian Brauner]

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

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* patch added
---
 kernel/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 62e2586f961e..208e99b57e6f 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -72,7 +72,7 @@ static inline bool sig_handler_ignored(void __user *handler, int sig)
 	       (handler == SIG_DFL && sig_kernel_ignore(sig));
 }
 
-static int sig_task_ignored(struct task_struct *t, int sig, bool force)
+static bool sig_task_ignored(struct task_struct *t, int sig, bool force)
 {
 	void __user *handler;
 
@@ -80,7 +80,7 @@ static int sig_task_ignored(struct task_struct *t, int sig, bool force)
 
 	if (unlikely(t->signal->flags & SIGNAL_UNKILLABLE) &&
 	    handler == SIG_DFL && !(force && sig_kernel_only(sig)))
-		return 1;
+		return true;
 
 	return sig_handler_ignored(handler, sig);
 }
-- 
2.17.0

[PATCH v2 09/17] signal: make sig_ignored() return bool

[Christian Brauner]

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

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* patch added
---
 kernel/signal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 208e99b57e6f..e48675bd3be8 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -85,7 +85,7 @@ static bool sig_task_ignored(struct task_struct *t, int sig, bool force)
 	return sig_handler_ignored(handler, sig);
 }
 
-static int sig_ignored(struct task_struct *t, int sig, bool force)
+static bool sig_ignored(struct task_struct *t, int sig, bool force)
 {
 	/*
 	 * Blocked signals are never ignored, since the
@@ -93,7 +93,7 @@ static int sig_ignored(struct task_struct *t, int sig, bool force)
 	 * unblocked.
 	 */
 	if (sigismember(&t->blocked, sig) || sigismember(&t->real_blocked, sig))
-		return 0;
+		return false;
 
 	/*
 	 * Tracers may want to know about even ignored signal unless it
@@ -101,7 +101,7 @@ static int sig_ignored(struct task_struct *t, int sig, bool force)
 	 * by SIGNAL_UNKILLABLE task.
 	 */
 	if (t->ptrace && sig != SIGKILL)
-		return 0;
+		return false;
 
 	return sig_task_ignored(t, sig, force);
 }
-- 
2.17.0

[PATCH v2 10/17] signal: make has_pending_signals() return bool

[Christian Brauner]

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

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* patch added
---
 kernel/signal.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index e48675bd3be8..faf38b46d124 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -110,30 +110,33 @@ static bool sig_ignored(struct task_struct *t, int sig, bool force)
  * Re-calculate pending state from the set of locally pending
  * signals, globally pending signals, and blocked signals.
  */
-static inline int has_pending_signals(sigset_t *signal, sigset_t *blocked)
+static inline bool has_pending_signals(sigset_t *signal, sigset_t *blocked)
 {
 	unsigned long ready;
 	long i;
 
 	switch (_NSIG_WORDS) {
 	default:
-		for (i = _NSIG_WORDS, ready = 0; --i >= 0 ;)
-			ready |= signal->sig[i] &~ blocked->sig[i];
+		for (i = _NSIG_WORDS, ready = 0; --i >= 0;)
+			ready |= signal->sig[i] & ~blocked->sig[i];
 		break;
 
-	case 4: ready  = signal->sig[3] &~ blocked->sig[3];
-		ready |= signal->sig[2] &~ blocked->sig[2];
-		ready |= signal->sig[1] &~ blocked->sig[1];
-		ready |= signal->sig[0] &~ blocked->sig[0];
+	case 4:
+		ready = signal->sig[3] & ~blocked->sig[3];
+		ready |= signal->sig[2] & ~blocked->sig[2];
+		ready |= signal->sig[1] & ~blocked->sig[1];
+		ready |= signal->sig[0] & ~blocked->sig[0];
 		break;
 
-	case 2: ready  = signal->sig[1] &~ blocked->sig[1];
-		ready |= signal->sig[0] &~ blocked->sig[0];
+	case 2:
+		ready = signal->sig[1] & ~blocked->sig[1];
+		ready |= signal->sig[0] & ~blocked->sig[0];
 		break;
 
-	case 1: ready  = signal->sig[0] &~ blocked->sig[0];
+	case 1:
+		ready = signal->sig[0] & ~blocked->sig[0];
 	}
-	return ready !=	0;
+	return ready != 0;
 }
 
 #define PENDING(p,b) has_pending_signals(&(p)->signal, (b))
-- 
2.17.0

Re: [PATCH v2 10/17] signal: make has_pending_signals() return bool

[Oleg Nesterov]

On 06/01, Christian Brauner wrote:
>
> has_pending_signals() already behaves like a boolean function. Let's
> actually declare it as such too.

But this patch does more.

> -	case 4: ready  = signal->sig[3] &~ blocked->sig[3];
> -		ready |= signal->sig[2] &~ blocked->sig[2];
> -		ready |= signal->sig[1] &~ blocked->sig[1];
> -		ready |= signal->sig[0] &~ blocked->sig[0];
> +	case 4:
> +		ready = signal->sig[3] & ~blocked->sig[3];
> +		ready |= signal->sig[2] & ~blocked->sig[2];
> +		ready |= signal->sig[1] & ~blocked->sig[1];
> +		ready |= signal->sig[0] & ~blocked->sig[0];
>  		break;

Again, personally I do not care at all. But why do you think the code looks
better after re-formatting? This is subjective, but to me it does not.

In particular, note the extra space before "=" removed by this patch. I guess
it was added on purpose, and to me

	ready  = signal->sig[3] &~ blocked->sig[3];
	ready |= signal->sig[2] &~ blocked->sig[2];

actually looks better thab

	ready = signal->sig[3] &~ blocked->sig[3];
	ready |= signal->sig[2] &~ blocked->sig[2];

after your patch.

Oleg.

Re: [PATCH v2 10/17] signal: make has_pending_signals() return bool

[Christian Brauner]

On Fri, Jun 01, 2018 at 06:16:58PM +0200, Oleg Nesterov wrote:
> On 06/01, Christian Brauner wrote:
> >
> > has_pending_signals() already behaves like a boolean function. Let's
> > actually declare it as such too.
> 
> But this patch does more.
> 
> > -	case 4: ready  = signal->sig[3] &~ blocked->sig[3];
> > -		ready |= signal->sig[2] &~ blocked->sig[2];
> > -		ready |= signal->sig[1] &~ blocked->sig[1];
> > -		ready |= signal->sig[0] &~ blocked->sig[0];
> > +	case 4:
> > +		ready = signal->sig[3] & ~blocked->sig[3];
> > +		ready |= signal->sig[2] & ~blocked->sig[2];
> > +		ready |= signal->sig[1] & ~blocked->sig[1];
> > +		ready |= signal->sig[0] & ~blocked->sig[0];
> >  		break;
> 
> Again, personally I do not care at all. But why do you think the code looks
> better after re-formatting? This is subjective, but to me it does not.
> 
> In particular, note the extra space before "=" removed by this patch. I guess
> it was added on purpose, and to me
> 
> 	ready  = signal->sig[3] &~ blocked->sig[3];
> 	ready |= signal->sig[2] &~ blocked->sig[2];
> 
> actually looks better thab
> 
> 	ready = signal->sig[3] &~ blocked->sig[3];
> 	ready |= signal->sig[2] &~ blocked->sig[2];
> 
> after your patch.

I can drop the changes in v3.

Thanks!
Christian

> 
> Oleg.
> 

[PATCH v2 11/17] signal: make recalc_sigpending_tsk() return bool

[Christian Brauner]

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

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

diff --git a/kernel/signal.c b/kernel/signal.c
index faf38b46d124..d5a3b1ef775e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -141,20 +141,21 @@ static inline bool has_pending_signals(sigset_t *signal, sigset_t *blocked)
 
 #define PENDING(p,b) has_pending_signals(&(p)->signal, (b))
 
-static int recalc_sigpending_tsk(struct task_struct *t)
+static bool recalc_sigpending_tsk(struct task_struct *t)
 {
 	if ((t->jobctl & JOBCTL_PENDING_MASK) ||
 	    PENDING(&t->pending, &t->blocked) ||
 	    PENDING(&t->signal->shared_pending, &t->blocked)) {
 		set_tsk_thread_flag(t, TIF_SIGPENDING);
-		return 1;
+		return true;
 	}
+
 	/*
 	 * We must never clear the flag in another thread, or in current
 	 * when it's possible the current syscall is returning -ERESTART*.
 	 * So we don't clear it here, and only callers who know they should do.
 	 */
-	return 0;
+	return false;
 }
 
 /*
-- 
2.17.0

[PATCH v2 12/17] signal: make unhandled_signal() return bool

[Christian Brauner]

unhandled_signal() already behaves like a boolean function. Let's actually
declare it as such too.
All callers treat it as such too.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* patch added
---
 include/linux/signal.h |  2 +-
 kernel/signal.c        | 11 +++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/signal.h b/include/linux/signal.h
index a9bc7e1b077e..1145d7061ed9 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -284,7 +284,7 @@ static inline void disallow_signal(int sig)
 
 extern struct kmem_cache *sighand_cachep;
 
-int unhandled_signal(struct task_struct *tsk, int sig);
+extern bool unhandled_signal(struct task_struct *tsk, int sig);
 
 /*
  * In POSIX a signal is sent either to a specific thread (Linux task)
diff --git a/kernel/signal.c b/kernel/signal.c
index d5a3b1ef775e..a41af5a7749f 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -508,13 +508,16 @@ flush_signal_handlers(struct task_struct *t, int force_default)
 	}
 }
 
-int unhandled_signal(struct task_struct *tsk, int sig)
+bool unhandled_signal(struct task_struct *tsk, int sig)
 {
-	void __user *handler = tsk->sighand->action[sig-1].sa.sa_handler;
+	void __user *handler = tsk->sighand->action[sig - 1].sa.sa_handler;
+
 	if (is_global_init(tsk))
-		return 1;
+		return true;
+
 	if (handler != SIG_IGN && handler != SIG_DFL)
-		return 0;
+		return false;
+
 	/* if ptraced, let the tracer determine */
 	return !tsk->ptrace;
 }
-- 
2.17.0

[PATCH v2 13/17] signal: make flush_sigqueue_mask() void

[Christian Brauner]

The return value of flush_sigqueue_mask() is never checked anywhere.

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* patch added
---
 kernel/signal.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index a41af5a7749f..dd32a1ce58e2 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -691,14 +691,14 @@ void signal_wake_up_state(struct task_struct *t, unsigned int state)
  *
  * All callers must be holding the siglock.
  */
-static int flush_sigqueue_mask(sigset_t *mask, struct sigpending *s)
+static void flush_sigqueue_mask(sigset_t *mask, struct sigpending *s)
 {
 	struct sigqueue *q, *n;
 	sigset_t m;
 
 	sigandsets(&m, mask, &s->signal);
 	if (sigisemptyset(&m))
-		return 0;
+		return;
 
 	sigandnsets(&s->signal, &s->signal, mask);
 	list_for_each_entry_safe(q, n, &s->list, list) {
@@ -707,7 +707,6 @@ static int flush_sigqueue_mask(sigset_t *mask, struct sigpending *s)
 			__sigqueue_free(q);
 		}
 	}
-	return 1;
 }
 
 static inline int is_si_special(const struct siginfo *info)
-- 
2.17.0

[PATCH v2 14/17] signal: make wants_signal() return bool

[Christian Brauner]

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

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* patch added
---
 kernel/signal.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index dd32a1ce58e2..8ee3e4a57117 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -883,16 +883,20 @@ static bool prepare_signal(int sig, struct task_struct *p, bool force)
  * as soon as they're available, so putting the signal on the shared queue
  * will be equivalent to sending it to one such thread.
  */
-static inline int wants_signal(int sig, struct task_struct *p)
+static inline bool wants_signal(int sig, struct task_struct *p)
 {
 	if (sigismember(&p->blocked, sig))
-		return 0;
+		return false;
+
 	if (p->flags & PF_EXITING)
-		return 0;
+		return false;
+
 	if (sig == SIGKILL)
-		return 1;
+		return true;
+
 	if (task_is_stopped_or_traced(p))
-		return 0;
+		return false;
+
 	return task_curr(p) || !signal_pending(p);
 }
 
-- 
2.17.0

[PATCH v2 15/17] signal: make legacy_queue() return bool

[Christian Brauner]

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

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* patch added
---
 kernel/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 8ee3e4a57117..515fa59a0e9c 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -976,7 +976,7 @@ static void complete_signal(int sig, struct task_struct *p, int group)
 	return;
 }
 
-static inline int legacy_queue(struct sigpending *signals, int sig)
+static inline bool legacy_queue(struct sigpending *signals, int sig)
 {
 	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
 }
-- 
2.17.0

[PATCH v2 16/17] signal: make security_task_kill() return bool

[Christian Brauner]

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

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* patch added
---
 kernel/signal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 515fa59a0e9c..d5f9472a0935 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1922,10 +1922,10 @@ static inline bool may_ptrace_stop(void)
  * Return non-zero if there is a SIGKILL that should be waking us up.
  * Called with the siglock held.
  */
-static int sigkill_pending(struct task_struct *tsk)
+static bool sigkill_pending(struct task_struct *tsk)
 {
-	return	sigismember(&tsk->pending.signal, SIGKILL) ||
-		sigismember(&tsk->signal->shared_pending.signal, SIGKILL);
+	return sigismember(&tsk->pending.signal, SIGKILL) ||
+	       sigismember(&tsk->signal->shared_pending.signal, SIGKILL);
 }
 
 /*
-- 
2.17.0

Re: [PATCH v2 16/17] signal: make security_task_kill() return bool

[Oleg Nesterov]

On 06/01, Christian Brauner wrote:
>
> security_task_kill() already behaves like a boolean function. Let's
> actually declare it as such too.

The subject/changelog is wrong, this patch changes sigkill_pending()

> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> v1->v2:
> * unchanged
> v0->v1:
> * patch added
> ---
>  kernel/signal.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 515fa59a0e9c..d5f9472a0935 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1922,10 +1922,10 @@ static inline bool may_ptrace_stop(void)
>   * Return non-zero if there is a SIGKILL that should be waking us up.
>   * Called with the siglock held.
>   */
> -static int sigkill_pending(struct task_struct *tsk)
> +static bool sigkill_pending(struct task_struct *tsk)
>  {
> -	return	sigismember(&tsk->pending.signal, SIGKILL) ||
> -		sigismember(&tsk->signal->shared_pending.signal, SIGKILL);
> +	return sigismember(&tsk->pending.signal, SIGKILL) ||
> +	       sigismember(&tsk->signal->shared_pending.signal, SIGKILL);
>  }
>  
>  /*
> -- 
> 2.17.0
> 

Re: [PATCH v2 16/17] signal: make security_task_kill() return bool

[Christian Brauner]

On Fri, Jun 01, 2018 at 06:26:25PM +0200, Oleg Nesterov wrote:
> On 06/01, Christian Brauner wrote:
> >
> > security_task_kill() already behaves like a boolean function. Let's
> > actually declare it as such too.
> 
> The subject/changelog is wrong, this patch changes sigkill_pending()

I'll add a note in the commit message that superfuous whitespace is
removed too.

Thanks!
Christian

> 
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> > v1->v2:
> > * unchanged
> > v0->v1:
> > * patch added
> > ---
> >  kernel/signal.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 515fa59a0e9c..d5f9472a0935 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1922,10 +1922,10 @@ static inline bool may_ptrace_stop(void)
> >   * Return non-zero if there is a SIGKILL that should be waking us up.
> >   * Called with the siglock held.
> >   */
> > -static int sigkill_pending(struct task_struct *tsk)
> > +static bool sigkill_pending(struct task_struct *tsk)
> >  {
> > -	return	sigismember(&tsk->pending.signal, SIGKILL) ||
> > -		sigismember(&tsk->signal->shared_pending.signal, SIGKILL);
> > +	return sigismember(&tsk->pending.signal, SIGKILL) ||
> > +	       sigismember(&tsk->signal->shared_pending.signal, SIGKILL);
> >  }
> >  
> >  /*
> > -- 
> > 2.17.0
> > 
> 

Re: [PATCH v2 16/17] signal: make security_task_kill() return bool

[Christian Brauner]

On Fri, Jun 01, 2018 at 06:26:25PM +0200, Oleg Nesterov wrote:
> On 06/01, Christian Brauner wrote:
> >
> > security_task_kill() already behaves like a boolean function. Let's
> > actually declare it as such too.
> 
> The subject/changelog is wrong, this patch changes sigkill_pending()

Ah yes, s/security_task_kill()/sigkill_pending()

Thanks!
Christian

> 
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> > v1->v2:
> > * unchanged
> > v0->v1:
> > * patch added
> > ---
> >  kernel/signal.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 515fa59a0e9c..d5f9472a0935 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1922,10 +1922,10 @@ static inline bool may_ptrace_stop(void)
> >   * Return non-zero if there is a SIGKILL that should be waking us up.
> >   * Called with the siglock held.
> >   */
> > -static int sigkill_pending(struct task_struct *tsk)
> > +static bool sigkill_pending(struct task_struct *tsk)
> >  {
> > -	return	sigismember(&tsk->pending.signal, SIGKILL) ||
> > -		sigismember(&tsk->signal->shared_pending.signal, SIGKILL);
> > +	return sigismember(&tsk->pending.signal, SIGKILL) ||
> > +	       sigismember(&tsk->signal->shared_pending.signal, SIGKILL);
> >  }
> >  
> >  /*
> > -- 
> > 2.17.0
> > 
> 

[PATCH v2 17/17] signal: make get_signal() return bool

[Christian Brauner]

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

Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
* unchanged
v0->v1:
* patch added
---
 include/linux/signal.h | 2 +-
 kernel/signal.c        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/signal.h b/include/linux/signal.h
index 1145d7061ed9..97d5ff809716 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -262,7 +262,7 @@ extern void set_current_blocked(sigset_t *);
 extern void __set_current_blocked(const sigset_t *);
 extern int show_unhandled_signals;
 
-extern int get_signal(struct ksignal *ksig);
+extern bool get_signal(struct ksignal *ksig);
 extern void signal_setup_done(int failed, struct ksignal *ksig, int stepping);
 extern void exit_signals(struct task_struct *tsk);
 extern void kernel_sigaction(int, __sighandler_t);
diff --git a/kernel/signal.c b/kernel/signal.c
index d5f9472a0935..be5b85567ca4 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2305,7 +2305,7 @@ static int ptrace_signal(int signr, siginfo_t *info)
 	return signr;
 }
 
-int get_signal(struct ksignal *ksig)
+bool get_signal(struct ksignal *ksig)
 {
 	struct sighand_struct *sighand = current->sighand;
 	struct signal_struct *signal = current->signal;
@@ -2315,7 +2315,7 @@ int get_signal(struct ksignal *ksig)
 		task_work_run();
 
 	if (unlikely(uprobe_deny_signal()))
-		return 0;
+		return false;
 
 	/*
 	 * Do this once, we can't return to user-mode if freezing() == T.
-- 
2.17.0