Re: [PATCH 2/3] pidns: Guarantee that the pidns init will be the last pidns process reaped.

[ebiederm@xmission.com (Eric W. Biederman)]

Oleg Nesterov <oleg@redhat.com> writes:

> On 05/17, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > What do you think?
>>
>> I think there is something very compelling about your solution,
>> we do need my bit about making the init process ignore SIGCHLD
>> so all of init's children self reap.
>
> Not sure I understand. This can work with or without 3/3 which
> changes zap_pid_ns_processes() to ignore SIGCHLD. And just in
> case, I think 3/3 is fine.

The only issue I see is that without 3/3 we might have processes that
on one wait(2)s for and so will never have release_task called on.

We do have the wait loop but I think there is a race possible there.

> And once again, this wait_event() + __wake_up_parent() is very
> simple and straightforward, we can cleanup this code later if
> needed.

Yes, and it doesn't when you do an UNINTERRUPTIBLE sleep with
an INTERRUPTIBLE wake up unless I misread the code.

>> > Do you mean the "if (tsk->ptrace)" code in exit_notify() ? Nobody
>> > understand it ;) Last time this code was modified by me (iirc), but
>> > I simply tried to preserve the previous behaviour.
>>
>> Yes.  It is some pretty strange code.
>
> Yes. In particular, I think it should always use SIGCHLD.
>
>> Especially where we are reading
>> a return result which is always false.  I think there is a bug somewhere
>> between that code and ptrace detach
>
> Yes. This is the known oddity. We always notify the tracer if the
> leader exits, even if !thread_group_empty(). But after that the
> tracer can't detach, and it can't do do_wait(WEXITED).
>
> The problem is not that we can't "fix" this. Just any discussed
> fix adds the subtle/incompatible user-visible change.

Yes and that is nasty.

I need to sit down and write a good change log and do a bit more testing
(hopefully tonight) but this is what I have come up with so far.

It is based on your first version of the patch with a few changes
a TASK_INTERRUPTIBLE sleep so that we don't count in the load average,
and moving detach_pid so we don't have to be super careful about
where we call task_active_pid_ns.

Eric


---
 kernel/exit.c          |   13 ++++++++++++-
 kernel/pid_namespace.c |   11 +++++++++++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index d8bd3b42..abc4fc0 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -64,15 +64,26 @@ static void exit_mm(struct task_struct * tsk);
 static void __unhash_process(struct task_struct *p, bool group_dead)
 {
 	nr_threads--;
-	detach_pid(p, PIDTYPE_PID);
 	if (group_dead) {
+		struct task_struct *parent;
+
 		detach_pid(p, PIDTYPE_PGID);
 		detach_pid(p, PIDTYPE_SID);
 
 		list_del_rcu(&p->tasks);
 		list_del_init(&p->sibling);
 		__this_cpu_dec(process_counts);
+
+		/* If we are the last child process in a pid namespace
+		 * to be reaped notify the child_reaper.
+		 */
+		parent = p->real_parent;
+		if ((task_active_pid_ns(p)->child_reaper == parent) &&
+		    list_empty(&parent->children) &&
+		    (parent->flags & PF_EXITING))
+			wake_up_process(parent);
 	}
+	detach_pid(p, PIDTYPE_PID);
 	list_del_rcu(&p->thread_group);
 }
 
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index b98b0ed..ce96627 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -189,6 +189,17 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 		rc = sys_wait4(-1, NULL, __WALL, NULL);
 	} while (rc != -ECHILD);
 
+	read_lock(&tasklist_lock);
+	for (;;) {
+		__set_current_state(TASK_INTERRUPTIBLE);
+		if (list_empty(&current->children))
+			break;
+		read_unlock(&tasklist_lock);
+		schedule();
+		read_lock(&tasklist_lock);
+	}
+	read_unlock(&tasklist_lock);
+
 	if (pid_ns->reboot)
 		current->signal->group_exit_code = pid_ns->reboot;
 
-- 
1.7.5.4