| Message ID | 87k1026h4x.fsf@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exec: s/group_exit_task/group_exec_task/ for clarity | expand |
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
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.
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
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
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); }
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(-)