[lkml]   [2012]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH 2/3] pidns: Guarantee that the pidns init will be the last pidns process reaped.
    On 05/18, Eric W. Biederman wrote:
    > Oleg Nesterov <> writes:
    > >> 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

    Yes, and we need this loop anyway, even if SIGCHLD is ignored.
    It is possible that we already have a EXIT_ZOMBIE child(s) when

    > but I think there is a race possible there.

    Hmm. I do not see any race, but perhaps I missed something.
    I think we can trust -ECHILD, or do_wait() is buggy.

    Hmm. But there is another (off-topic) problem, security_task_wait()
    can return an error if there are some security policy problems...
    OK, this shouldn't happen I hope.

    > > 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.

    Yes. so we need wait_event_interruptible() or __unhash_process()
    should use __wake_up_sync_key(wait_chldexit).

    > > 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.

    Agreed. ptrace API is nasty ;)

    > and moving detach_pid so we don't have to be super careful about
    > where we call task_active_pid_ns.

    Yes, I was thinking about this change too,

    > --- 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();

    OK, but then it makes sense to add clear_thread_flag(TIF_SIGPENDING)
    before schedule, to avoid the busy-wait loop (like the sys_wait4 loop
    does). Or simply use TASK_UNINTERRUPTIBLE, I do not think it is that
    important to "fool" /proc/loadavg. But I am fine either way.

    Maybe you can also add "ifdef CONFIG_PID_NS" into __unhash_process(),
    but this is minor too.


     \ /
      Last update: 2012-05-21 15:21    [W:0.030 / U:3.968 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site