* [PATCH 0/8] signal: refactor some functions
@ 2018-05-28 13:49 Christian Brauner
2018-05-28 13:49 ` [PATCH 1/8] signal: make force_sigsegv() void Christian Brauner
` (7 more replies)
0 siblings, 8 replies; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 13:49 UTC (permalink / raw)
To: linux-kernel
Cc: ebiederm, gregkh, mingo, james.morris, keescook, peterz, sds,
viro, akpm, oleg, 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/8] signal: make force_sigsegv() void
2018-05-28 13:49 [PATCH 0/8] signal: refactor some functions Christian Brauner
@ 2018-05-28 13:49 ` Christian Brauner
2018-05-28 13:49 ` [PATCH 2/8] signal: make kill_as_cred_perm() return bool Christian Brauner
` (6 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 13:49 UTC (permalink / raw)
To: linux-kernel
Cc: ebiederm, gregkh, mingo, james.morris, keescook, peterz, sds,
viro, akpm, oleg, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/8] signal: make kill_as_cred_perm() return bool
2018-05-28 13:49 [PATCH 0/8] signal: refactor some functions Christian Brauner
2018-05-28 13:49 ` [PATCH 1/8] signal: make force_sigsegv() void Christian Brauner
@ 2018-05-28 13:49 ` Christian Brauner
2018-05-28 14:07 ` Al Viro
2018-05-28 13:49 ` [PATCH 3/8] signal: make may_ptrace_stop() " Christian Brauner
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 13:49 UTC (permalink / raw)
To: linux-kernel
Cc: ebiederm, gregkh, mingo, james.morris, keescook, peterz, sds,
viro, akpm, oleg, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/8] signal: make may_ptrace_stop() return bool
2018-05-28 13:49 [PATCH 0/8] signal: refactor some functions Christian Brauner
2018-05-28 13:49 ` [PATCH 1/8] signal: make force_sigsegv() void Christian Brauner
2018-05-28 13:49 ` [PATCH 2/8] signal: make kill_as_cred_perm() return bool Christian Brauner
@ 2018-05-28 13:49 ` Christian Brauner
2018-05-28 14:12 ` Al Viro
2018-05-28 13:49 ` [PATCH 4/8] signal: add copy_pending() helper Christian Brauner
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 13:49 UTC (permalink / raw)
To: linux-kernel
Cc: ebiederm, gregkh, mingo, james.morris, keescook, peterz, sds,
viro, akpm, oleg, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/8] signal: add copy_pending() helper
2018-05-28 13:49 [PATCH 0/8] signal: refactor some functions Christian Brauner
` (2 preceding siblings ...)
2018-05-28 13:49 ` [PATCH 3/8] signal: make may_ptrace_stop() " Christian Brauner
@ 2018-05-28 13:49 ` Christian Brauner
2018-05-28 13:49 ` [PATCH 5/8] signal: flatten do_send_sig_info() Christian Brauner
` (3 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 13:49 UTC (permalink / raw)
To: linux-kernel
Cc: ebiederm, gregkh, mingo, james.morris, keescook, peterz, sds,
viro, akpm, oleg, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 5/8] signal: flatten do_send_sig_info()
2018-05-28 13:49 [PATCH 0/8] signal: refactor some functions Christian Brauner
` (3 preceding siblings ...)
2018-05-28 13:49 ` [PATCH 4/8] signal: add copy_pending() helper Christian Brauner
@ 2018-05-28 13:49 ` Christian Brauner
2018-05-28 14:15 ` Al Viro
2018-05-28 13:49 ` [PATCH 6/8] signal: drop else branch in do_signal_stop() Christian Brauner
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 13:49 UTC (permalink / raw)
To: linux-kernel
Cc: ebiederm, gregkh, mingo, james.morris, keescook, peterz, sds,
viro, akpm, oleg, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 6/8] signal: drop else branch in do_signal_stop()
2018-05-28 13:49 [PATCH 0/8] signal: refactor some functions Christian Brauner
` (4 preceding siblings ...)
2018-05-28 13:49 ` [PATCH 5/8] signal: flatten do_send_sig_info() Christian Brauner
@ 2018-05-28 13:49 ` Christian Brauner
2018-05-28 13:49 ` [PATCH 7/8] signal: make do_sigpending() void Christian Brauner
2018-05-28 13:49 ` [PATCH 8/8] signal: simplify rt_sigaction() Christian Brauner
7 siblings, 0 replies; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 13:49 UTC (permalink / raw)
To: linux-kernel
Cc: ebiederm, gregkh, mingo, james.morris, keescook, peterz, sds,
viro, akpm, oleg, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 7/8] signal: make do_sigpending() void
2018-05-28 13:49 [PATCH 0/8] signal: refactor some functions Christian Brauner
` (5 preceding siblings ...)
2018-05-28 13:49 ` [PATCH 6/8] signal: drop else branch in do_signal_stop() Christian Brauner
@ 2018-05-28 13:49 ` Christian Brauner
2018-05-28 14:20 ` Al Viro
2018-05-28 13:49 ` [PATCH 8/8] signal: simplify rt_sigaction() Christian Brauner
7 siblings, 1 reply; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 13:49 UTC (permalink / raw)
To: linux-kernel
Cc: ebiederm, gregkh, mingo, james.morris, keescook, peterz, sds,
viro, akpm, oleg, 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(¤t->sighand->siglock);
sigorsets(set, ¤t->pending.signal,
@@ -2782,7 +2782,6 @@ static int do_sigpending(sigset_t *set)
/* Outside the lock because only this thread touches it. */
sigandsets(set, ¤t->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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 8/8] signal: simplify rt_sigaction()
2018-05-28 13:49 [PATCH 0/8] signal: refactor some functions Christian Brauner
` (6 preceding siblings ...)
2018-05-28 13:49 ` [PATCH 7/8] signal: make do_sigpending() void Christian Brauner
@ 2018-05-28 13:49 ` Christian Brauner
2018-05-29 6:47 ` Christoph Hellwig
7 siblings, 1 reply; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 13:49 UTC (permalink / raw)
To: linux-kernel
Cc: ebiederm, gregkh, mingo, james.morris, keescook, peterz, sds,
viro, akpm, oleg, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/8] signal: make kill_as_cred_perm() return bool
2018-05-28 13:49 ` [PATCH 2/8] signal: make kill_as_cred_perm() return bool Christian Brauner
@ 2018-05-28 14:07 ` Al Viro
2018-05-28 18:31 ` Christian Brauner
0 siblings, 1 reply; 18+ messages in thread
From: Al Viro @ 2018-05-28 14:07 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, ebiederm, gregkh, mingo, james.morris, keescook,
peterz, sds, akpm, oleg
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;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/8] signal: make may_ptrace_stop() return bool
2018-05-28 13:49 ` [PATCH 3/8] signal: make may_ptrace_stop() " Christian Brauner
@ 2018-05-28 14:12 ` Al Viro
2018-05-28 18:30 ` Christian Brauner
0 siblings, 1 reply; 18+ messages in thread
From: Al Viro @ 2018-05-28 14:12 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, ebiederm, gregkh, mingo, james.morris, keescook,
peterz, sds, akpm, oleg
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);
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/8] signal: flatten do_send_sig_info()
2018-05-28 13:49 ` [PATCH 5/8] signal: flatten do_send_sig_info() Christian Brauner
@ 2018-05-28 14:15 ` Al Viro
2018-05-28 15:07 ` Christian Brauner
0 siblings, 1 reply; 18+ messages in thread
From: Al Viro @ 2018-05-28 14:15 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, ebiederm, gregkh, mingo, james.morris, keescook,
peterz, sds, akpm, oleg
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?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 7/8] signal: make do_sigpending() void
2018-05-28 13:49 ` [PATCH 7/8] signal: make do_sigpending() void Christian Brauner
@ 2018-05-28 14:20 ` Al Viro
0 siblings, 0 replies; 18+ messages in thread
From: Al Viro @ 2018-05-28 14:20 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, ebiederm, gregkh, mingo, james.morris, keescook,
peterz, sds, akpm, oleg
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/8] signal: flatten do_send_sig_info()
2018-05-28 14:15 ` Al Viro
@ 2018-05-28 15:07 ` Christian Brauner
0 siblings, 0 replies; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 15:07 UTC (permalink / raw)
To: Al Viro
Cc: linux-kernel, ebiederm, gregkh, mingo, james.morris, keescook,
peterz, sds, akpm, oleg
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/8] signal: make may_ptrace_stop() return bool
2018-05-28 14:12 ` Al Viro
@ 2018-05-28 18:30 ` Christian Brauner
0 siblings, 0 replies; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 18:30 UTC (permalink / raw)
To: Al Viro
Cc: linux-kernel, ebiederm, gregkh, mingo, james.morris, keescook,
peterz, sds, akpm, oleg
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/8] signal: make kill_as_cred_perm() return bool
2018-05-28 14:07 ` Al Viro
@ 2018-05-28 18:31 ` Christian Brauner
0 siblings, 0 replies; 18+ messages in thread
From: Christian Brauner @ 2018-05-28 18:31 UTC (permalink / raw)
To: Al Viro
Cc: linux-kernel, ebiederm, gregkh, mingo, james.morris, keescook,
peterz, sds, akpm, oleg
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 8/8] signal: simplify rt_sigaction()
2018-05-28 13:49 ` [PATCH 8/8] signal: simplify rt_sigaction() Christian Brauner
@ 2018-05-29 6:47 ` Christoph Hellwig
2018-05-29 10:24 ` Christian Brauner
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2018-05-29 6:47 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, ebiederm, gregkh, mingo, james.morris, keescook,
peterz, sds, viro, akpm, oleg
> + 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;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 8/8] signal: simplify rt_sigaction()
2018-05-29 6:47 ` Christoph Hellwig
@ 2018-05-29 10:24 ` Christian Brauner
0 siblings, 0 replies; 18+ messages in thread
From: Christian Brauner @ 2018-05-29 10:24 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-kernel, ebiederm, gregkh, mingo, james.morris, keescook,
peterz, sds, viro, akpm, oleg
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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-05-29 10:25 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-28 13:49 [PATCH 0/8] signal: refactor some functions Christian Brauner
2018-05-28 13:49 ` [PATCH 1/8] signal: make force_sigsegv() void Christian Brauner
2018-05-28 13:49 ` [PATCH 2/8] signal: make kill_as_cred_perm() return bool Christian Brauner
2018-05-28 14:07 ` Al Viro
2018-05-28 18:31 ` Christian Brauner
2018-05-28 13:49 ` [PATCH 3/8] signal: make may_ptrace_stop() " Christian Brauner
2018-05-28 14:12 ` Al Viro
2018-05-28 18:30 ` Christian Brauner
2018-05-28 13:49 ` [PATCH 4/8] signal: add copy_pending() helper Christian Brauner
2018-05-28 13:49 ` [PATCH 5/8] signal: flatten do_send_sig_info() Christian Brauner
2018-05-28 14:15 ` Al Viro
2018-05-28 15:07 ` Christian Brauner
2018-05-28 13:49 ` [PATCH 6/8] signal: drop else branch in do_signal_stop() Christian Brauner
2018-05-28 13:49 ` [PATCH 7/8] signal: make do_sigpending() void Christian Brauner
2018-05-28 14:20 ` Al Viro
2018-05-28 13:49 ` [PATCH 8/8] signal: simplify rt_sigaction() Christian Brauner
2018-05-29 6:47 ` Christoph Hellwig
2018-05-29 10:24 ` Christian Brauner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).