[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.
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 (20):
  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()
  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              | 269 ++++++++++++++++++-----------------
 3 files changed, 140 insertions(+), 135 deletions(-)

-- 
2.17.0

[PATCH v1 01/20] 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>
---
v0->v1:
* patch 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 v1 02/20] 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>
---
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 v1 03/20] 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>
---
v0->v1:
* patch 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

[PATCH v1 04/20] 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>
---
v0->v1:
* patch unchanged
---
 kernel/signal.c | 54 +++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index bc750fb4ddcc..baae137455eb 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 v1 05/20] 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>
---
v0->v1:
* patch unchanged
---
 kernel/signal.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index baae137455eb..a628b56415e6 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

[PATCH v1 06/20] 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>
---
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

[PATCH v1 07/20] 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>
---
v0->v1:
* added Acked-By line from Al
---
 kernel/signal.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index d1914439f144..3af140048366 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

[PATCH v1 08/20] 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>
---
v0->v1:
* patch unchanged
---
 kernel/signal.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 3af140048366..7095fa11af15 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

[PATCH v1 09/20] 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>
---
v0->v1:
* patch introduced
---
 kernel/signal.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 7095fa11af15..faef85bb243e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -723,21 +723,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 v1 10/20] 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>
---
v0->v1:
* patch introduced
---
 kernel/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index faef85bb243e..78a9b32785df 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 v1 11/20] 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>
---
v0->v1:
* patch introduced
---
 kernel/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 78a9b32785df..c3981081ec25 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 v1 12/20] 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>
---
v0->v1:
* patch introduced
---
 kernel/signal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index c3981081ec25..60c501db15d9 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 v1 13/20] 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>
---
v0->v1:
* patch introduced
---
 kernel/signal.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 60c501db15d9..d2d4d10223ca 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

[PATCH v1 14/20] 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>
---
v0->v1:
* patch introduced
---
 kernel/signal.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index d2d4d10223ca..623039987546 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 v1 15/20] 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>
---
v0->v1:
* patch introduced
---
 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 623039987546..c382ac346a81 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 v1 16/20] 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>
---
v0->v1:
* patch introduced
---
 kernel/signal.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index c382ac346a81..0959965b523d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -697,14 +697,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) {
@@ -713,7 +713,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 v1 17/20] 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>
---
v0->v1:
* patch introduced
---
 kernel/signal.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 0959965b523d..ac5c2700901d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -889,16 +889,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 v1 18/20] 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>
---
v0->v1:
* patch introduced
---
 kernel/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index ac5c2700901d..7e217e83a6de 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -982,7 +982,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 v1 19/20] 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>
---
v0->v1:
* patch introduced
---
 kernel/signal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 7e217e83a6de..a4787c90faab 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1928,10 +1928,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 v1 20/20] 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>
---
v0->v1:
* patch introduced
---
 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 a4787c90faab..e3b0719986a2 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2311,7 +2311,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;
@@ -2321,7 +2321,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

Re: [PATCH v1 04/20] signal: add copy_pending() helper

[Eric W. Biederman]

Christian Brauner <christian@brauner.io> writes:

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

Ick no.  As far as I can see this just confuses the logic of the
collect_signal function.

Instead of having two cases with an optional
"sigdelset(&list->signal, sig)" if the signal is no longer in the queue,
you are moving the core work of collect_signal into another function.

At the very least this is going to make maintenance more difficult
as now the work of this function is split into two functions.

It most definitely would have made the bug fix to add resched_timer more
difficult.

Eric

> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> v0->v1:
> * patch unchanged
> ---
>  kernel/signal.c | 54 +++++++++++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index bc750fb4ddcc..baae137455eb 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,

Re: [PATCH v1 05/20] signal: flatten do_send_sig_info()

[Eric W. Biederman]

Christian Brauner <christian@brauner.io> writes:

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

Introducing multiple exits into a function.  Ick.
You do know that is what Dijkstra was arguing against in his paper
"Goto Considered Harmful"

That introduces mutiple exits and makes the function harder to analyze.
It is especially a pain as I have something in my queue that will
shuffle things around and remove the possibility of lock_task_sighand
failing.

Eric

> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> v0->v1:
> * patch unchanged
> ---
>  kernel/signal.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index baae137455eb..a628b56415e6 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;
>  }

Re: [PATCH v1 05/20] signal: flatten do_send_sig_info()

[Christian Brauner]

On Tue, May 29, 2018 at 07:28:27AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian@brauner.io> writes:
> 
> > Let's return early when lock_task_sighand() fails and move send_signal()
> > and unlock_task_sighand() out of the if block.
> 
> Introducing multiple exits into a function.  Ick.
> You do know that is what Dijkstra was arguing against in his paper
> "Goto Considered Harmful"
> 
> That introduces mutiple exits and makes the function harder to analyze.
> It is especially a pain as I have something in my queue that will
> shuffle things around and remove the possibility of lock_task_sighand
> failing.

I'm happy to drop this one if you have a fix for this in your tree
anyway.

Aside from that, I think it might make sense to route this patch series
through your tree though since you're doing the siginfo rework
currently.(?)

Christian

> 
> Eric
> 
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> > v0->v1:
> > * patch unchanged
> > ---
> >  kernel/signal.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index baae137455eb..a628b56415e6 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;
> >  }

Re: [PATCH v1 04/20] signal: add copy_pending() helper

[Christian Brauner]

On Tue, May 29, 2018 at 07:24:26AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian@brauner.io> writes:
> 
> > Instead of using a goto for this let's add a simple helper copy_pending()
> > which can be called in both places.
> 
> Ick no.  As far as I can see this just confuses the logic of the
> collect_signal function.
> 
> Instead of having two cases with an optional
> "sigdelset(&list->signal, sig)" if the signal is no longer in the queue,
> you are moving the core work of collect_signal into another function.
> 
> At the very least this is going to make maintenance more difficult
> as now the work of this function is split into two functions.

I do disagree here tbh. The goto jump into it the if part of an if-else
seems pretty nasty.
I also don't know why this should be confusing the logic. There's a
single function that is called in two places and it is declared directly
atop it's only caller. Additionally, recognizing a single name of a
function as being the same in two places is way easier then recognizing
that a multi-line pattern is the same in two places.

Christian

> 
> It most definitely would have made the bug fix to add resched_timer more
> difficult.
> 
> Eric
> 
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> > v0->v1:
> > * patch unchanged
> > ---
> >  kernel/signal.c | 54 +++++++++++++++++++++++++++----------------------
> >  1 file changed, 30 insertions(+), 24 deletions(-)
> >
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index bc750fb4ddcc..baae137455eb 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,

Re: [PATCH v1 04/20] signal: add copy_pending() helper

[Eric W. Biederman]

Christian Brauner <christian@brauner.io> writes:

> On Tue, May 29, 2018 at 07:24:26AM -0500, Eric W. Biederman wrote:
>> Christian Brauner <christian@brauner.io> writes:
>> 
>> > Instead of using a goto for this let's add a simple helper copy_pending()
>> > which can be called in both places.
>> 
>> Ick no.  As far as I can see this just confuses the logic of the
>> collect_signal function.
>> 
>> Instead of having two cases with an optional
>> "sigdelset(&list->signal, sig)" if the signal is no longer in the queue,
>> you are moving the core work of collect_signal into another function.
>> 
>> At the very least this is going to make maintenance more difficult
>> as now the work of this function is split into two functions.
>
> I do disagree here tbh. The goto jump into it the if part of an if-else
> seems pretty nasty.
> I also don't know why this should be confusing the logic. There's a
> single function that is called in two places and it is declared directly
> atop it's only caller. Additionally, recognizing a single name of a
> function as being the same in two places is way easier then recognizing
> that a multi-line pattern is the same in two places.

But there are not two places.  There is only one place.
The logic might be cleaned up reorganizing the tests a little bit.
Something like this perhaps.

	/*
	 * Collect the siginfo appropriate to this signal.  Check if
	 * there is another siginfo for the same signal.
	*/
	list_for_each_entry(q, &list->list, list) {
		if (q->info.si_signo == sig) {
			if (first)
                        	break;
			first = q;
		}
	}

	/* Not still pending? */
	if (!first || (&q->list != &list->list))
        	sigdelset(&list->signal, sig);
	if (first) {
		...


The logic at a high level is:
	Is there another instance of this signal pending?
             yes?  Then don't "sigdelset"
        Do we have siginfo?
           yes? return it.
           no?  dummy up a siginfo.

Making that logic clearer would be nice.  Obscuring it with
an extra function just obstructs maintenance.

Eric

Re: [PATCH v1 04/20] signal: add copy_pending() helper

[Christian Brauner]

On Tue, May 29, 2018 at 08:44:22AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian@brauner.io> writes:
> 
> > On Tue, May 29, 2018 at 07:24:26AM -0500, Eric W. Biederman wrote:
> >> Christian Brauner <christian@brauner.io> writes:
> >> 
> >> > Instead of using a goto for this let's add a simple helper copy_pending()
> >> > which can be called in both places.
> >> 
> >> Ick no.  As far as I can see this just confuses the logic of the
> >> collect_signal function.
> >> 
> >> Instead of having two cases with an optional
> >> "sigdelset(&list->signal, sig)" if the signal is no longer in the queue,
> >> you are moving the core work of collect_signal into another function.
> >> 
> >> At the very least this is going to make maintenance more difficult
> >> as now the work of this function is split into two functions.
> >
> > I do disagree here tbh. The goto jump into it the if part of an if-else
> > seems pretty nasty.
> > I also don't know why this should be confusing the logic. There's a
> > single function that is called in two places and it is declared directly
> > atop it's only caller. Additionally, recognizing a single name of a
> > function as being the same in two places is way easier then recognizing
> > that a multi-line pattern is the same in two places.
> 
> But there are not two places.  There is only one place.

Which is reachable from two different places and the goal was to avoid
having to jump into the second location with a goto in an if-else
construct.

> The logic might be cleaned up reorganizing the tests a little bit.
> Something like this perhaps.
> 
> 	/*
> 	 * Collect the siginfo appropriate to this signal.  Check if
> 	 * there is another siginfo for the same signal.
> 	*/
> 	list_for_each_entry(q, &list->list, list) {
> 		if (q->info.si_signo == sig) {
> 			if (first)
>                         	break;
> 			first = q;
> 		}
> 	}
> 
> 	/* Not still pending? */
> 	if (!first || (&q->list != &list->list))
>         	sigdelset(&list->signal, sig);
> 	if (first) {
> 		...
> 
> 
> The logic at a high level is:
> 	Is there another instance of this signal pending?
>              yes?  Then don't "sigdelset"
>         Do we have siginfo?
>            yes? return it.
>            no?  dummy up a siginfo.
> 
> Making that logic clearer would be nice.  Obscuring it with

I'm happy to change this in v2. But there's nothing obscure about
calling a helper function in two places while I can keep the definiton
of the helper function and the two places that it is called in on the
same 80x43 terminal.

Christian

> an extra function just obstructs maintenance.
> 
> Eric

Re: [PATCH v1 06/20] signal: drop else branch in do_signal_stop()

[Oleg Nesterov]

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???

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.



> Signed-off-by: Christian Brauner <christian@brauner.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
> 

Re: [PATCH v1 06/20] signal: drop else branch in do_signal_stop()

[Christian Brauner]

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@brauner.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
> > 
> 

Re: [PATCH v1 05/20] signal: flatten do_send_sig_info()

[Eric W. Biederman]

Christian Brauner <christian@brauner.io> writes:

2> On Tue, May 29, 2018 at 07:28:27AM -0500, Eric W. Biederman wrote:
>> Christian Brauner <christian@brauner.io> writes:
>> 
>> > Let's return early when lock_task_sighand() fails and move send_signal()
>> > and unlock_task_sighand() out of the if block.
>> 
>> Introducing multiple exits into a function.  Ick.
>> You do know that is what Dijkstra was arguing against in his paper
>> "Goto Considered Harmful"
>> 
>> That introduces mutiple exits and makes the function harder to analyze.
>> It is especially a pain as I have something in my queue that will
>> shuffle things around and remove the possibility of lock_task_sighand
>> failing.
>
> I'm happy to drop this one if you have a fix for this in your tree
> anyway.
>
> Aside from that, I think it might make sense to route this patch series
> through your tree though since you're doing the siginfo rework
> currently.(?)

If you will purely code style changes such as this one I will be happy
to pick up the rest as they are pretty much obviously correct changes.

Eric