lkml.org 
[lkml]   [2012]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [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(&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


\
 
 \ /
  Last update: 2012-05-19 02:21    [W:0.389 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site