[RFC,PATCH 3/3] do_wait: fix waiting for stopped group with dead leader

[RFC,PATCH 3/3] do_wait: fix waiting for stopped group with dead leader

[Oleg Nesterov]

do_wait(WSTOPPED) assumes that p->state must be == TASK_STOPPED, this is not
true if the leader is already dead. Check SIGNAL_STOP_STOPPED instead and use
->signal->group_exit_code.

This patch is not complete if not buggy. At the very minimum it needs cleanup.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- PT/kernel/exit.c~6_wait_stop	2007-12-07 20:25:40.000000000 +0300
+++ PT/kernel/exit.c	2007-12-08 20:30:55.000000000 +0300
@@ -1316,6 +1316,11 @@ static int wait_task_zombie(struct task_
 	return retval;
 }
 
+static inline int is_task_wait_stopped(struct task_struct *p)
+{
+	return	is_task_stopped_or_traced(p) ||
+		(p->signal->flags & SIGNAL_STOP_STOPPED);
+}
 /*
  * Handle sys_wait4 work for one task in state TASK_STOPPED.  We hold
  * read_lock(&tasklist_lock) on entry.  If we return zero, we still hold
@@ -1326,29 +1331,28 @@ static int wait_task_stopped(struct task
 			     int noreap, struct siginfo __user *infop,
 			     int __user *stat_addr, struct rusage __user *ru)
 {
-	int retval, exit_code, why;
+	int retval, *p_code, exit_code, why;
 	uid_t uid = 0; /* unneeded, required by compiler */
 	pid_t pid;
 
 	exit_code = 0;
 	spin_lock_irq(&p->sighand->siglock);
+	if (p->ptrace & PT_PTRACED) {
+		if (!is_task_stopped_or_traced(p))
+			goto unlock_sig;
+		p_code = &p->exit_code;
+	} else {
+		if (!(p->signal->flags & SIGNAL_STOP_STOPPED))
+			goto unlock_sig;
+		p_code = &p->signal->group_exit_code;
+	}
 
-	if (unlikely(!is_task_stopped_or_traced(p)))
-		goto unlock_sig;
-
-	if (!(p->ptrace & PT_PTRACED) && p->signal->group_stop_count > 0)
-		/*
-		 * A group stop is in progress and this is the group leader.
-		 * We won't report until all threads have stopped.
-		 */
-		goto unlock_sig;
-
-	exit_code = p->exit_code;
+	exit_code = *p_code;
 	if (!exit_code)
 		goto unlock_sig;
 
 	if (!noreap)
-		p->exit_code = 0;
+		*p_code = 0;
 
 	uid = p->uid;
 unlock_sig:
@@ -1472,7 +1476,7 @@ repeat:
 
 			if (unlikely(ret < 0)) {
 				retval = ret;
-			} else if (is_task_stopped_or_traced(p)) {
+			} else if (is_task_wait_stopped(p)) {
 				/*
 				 * It's stopped now, so it might later
 				 * continue, exit, or stop again.

Re: [RFC,PATCH 3/3] do_wait: fix waiting for stopped group with dead leader

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> do_wait(WSTOPPED) assumes that p->state must be == TASK_STOPPED, this is not
> true if the leader is already dead. Check SIGNAL_STOP_STOPPED instead and use
> ->signal->group_exit_code.
>
> This patch is not complete if not buggy. At the very minimum it needs cleanup.

Thinking about this set of problems.  Testing SIGNAL_STOP_STOPPED
seems more correct then testing TASK_STOPPED.  It ensures we don't
have a race, and except for ptrace the only way to stop a task
triggers SIGNAL_STOP_STOPPED.

We need a similar flag for thread group exit, to mark when every task
in the thread group has exited.

With those in place we can have race free tests of our status.
/proc/<tgid>/status needs to be updated to use those the per
signal struct status bits as well.

As for the exit_code, we set tsk->exit_code = sig->group_exit_code
so that doesn't seem to be a problem either.

So to get a task group status looking at bits on the signal struct
looks like the right approach, as this ensures we can avoid races in
setting the status, and we don't need to test a dozen other fields.

There is still some value in my other approach but even it will
have small races if we continue look at per task status bits when
what we want is a per thread group status.

Eric