[PATCH 0/2] exec: s/group_exit_task/group_exec_task/ for clarity

[PATCH 0/2] exec: s/group_exit_task/group_exec_task/ for clarity

[Eric W. Biederman]

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(-)

[PATCH 1/2] exec: Don't set group_exit_task during a coredump

[Eric W. Biederman]

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(&current->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(&current->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

[PATCH 2/2] exec: Rename group_exit_task group_exec_task and correct the Documentation

[Eric W. Biederman]

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

Re: [PATCH 1/2] exec: Don't set group_exit_task during a coredump

[Linus Torvalds]

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

Re: [PATCH 1/2] exec: Don't set group_exit_task during a coredump

[Oleg Nesterov]

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(&current->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(&current->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.

Re: [PATCH 1/2] exec: Don't set group_exit_task during a coredump

[Eric W. Biederman]

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

Re: [PATCH 1/2] exec: Don't set group_exit_task during a coredump

[Linus Torvalds]

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

[PATCH v2 0/6] exec: s/group_exit_task/group_exec_task/ for clarity

[Eric W. Biederman]

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(-)

[PATCH v2 1/6] signal: Pretty up the SIGNAL_GROUP_FLAGS

[Eric W. Biederman]

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

[PATCH v2 2/6] exec: Lock more defensively in exec

[Eric W. Biederman]

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

[PATCH v2 3/6] signal: Implement SIGNAL_GROUP_DETHREAD

[Eric W. Biederman]

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

[PATCH v2 4/6] signal: In signal_group_exit remove the group_exit_task test

[Eric W. Biederman]

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

[PATCH v2 5/6] coredump: Stop using group_exit_task

[Eric W. Biederman]

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(&current->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(&current->sighand->siglock);
 
-- 
2.20.1

[PATCH v2 6/6] exec: Rename group_exit_task group_exec_task and correct the Documentation

[Eric W. Biederman]

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