Re: [patch] Re: Problem with exiting threads under NPTL

[Linus Torvalds]

Ingo,

I'm being anal for 2.6.0 again and trying to think of all the
ramifications of the changes, and I found a serious bug in your patch:

Even though the parent ignores SIGCHLD it _can_ be running on another CPU
in "wait4()". And since we drop the tasklist lock before we do the
"release_task()" on the leader, and since the leader is still marked
TASK_ZOMBIE, we may have _two_ different people trying to release it.
First the parent, and then the last thread that still remembers the
leader.

Note that we at this point have not unhashed the thread group leader yet:
we just unhashed the last thread. So we're dropping the task list lock,
and the leader pointer is _not_ safe to keep that way.

We've had that race before, and the fix is to mark the leader TASK_DEAD
while still under the tasklist lock - that way the parent won't see the
soon-to-be-reaped leader even if it were to happen to be in the middle of
a wait4() call and get in while the tasklist lock is released.

So how about this patch? It has this bug fixed, and is otherwise a mix of
your two patches, along with an added sanity check ("the exit_signal for a
thread group leader cannot be -1 until the last thread exited"), which
makes the test make a whole lot more sense in my opinion (the previous
test for "exit_signal != -1" looks nonsensical, and appears to be a
cut-and-paste from somewhere else).

Roland, Petr, comments? Can you see any hole in my logic?

		Linus

----
--- 1.119/kernel/exit.c	Mon Oct 27 11:49:59 2003
+++ edited/kernel/exit.c	Sun Dec 14 14:00:10 2003
@@ -49,9 +49,11 @@

 void release_task(struct task_struct * p)
 {
+	int zap_leader;
 	task_t *leader;
 	struct dentry *proc_dentry;
-
+
+repeat:
 	BUG_ON(p->state < TASK_ZOMBIE);

 	atomic_dec(&p->user->processes);
@@ -70,10 +72,24 @@
 	 * group, and the leader is zombie, then notify the
 	 * group leader's parent process. (if it wants notification.)
 	 */
+	zap_leader = 0;
 	leader = p->group_leader;
-	if (leader != p && thread_group_empty(leader) &&
-		    leader->state == TASK_ZOMBIE && leader->exit_signal != -1)
+	if (leader != p && thread_group_empty(leader) && leader->state == TASK_ZOMBIE) {
+		BUG_ON(leader->exit_signal == -1);
 		do_notify_parent(leader, leader->exit_signal);
+		/*
+		 * If we were the last child thread and the leader has
+		 * exited already, and the leader's parent ignores SIGCHLD,
+		 * then we are the one who should release the leader.
+		 *
+		 * do_notify_parent() will have marked it self-reaping in
+		 * that case.
+		 */
+		if (leader->exit_signal == -1) {
+			zap_leader = 1;
+			leader->state = TASK_DEAD;
+		}
+	}

 	p->parent->cutime += p->utime + p->cutime;
 	p->parent->cstime += p->stime + p->cstime;
@@ -88,6 +104,10 @@
 	proc_pid_flush(proc_dentry);
 	release_thread(p);
 	put_task_struct(p);
+
+	p = leader;
+	if (zap_leader)
+		goto repeat;
 }

 /* we are using it only for SMP init */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/