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

[Oleg Nesterov <oleg@redhat.com>]

On 05/18, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> 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
zap_pid_ns_processes().

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

Oleg.