* [PATCH 0/2] exec: s/group_exit_task/group_exec_task/ for clarity
@ 2020-06-19 18:30 Eric W. Biederman
2020-06-19 18:32 ` [PATCH 1/2] exec: Don't set group_exit_task during a coredump Eric W. Biederman
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Eric W. Biederman @ 2020-06-19 18:30 UTC (permalink / raw)
To: linux-kernel
Cc: linux-fsdevel, Linus Torvalds, Oleg Nesterov, Jann Horn,
Kees Cook, Bernd Edlinger
I am hoping to be able to stop all of the threads at the beginning of
exec so we can write the exec code as if it is single threaded. That
is hard but cleanups to enable that change are easy.
There is a variable tsk->signal->group_exit_task that is only truly
used in de_thread. The changes clean up the coredump code and
rename the variable to make it clear that exec is it's only user.
Eric W. Biederman (2):
exec: Don't set group_exit_task during a coredump
exec: Rename group_exit_task group_exec_task and correct the Documentation
fs/coredump.c | 2 --
fs/exec.c | 8 ++++----
include/linux/sched/signal.h | 15 ++++++---------
kernel/exit.c | 4 ++--
4 files changed, 12 insertions(+), 17 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2] exec: Don't set group_exit_task during a coredump 2020-06-19 18:30 [PATCH 0/2] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman @ 2020-06-19 18:32 ` Eric W. Biederman 2020-06-20 18:58 ` Linus Torvalds 2020-06-22 11:25 ` Oleg Nesterov 2020-06-19 18:33 ` [PATCH 2/2] exec: Rename group_exit_task group_exec_task and correct the Documentation Eric W. Biederman 2020-06-23 21:52 ` [PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman 2 siblings, 2 replies; 14+ messages in thread From: Eric W. Biederman @ 2020-06-19 18:32 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Linus Torvalds, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger Instead test SIGNAL_GROUP_COREDUMP in signal_group_exit(). This results in clearer easier to understand logic. This makes the code easier to modify in the future as this leaves de_thread as the only user of group_exit_task. This is safe because there are only two places that set SIGNAL_GROUP_COREDUMP. In one place the code is setting SIGNAL_GROUP_EXIT and SIGNAL_GROUP_COREDUMP together with the result that signal_group_exit() will subsequently return true. In the other the location which is being changed SIGNAL_GROUP_COREDUMP is being set along with signal_group_exit, which also causes subsequent calls of signal_group_exit to return true. Thus testing SIGNAL_GROUP_COREDUMP in signal_group_exit() results in no change in behavior. Only signal_group_exit tests group_exit_task so leaving as NULL during a coredump and nothing uses the value of group_exit_task that the coredump sets. So not setting group_exit_task is safe during a coredump. I looked at the commit that introduced this behavior[1] and Oleg describes that he was setting group_exit_task simply to cause signal_group_exit to return true. So no surprises come from the history. Cc: Oleg Nesterov <oleg@redhat.com> [1] 6cd8f0acae34 ("coredump: ensure that SIGKILL always kills the dumping thread") Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/coredump.c | 2 -- include/linux/sched/signal.h | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/fs/coredump.c b/fs/coredump.c index 7237f07ff6be..37b71c72ab3a 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -369,7 +369,6 @@ static int zap_threads(struct task_struct *tsk, struct mm_struct *mm, spin_lock_irq(&tsk->sighand->siglock); if (!signal_group_exit(tsk->signal)) { mm->core_state = core_state; - tsk->signal->group_exit_task = tsk; nr = zap_process(tsk, exit_code, 0); clear_tsk_thread_flag(tsk, TIF_SIGPENDING); } @@ -481,7 +480,6 @@ static void coredump_finish(struct mm_struct *mm, bool core_dumped) spin_lock_irq(¤t->sighand->siglock); if (core_dumped && !__fatal_signal_pending(current)) current->signal->group_exit_code |= 0x80; - current->signal->group_exit_task = NULL; current->signal->flags = SIGNAL_GROUP_EXIT; spin_unlock_irq(¤t->sighand->siglock); diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 0ee5e696c5d8..92c72f5db111 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -265,7 +265,7 @@ static inline void signal_set_stop_flags(struct signal_struct *sig, /* If true, all threads except ->group_exit_task have pending SIGKILL */ static inline int signal_group_exit(const struct signal_struct *sig) { - return (sig->flags & SIGNAL_GROUP_EXIT) || + return (sig->flags & (SIGNAL_GROUP_EXIT | SIGNAL_GROUP_COREDUMP)) || (sig->group_exit_task != NULL); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] exec: Don't set group_exit_task during a coredump 2020-06-19 18:32 ` [PATCH 1/2] exec: Don't set group_exit_task during a coredump Eric W. Biederman @ 2020-06-20 18:58 ` Linus Torvalds 2020-06-22 16:20 ` Eric W. Biederman 2020-06-22 11:25 ` Oleg Nesterov 1 sibling, 1 reply; 14+ messages in thread From: Linus Torvalds @ 2020-06-20 18:58 UTC (permalink / raw) To: Eric W. Biederman Cc: Linux Kernel Mailing List, linux-fsdevel, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger On Fri, Jun 19, 2020 at 11:36 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Instead test SIGNAL_GROUP_COREDUMP in signal_group_exit(). You say "instead", but the patch itself doesn't agree: > static inline int signal_group_exit(const struct signal_struct *sig) > { > - return (sig->flags & SIGNAL_GROUP_EXIT) || > + return (sig->flags & (SIGNAL_GROUP_EXIT | SIGNAL_GROUP_COREDUMP)) || > (sig->group_exit_task != NULL); > } it does it _in_addition_to_. I think the whole test for "sig->group_exit_task != NULL" should be removed for this commit to make sense. Linus ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] exec: Don't set group_exit_task during a coredump 2020-06-20 18:58 ` Linus Torvalds @ 2020-06-22 16:20 ` Eric W. Biederman 2020-06-22 16:32 ` Linus Torvalds 0 siblings, 1 reply; 14+ messages in thread From: Eric W. Biederman @ 2020-06-22 16:20 UTC (permalink / raw) To: Linus Torvalds Cc: Linux Kernel Mailing List, linux-fsdevel, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger Linus Torvalds <torvalds@linux-foundation.org> writes: > On Fri, Jun 19, 2020 at 11:36 AM Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> Instead test SIGNAL_GROUP_COREDUMP in signal_group_exit(). > > You say "instead", but the patch itself doesn't agree: > >> static inline int signal_group_exit(const struct signal_struct *sig) >> { >> - return (sig->flags & SIGNAL_GROUP_EXIT) || >> + return (sig->flags & (SIGNAL_GROUP_EXIT | SIGNAL_GROUP_COREDUMP)) || >> (sig->group_exit_task != NULL); >> } > > it does it _in_addition_to_. Hmm. I think I can change that line to: >> Instead add a test for SIGNAL_GROUP_COREDUMP in signal_group_exit(). Does that read better? > I think the whole test for "sig->group_exit_task != NULL" should be > removed for this commit to make sense. The code change is designed not to have a behavioral change in signal_group_exit(). As de_thread also sets sig->group_exit_task the test for sig->group_exit_task needs to remain in signal_group_exit() for the behavior of signal_group_exit() to remain unchanged. Why do you think the test sig->group_exit_task != NULL should be removed for the commit to make sense? Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] exec: Don't set group_exit_task during a coredump 2020-06-22 16:20 ` Eric W. Biederman @ 2020-06-22 16:32 ` Linus Torvalds 0 siblings, 0 replies; 14+ messages in thread From: Linus Torvalds @ 2020-06-22 16:32 UTC (permalink / raw) To: Eric W. Biederman Cc: Linux Kernel Mailing List, linux-fsdevel, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger On Mon, Jun 22, 2020 at 9:24 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Why do you think the test sig->group_exit_task != NULL should be removed > for the commit to make sense? Because that's what your commit message _said_. It still implies that with your changed language. And honestly, wouldn't it be a lot more understandable if the state was tracked with a single variable? The whole point of this series has been "clarify exec". So let's clarify it. There aren't that many places that set sig->group_exit_task (whether renamed or not). How about we just change _all_ of those to set 'sig->flags', and really clarify things? Linus ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] exec: Don't set group_exit_task during a coredump 2020-06-19 18:32 ` [PATCH 1/2] exec: Don't set group_exit_task during a coredump Eric W. Biederman 2020-06-20 18:58 ` Linus Torvalds @ 2020-06-22 11:25 ` Oleg Nesterov 1 sibling, 0 replies; 14+ messages in thread From: Oleg Nesterov @ 2020-06-22 11:25 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-kernel, linux-fsdevel, Linus Torvalds, Jann Horn, Kees Cook, Bernd Edlinger On 06/19, Eric W. Biederman wrote: > > --- a/fs/coredump.c > +++ b/fs/coredump.c > @@ -369,7 +369,6 @@ static int zap_threads(struct task_struct *tsk, struct mm_struct *mm, > spin_lock_irq(&tsk->sighand->siglock); > if (!signal_group_exit(tsk->signal)) { > mm->core_state = core_state; > - tsk->signal->group_exit_task = tsk; > nr = zap_process(tsk, exit_code, 0); > clear_tsk_thread_flag(tsk, TIF_SIGPENDING); > } > @@ -481,7 +480,6 @@ static void coredump_finish(struct mm_struct *mm, bool core_dumped) > spin_lock_irq(¤t->sighand->siglock); > if (core_dumped && !__fatal_signal_pending(current)) > current->signal->group_exit_code |= 0x80; > - current->signal->group_exit_task = NULL; > current->signal->flags = SIGNAL_GROUP_EXIT; > spin_unlock_irq(¤t->sighand->siglock); > > diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h > index 0ee5e696c5d8..92c72f5db111 100644 > --- a/include/linux/sched/signal.h > +++ b/include/linux/sched/signal.h > @@ -265,7 +265,7 @@ static inline void signal_set_stop_flags(struct signal_struct *sig, > /* If true, all threads except ->group_exit_task have pending SIGKILL */ > static inline int signal_group_exit(const struct signal_struct *sig) > { > - return (sig->flags & SIGNAL_GROUP_EXIT) || > + return (sig->flags & (SIGNAL_GROUP_EXIT | SIGNAL_GROUP_COREDUMP)) || > (sig->group_exit_task != NULL); > } Looks correct. Oleg. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] exec: Rename group_exit_task group_exec_task and correct the Documentation 2020-06-19 18:30 [PATCH 0/2] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman 2020-06-19 18:32 ` [PATCH 1/2] exec: Don't set group_exit_task during a coredump Eric W. Biederman @ 2020-06-19 18:33 ` Eric W. Biederman 2020-06-23 21:52 ` [PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman 2 siblings, 0 replies; 14+ messages in thread From: Eric W. Biederman @ 2020-06-19 18:33 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Linus Torvalds, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger Rename group_exit_task to group_exec_task to make it clear this field is only used during exec. Update the comments for the fields group_exec_task and notify_count as they are only used by exec. Notifications to the execing task aka group_exec_task happen at 0 and -1. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 8 ++++---- include/linux/sched/signal.h | 13 +++++-------- kernel/exit.c | 4 ++-- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index e6e8a9a70327..0bf8bde6edfd 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1145,7 +1145,7 @@ static int de_thread(struct task_struct *tsk) return -EAGAIN; } - sig->group_exit_task = tsk; + sig->group_exec_task = tsk; sig->notify_count = zap_other_threads(tsk); if (!thread_group_leader(tsk)) sig->notify_count--; @@ -1173,7 +1173,7 @@ static int de_thread(struct task_struct *tsk) write_lock_irq(&tasklist_lock); /* * Do this under tasklist_lock to ensure that - * exit_notify() can't miss ->group_exit_task + * exit_notify() can't miss ->group_exec_task */ sig->notify_count = -1; if (likely(leader->exit_state)) @@ -1240,7 +1240,7 @@ static int de_thread(struct task_struct *tsk) release_task(leader); } - sig->group_exit_task = NULL; + sig->group_exec_task = NULL; sig->notify_count = 0; no_thread_group: @@ -1253,7 +1253,7 @@ static int de_thread(struct task_struct *tsk) killed: /* protects against exit_notify() and __exit_signal() */ read_lock(&tasklist_lock); - sig->group_exit_task = NULL; + sig->group_exec_task = NULL; sig->notify_count = 0; read_unlock(&tasklist_lock); return -EAGAIN; diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 92c72f5db111..61019d8fe86b 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -98,13 +98,10 @@ struct signal_struct { /* thread group exit support */ int group_exit_code; - /* overloaded: - * - notify group_exit_task when ->count is equal to notify_count - * - everyone except group_exit_task is stopped during signal delivery - * of fatal signals, group_exit_task processes the signal. - */ + + /* exec support, notify group_exec_task when notify_count is 0 or -1 */ int notify_count; - struct task_struct *group_exit_task; + struct task_struct *group_exec_task; /* thread group stop support, overloads group_exit_code too */ int group_stop_count; @@ -262,11 +259,11 @@ static inline void signal_set_stop_flags(struct signal_struct *sig, sig->flags = (sig->flags & ~SIGNAL_STOP_MASK) | flags; } -/* If true, all threads except ->group_exit_task have pending SIGKILL */ +/* If true, all threads except ->group_exec_task have pending SIGKILL */ static inline int signal_group_exit(const struct signal_struct *sig) { return (sig->flags & (SIGNAL_GROUP_EXIT | SIGNAL_GROUP_COREDUMP)) || - (sig->group_exit_task != NULL); + (sig->group_exec_task != NULL); } extern void flush_signals(struct task_struct *); diff --git a/kernel/exit.c b/kernel/exit.c index 727150f28103..4206d33b4904 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -115,7 +115,7 @@ static void __exit_signal(struct task_struct *tsk) * then notify it: */ if (sig->notify_count > 0 && !--sig->notify_count) - wake_up_process(sig->group_exit_task); + wake_up_process(sig->group_exec_task); if (tsk == sig->curr_target) sig->curr_target = next_thread(tsk); @@ -672,7 +672,7 @@ static void exit_notify(struct task_struct *tsk, int group_dead) /* mt-exec, de_thread() is waiting for group leader */ if (unlikely(tsk->signal->notify_count < 0)) - wake_up_process(tsk->signal->group_exit_task); + wake_up_process(tsk->signal->group_exec_task); write_unlock_irq(&tasklist_lock); list_for_each_entry_safe(p, n, &dead, ptrace_entry) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity 2020-06-19 18:30 [PATCH 0/2] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman 2020-06-19 18:32 ` [PATCH 1/2] exec: Don't set group_exit_task during a coredump Eric W. Biederman 2020-06-19 18:33 ` [PATCH 2/2] exec: Rename group_exit_task group_exec_task and correct the Documentation Eric W. Biederman @ 2020-06-23 21:52 ` Eric W. Biederman 2020-06-23 21:53 ` [PATCH v2 1/6] signal: Pretty up the SIGNAL_GROUP_FLAGS Eric W. Biederman ` (5 more replies) 2 siblings, 6 replies; 14+ messages in thread From: Eric W. Biederman @ 2020-06-23 21:52 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Linus Torvalds, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger There is a variable tsk->signal->group_exit_task that is only truly used in de_thread. These changes modify the coredump code and the signal_group_exit() function to stop using group_exit_task, and rename the group_exit_task variable to group_exec_task. In addition as I think Linus was asking an additional signal flag is added called SIGNAL_GROUP_DETHREAD, and that flag is used in the function signal_group_exit() so that only signal->flags needs to be tested. Eric W. Biederman (6): signal: Pretty up the SIGNAL_GROUP_FLAGS exec: Lock more defensively in exec signal: Implement SIGNAL_GROUP_DETHREAD signal: In signal_group_exit remove the group_exit_task test coredump: Stop using group_exit_task exec: Rename group_exit_task group_exec_task and correct the Documentation fs/coredump.c | 2 -- fs/exec.c | 22 ++++++++++++++++------ include/linux/sched/signal.h | 33 +++++++++++++++++---------------- kernel/exit.c | 4 ++-- 4 files changed, 35 insertions(+), 26 deletions(-) ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/6] signal: Pretty up the SIGNAL_GROUP_FLAGS 2020-06-23 21:52 ` [PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman @ 2020-06-23 21:53 ` Eric W. Biederman 2020-06-23 21:54 ` [PATCH v2 2/6] exec: Lock more defensively in exec Eric W. Biederman ` (4 subsequent siblings) 5 siblings, 0 replies; 14+ messages in thread From: Eric W. Biederman @ 2020-06-23 21:53 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Linus Torvalds, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger Renumber the flags that go in sig->flags giving different groups of flags different hex digits. This is needed so that future additions of flags can be adjacent. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- include/linux/sched/signal.h | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 0ee5e696c5d8..b4f36a11be5e 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -241,20 +241,22 @@ struct signal_struct { */ #define SIGNAL_STOP_STOPPED 0x00000001 /* job control stop in effect */ #define SIGNAL_STOP_CONTINUED 0x00000002 /* SIGCONT since WCONTINUED reap */ -#define SIGNAL_GROUP_EXIT 0x00000004 /* group exit in progress */ -#define SIGNAL_GROUP_COREDUMP 0x00000008 /* coredump in progress */ -/* - * Pending notifications to parent. - */ + +/* Pending notifications to parent. */ #define SIGNAL_CLD_STOPPED 0x00000010 #define SIGNAL_CLD_CONTINUED 0x00000020 #define SIGNAL_CLD_MASK (SIGNAL_CLD_STOPPED|SIGNAL_CLD_CONTINUED) -#define SIGNAL_UNKILLABLE 0x00000040 /* for init: ignore fatal signals */ - #define SIGNAL_STOP_MASK (SIGNAL_CLD_MASK | SIGNAL_STOP_STOPPED | \ SIGNAL_STOP_CONTINUED) +/* Signal group actions. */ +#define SIGNAL_GROUP_EXIT 0x00000100 /* group exit in progress */ +#define SIGNAL_GROUP_COREDUMP 0x00000200 /* coredump in progress */ + +/* Flags applicable to the entire signal group. */ +#define SIGNAL_UNKILLABLE 0x00001000 /* for init: ignore fatal signals */ + static inline void signal_set_stop_flags(struct signal_struct *sig, unsigned int flags) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/6] exec: Lock more defensively in exec 2020-06-23 21:52 ` [PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman 2020-06-23 21:53 ` [PATCH v2 1/6] signal: Pretty up the SIGNAL_GROUP_FLAGS Eric W. Biederman @ 2020-06-23 21:54 ` Eric W. Biederman 2020-06-23 21:54 ` [PATCH v2 3/6] signal: Implement SIGNAL_GROUP_DETHREAD Eric W. Biederman ` (3 subsequent siblings) 5 siblings, 0 replies; 14+ messages in thread From: Eric W. Biederman @ 2020-06-23 21:54 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Linus Torvalds, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger When taking the task_list_lock in de_thread also take the siglock. This makes de_thread closer to fork the canonical place where these locks are taken. To complete the defensiveness always take siglock when clearing group_exit_task and notify_count. This gives now gives the guarantee that group_exit_task and notify_count are now always changed under siglock. As anything multi-threaded in exec is a rare and slow path I don't think we care if we take an extra lock in practice. The practical reason for doing this is to enable setting signal->flags along with group_exit_task so that the function signal_group_exit can be simplified. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index e6e8a9a70327..33b5d9229c01 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1171,6 +1171,7 @@ static int de_thread(struct task_struct *tsk) for (;;) { cgroup_threadgroup_change_begin(tsk); write_lock_irq(&tasklist_lock); + spin_lock(lock); /* * Do this under tasklist_lock to ensure that * exit_notify() can't miss ->group_exit_task @@ -1179,6 +1180,7 @@ static int de_thread(struct task_struct *tsk) if (likely(leader->exit_state)) break; __set_current_state(TASK_KILLABLE); + spin_unlock(lock); write_unlock_irq(&tasklist_lock); cgroup_threadgroup_change_end(tsk); schedule(); @@ -1234,14 +1236,17 @@ static int de_thread(struct task_struct *tsk) */ if (unlikely(leader->ptrace)) __wake_up_parent(leader, leader->parent); + spin_unlock(lock); write_unlock_irq(&tasklist_lock); cgroup_threadgroup_change_end(tsk); release_task(leader); } + spin_lock_irq(lock); sig->group_exit_task = NULL; sig->notify_count = 0; + spin_unlock_irq(lock); no_thread_group: /* we have changed execution domain */ @@ -1252,10 +1257,12 @@ static int de_thread(struct task_struct *tsk) killed: /* protects against exit_notify() and __exit_signal() */ - read_lock(&tasklist_lock); + read_lock_irq(&tasklist_lock); + spin_lock(lock); sig->group_exit_task = NULL; sig->notify_count = 0; - read_unlock(&tasklist_lock); + spin_unlock(lock); + read_unlock_irq(&tasklist_lock); return -EAGAIN; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/6] signal: Implement SIGNAL_GROUP_DETHREAD 2020-06-23 21:52 ` [PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman 2020-06-23 21:53 ` [PATCH v2 1/6] signal: Pretty up the SIGNAL_GROUP_FLAGS Eric W. Biederman 2020-06-23 21:54 ` [PATCH v2 2/6] exec: Lock more defensively in exec Eric W. Biederman @ 2020-06-23 21:54 ` Eric W. Biederman 2020-06-23 21:55 ` [PATCH v2 4/6] signal: In signal_group_exit remove the group_exit_task test Eric W. Biederman ` (2 subsequent siblings) 5 siblings, 0 replies; 14+ messages in thread From: Eric W. Biederman @ 2020-06-23 21:54 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Linus Torvalds, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger To allow signal_group_exit to be simplfied so that it can only test signal->flags. Add a new flag SIGNAL_GROUP_DETHREAD, that is set and cleared where de_thread sets and clears group_exit_task today. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 3 +++ include/linux/sched/signal.h | 1 + 2 files changed, 4 insertions(+) diff --git a/fs/exec.c b/fs/exec.c index 33b5d9229c01..9c4c1ab8f715 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1145,6 +1145,7 @@ static int de_thread(struct task_struct *tsk) return -EAGAIN; } + sig->flags |= SIGNAL_GROUP_DETHREAD; sig->group_exit_task = tsk; sig->notify_count = zap_other_threads(tsk); if (!thread_group_leader(tsk)) @@ -1244,6 +1245,7 @@ static int de_thread(struct task_struct *tsk) } spin_lock_irq(lock); + sig->flags &= ~SIGNAL_GROUP_DETHREAD; sig->group_exit_task = NULL; sig->notify_count = 0; spin_unlock_irq(lock); @@ -1259,6 +1261,7 @@ static int de_thread(struct task_struct *tsk) /* protects against exit_notify() and __exit_signal() */ read_lock_irq(&tasklist_lock); spin_lock(lock); + sig->flags &= ~SIGNAL_GROUP_DETHREAD; sig->group_exit_task = NULL; sig->notify_count = 0; spin_unlock(lock); diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index b4f36a11be5e..5ff8697b21cd 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -253,6 +253,7 @@ struct signal_struct { /* Signal group actions. */ #define SIGNAL_GROUP_EXIT 0x00000100 /* group exit in progress */ #define SIGNAL_GROUP_COREDUMP 0x00000200 /* coredump in progress */ +#define SIGNAL_GROUP_DETHREAD 0x00000400 /* exec de_thread in progress */ /* Flags applicable to the entire signal group. */ #define SIGNAL_UNKILLABLE 0x00001000 /* for init: ignore fatal signals */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/6] signal: In signal_group_exit remove the group_exit_task test 2020-06-23 21:52 ` [PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman ` (2 preceding siblings ...) 2020-06-23 21:54 ` [PATCH v2 3/6] signal: Implement SIGNAL_GROUP_DETHREAD Eric W. Biederman @ 2020-06-23 21:55 ` Eric W. Biederman 2020-06-23 21:55 ` [PATCH v2 5/6] coredump: Stop using group_exit_task Eric W. Biederman 2020-06-23 21:56 ` [PATCH v2 6/6] exec: Rename group_exit_task group_exec_task and correct the Documentation Eric W. Biederman 5 siblings, 0 replies; 14+ messages in thread From: Eric W. Biederman @ 2020-06-23 21:55 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Linus Torvalds, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger There are two places where signal_group_exit are set. In the fs/exec.c:de_thread() and in fs/coredump.c:zap_threads(). The coredump usage of group_exit_task was explicitly added[1] so that signal_group_exit() would return true during a coredump. When examining the coredump usage it turns out that SIGNAL_GROUP_COREDUMP is set in all of the same places as group_exit_task. So signal_group_exit can test SIGNAL_GROUP_COREDUMP and achieve the same results with respect to coredumps as testing group_exit_task. Similarly the exec code sets and clears SIGNAL_GROUP_DETHREAD in all of the places where group_exit_task is set and cleared. So test SIGNAL_GROUP_COREDUMP | SIGNAL_GROUP_DETHREAD instead of group_exit_task. Cc: Oleg Nesterov <oleg@redhat.com> [1] 6cd8f0acae34 ("coredump: ensure that SIGKILL always kills the dumping thread") Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- include/linux/sched/signal.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 5ff8697b21cd..43822e2b63e6 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -268,8 +268,9 @@ static inline void signal_set_stop_flags(struct signal_struct *sig, /* If true, all threads except ->group_exit_task have pending SIGKILL */ static inline int signal_group_exit(const struct signal_struct *sig) { - return (sig->flags & SIGNAL_GROUP_EXIT) || - (sig->group_exit_task != NULL); + return (sig->flags & (SIGNAL_GROUP_EXIT | + SIGNAL_GROUP_COREDUMP | + SIGNAL_GROUP_DETHREAD)); } extern void flush_signals(struct task_struct *); -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 5/6] coredump: Stop using group_exit_task 2020-06-23 21:52 ` [PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman ` (3 preceding siblings ...) 2020-06-23 21:55 ` [PATCH v2 4/6] signal: In signal_group_exit remove the group_exit_task test Eric W. Biederman @ 2020-06-23 21:55 ` Eric W. Biederman 2020-06-23 21:56 ` [PATCH v2 6/6] exec: Rename group_exit_task group_exec_task and correct the Documentation Eric W. Biederman 5 siblings, 0 replies; 14+ messages in thread From: Eric W. Biederman @ 2020-06-23 21:55 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Linus Torvalds, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger Setting and clearing of group_exit_task in the coredump code was added to affect the outcome of signal_group_exit()[1]. The coredump code has not grown any other uses for setting group_exit_task since. Now that signal_group_exit() no longer tests group_exit_task stop setting and clearing it. [1] 6cd8f0acae34 ("coredump: ensure that SIGKILL always kills the dumping thread") Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/coredump.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/fs/coredump.c b/fs/coredump.c index 7237f07ff6be..37b71c72ab3a 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -369,7 +369,6 @@ static int zap_threads(struct task_struct *tsk, struct mm_struct *mm, spin_lock_irq(&tsk->sighand->siglock); if (!signal_group_exit(tsk->signal)) { mm->core_state = core_state; - tsk->signal->group_exit_task = tsk; nr = zap_process(tsk, exit_code, 0); clear_tsk_thread_flag(tsk, TIF_SIGPENDING); } @@ -481,7 +480,6 @@ static void coredump_finish(struct mm_struct *mm, bool core_dumped) spin_lock_irq(¤t->sighand->siglock); if (core_dumped && !__fatal_signal_pending(current)) current->signal->group_exit_code |= 0x80; - current->signal->group_exit_task = NULL; current->signal->flags = SIGNAL_GROUP_EXIT; spin_unlock_irq(¤t->sighand->siglock); -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 6/6] exec: Rename group_exit_task group_exec_task and correct the Documentation 2020-06-23 21:52 ` [PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman ` (4 preceding siblings ...) 2020-06-23 21:55 ` [PATCH v2 5/6] coredump: Stop using group_exit_task Eric W. Biederman @ 2020-06-23 21:56 ` Eric W. Biederman 5 siblings, 0 replies; 14+ messages in thread From: Eric W. Biederman @ 2020-06-23 21:56 UTC (permalink / raw) To: linux-kernel Cc: linux-fsdevel, Linus Torvalds, Oleg Nesterov, Jann Horn, Kees Cook, Bernd Edlinger Rename group_exit_task to group_exec_task to make it clear this field is only used during exec. Update the comments for the fields group_exec_task and notify_count as they are only used by exec. Notifications to the execing task aka group_exec_task happen at 0 and -1. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 8 ++++---- include/linux/sched/signal.h | 11 ++++------- kernel/exit.c | 4 ++-- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 9c4c1ab8f715..c594af64acd2 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1146,7 +1146,7 @@ static int de_thread(struct task_struct *tsk) } sig->flags |= SIGNAL_GROUP_DETHREAD; - sig->group_exit_task = tsk; + sig->group_exec_task = tsk; sig->notify_count = zap_other_threads(tsk); if (!thread_group_leader(tsk)) sig->notify_count--; @@ -1175,7 +1175,7 @@ static int de_thread(struct task_struct *tsk) spin_lock(lock); /* * Do this under tasklist_lock to ensure that - * exit_notify() can't miss ->group_exit_task + * exit_notify() can't miss ->group_exec_task */ sig->notify_count = -1; if (likely(leader->exit_state)) @@ -1246,7 +1246,7 @@ static int de_thread(struct task_struct *tsk) spin_lock_irq(lock); sig->flags &= ~SIGNAL_GROUP_DETHREAD; - sig->group_exit_task = NULL; + sig->group_exec_task = NULL; sig->notify_count = 0; spin_unlock_irq(lock); @@ -1262,7 +1262,7 @@ static int de_thread(struct task_struct *tsk) read_lock_irq(&tasklist_lock); spin_lock(lock); sig->flags &= ~SIGNAL_GROUP_DETHREAD; - sig->group_exit_task = NULL; + sig->group_exec_task = NULL; sig->notify_count = 0; spin_unlock(lock); read_unlock_irq(&tasklist_lock); diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 43822e2b63e6..ad6b209b1363 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -98,13 +98,10 @@ struct signal_struct { /* thread group exit support */ int group_exit_code; - /* overloaded: - * - notify group_exit_task when ->count is equal to notify_count - * - everyone except group_exit_task is stopped during signal delivery - * of fatal signals, group_exit_task processes the signal. - */ + + /* exec support, notify group_exec_task when notify_count is 0 or -1 */ int notify_count; - struct task_struct *group_exit_task; + struct task_struct *group_exec_task; /* thread group stop support, overloads group_exit_code too */ int group_stop_count; @@ -265,7 +262,7 @@ static inline void signal_set_stop_flags(struct signal_struct *sig, sig->flags = (sig->flags & ~SIGNAL_STOP_MASK) | flags; } -/* If true, all threads except ->group_exit_task have pending SIGKILL */ +/* If true, all threads except ->group_exec_task have pending SIGKILL */ static inline int signal_group_exit(const struct signal_struct *sig) { return (sig->flags & (SIGNAL_GROUP_EXIT | diff --git a/kernel/exit.c b/kernel/exit.c index 727150f28103..4206d33b4904 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -115,7 +115,7 @@ static void __exit_signal(struct task_struct *tsk) * then notify it: */ if (sig->notify_count > 0 && !--sig->notify_count) - wake_up_process(sig->group_exit_task); + wake_up_process(sig->group_exec_task); if (tsk == sig->curr_target) sig->curr_target = next_thread(tsk); @@ -672,7 +672,7 @@ static void exit_notify(struct task_struct *tsk, int group_dead) /* mt-exec, de_thread() is waiting for group leader */ if (unlikely(tsk->signal->notify_count < 0)) - wake_up_process(tsk->signal->group_exit_task); + wake_up_process(tsk->signal->group_exec_task); write_unlock_irq(&tasklist_lock); list_for_each_entry_safe(p, n, &dead, ptrace_entry) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-06-23 22:00 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-19 18:30 [PATCH 0/2] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman 2020-06-19 18:32 ` [PATCH 1/2] exec: Don't set group_exit_task during a coredump Eric W. Biederman 2020-06-20 18:58 ` Linus Torvalds 2020-06-22 16:20 ` Eric W. Biederman 2020-06-22 16:32 ` Linus Torvalds 2020-06-22 11:25 ` Oleg Nesterov 2020-06-19 18:33 ` [PATCH 2/2] exec: Rename group_exit_task group_exec_task and correct the Documentation Eric W. Biederman 2020-06-23 21:52 ` [PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity Eric W. Biederman 2020-06-23 21:53 ` [PATCH v2 1/6] signal: Pretty up the SIGNAL_GROUP_FLAGS Eric W. Biederman 2020-06-23 21:54 ` [PATCH v2 2/6] exec: Lock more defensively in exec Eric W. Biederman 2020-06-23 21:54 ` [PATCH v2 3/6] signal: Implement SIGNAL_GROUP_DETHREAD Eric W. Biederman 2020-06-23 21:55 ` [PATCH v2 4/6] signal: In signal_group_exit remove the group_exit_task test Eric W. Biederman 2020-06-23 21:55 ` [PATCH v2 5/6] coredump: Stop using group_exit_task Eric W. Biederman 2020-06-23 21:56 ` [PATCH v2 6/6] exec: Rename group_exit_task group_exec_task and correct the Documentation Eric W. Biederman
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).