Re: [PATCH 2/2] ptrace children revamp

[Oleg Nesterov <oleg@tv-sign.ru>]

On 03/29, Oleg Nesterov wrote:
>
> > -	list_for_each_entry_safe(p, n, &father->ptrace_children, ptrace_list) {
> >  		p->real_parent = reaper;
> > -		reparent_thread(p, father, 1);
> > +		if (p->parent == father) {
> > +			ptrace_unlink(p);
> > +			p->parent = p->real_parent;
> > +		}
> > +		reparent_thread(p, father);
> 
> Unless I missed something again, this is not 100% right.
> 
> Suppose that we are ->real_parent, the child was traced by us, we ignore
> SIGCHLD, and we are doing the threaded reparenting. In that case we should
> release the child if it is dead, like the current code does.
> 
> Even if we don't ignore SIGCHLD, we should notify our thread-group, but
> reparent_thread() skips do_notify_parent() when the new parent is from
> is from the same thread-group.
> 
> Note also that reparent_thread() has a very old bug. When it sees the
> EXIT_ZOMBIE task, it notifies the new parent, but doesn't check if the task
> becomes detached because the new parent ignores SIGCHLD. Currently this means
> that init must have a handler for SIGCHLD or we leak zombies.

Also, I think ptrace_exit() is not right,

	if (p->exit_signal != -1 && !thread_group_empty(p))
		do_notify_parent(p, p->exit_signal);

note the "!thread_group_empty()" above, I guess this is typo, thread group
must be empty if we are going to release the task or notify its parent.


IOW, perhaps something like the patch below makes sense.

Oleg.

--- x/kernel/exit.c~x	2008-03-29 17:14:54.000000000 +0300
+++ x/kernel/exit.c	2008-03-29 17:28:17.000000000 +0300
@@ -596,6 +596,16 @@ static void exit_mm(struct task_struct *
 	mmput(mm);
 }
 
+static void xxx(struct task_struct *p, struct list_head *dead)
+{
+	if (p->exit_state == EXIT_ZOMBIE) {
+		if (p->exit_signal != -1 && thread_group_empty(p))
+			do_notify_parent(p, p->exit_signal);
+		if (p->exit_signal == -1)
+			list_add(&p->ptrace_list, dead);
+	}
+}
+
 /*
  * Detach any ptrace attachees (not our own natural children).
  * Any that need to be release_task'd are put on the @dead list.
@@ -616,12 +626,7 @@ static void ptrace_exit(struct task_stru
 		 * reap itself, add it to the @dead list.  We can't call
 		 * release_task() here because we already hold tasklist_lock.
 		 */
-		if (p->exit_state == EXIT_ZOMBIE) {
-			if (p->exit_signal != -1 && !thread_group_empty(p))
-				do_notify_parent(p, p->exit_signal);
-			if (p->exit_signal == -1)
-				list_add(&p->ptrace_list, dead);
-		}
+		 xxx(p, dead);
 	}
 }
 
@@ -661,13 +666,6 @@ static void reparent_thread(struct task_
 	if (p->exit_signal != -1)
 		p->exit_signal = SIGCHLD;
 
-	/* If we'd notified the old parent about this child's death,
-	 * also notify the new parent.
-	 */
-	if (p->exit_state == EXIT_ZOMBIE &&
-	    p->exit_signal != -1 && thread_group_empty(p))
-		do_notify_parent(p, p->exit_signal);
-
 	/*
 	 * process group orphan check
 	 * Case ii: Our child is in a different pgrp
@@ -720,6 +718,7 @@ static void forget_original_parent(struc
 			p->parent = p->real_parent;
 		}
 		reparent_thread(p, father);
+		xxx(p, &ptrace_dead);
 	}
 
 	write_unlock_irq(&tasklist_lock);