Messages in this thread Patch in this message | | | From | (Eric W. Biederman) | Date | Fri, 18 May 2012 18:03:40 -0600 | Subject | Re: [PATCH 2/3] pidns: Guarantee that the pidns init will be the last pidns process reaped. |
| |
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(¤t->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
| |