Re: [PATCH 1/2] exit: reparent: fix/update the comments in zap_pid_ns_processes()

[Oleg Nesterov <oleg@redhat.com>]

On 11/24, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > The comments in zap_pid_ns_processes() are simply wrong, we need
> > to explain how this code actually works.
> >
> > 1. "Ignore SIGCHLD" looks like optimization but it is not, we also
> >    need this for correctness.
> >
> > 2. The comment above sys_wait4() could be more clear.
> >
> > 3. The comment about TASK_DEAD children is outdated. Contrary to
> >    what it says we do not need to make sure they all go away.
>
> We absolutely do need to wait until they all go away.

I can easily miss something, and that is why I asked you to review this
change. But if I missed something, perhaps we should update the comments?

This "wait until TASK_DEAD children" loop was added to ensure that
child_reaper can't be reaped before other tasks in this pid namespace.
6347e90091041e "pidns: guarantee that the pidns init will be the last pidns
process reaped".

But this was needed because proc_flush_task(pid == 1) called
kern_unmount(proc_mnt). After 0a01f2cc390e10633 "pidns: Make the pidns
proc mount/umount logic obvious" we can rely on schedule_work() in
free_pid(nr_hashed == 0).

So in any case I think that the current comment is outdated.


> - rusage will be wrong if we don't wait.

I already answered in 0/2, let me quote myself:

	Do you mean cstime/cutime/c* accounting?

	Firstly it is not clear what makes child_reaper special in _this_ sense, but
	this doesn't matter at all.

	The auotoreaping/EXIT_DEAD children are not accounted, only wait_task_zombie()
	accumulates these counters. (just in case, accounting in __exit_signal() is
	another thing).


> - We won't wait for an injected process in a pid namespace,
>   or a processes debugged with gdb to be reaped before the pid
>   init process exits if we don't wait.

Yes, and I do not see why this is bad, but this is off-topic.

Again, lets discuss this in another thread. This patch doesn't try to
document the desired semantics, it only tries to explain why zap_pid_ns
_has_ to wait until EXIT_DEAD (and in fact EXIT_ZOMBIE) tasks go away.


> The user visible semantics go from weird to completely insane if we
> relax the rule that the init process is the last process in a pid
> namespace.
>
> I don't see anything approaching a good reason for changing the user
> space semantics.

See above.

> > @@ -190,7 +190,11 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
> >  	/* Don't allow any more processes into the pid namespace */
> >  	disable_pid_allocation(pid_ns);
>
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> The above line disables injections of new processes in this pid
> namespace.

Yes, sure. See below.

> > -	/* Ignore SIGCHLD causing any terminated children to autoreap */
> > +	/*
> > +	 * Ignore SIGCHLD causing any terminated children to autoreap.
> > +	 * This speeds up the namespace shutdown, plus see the comment
> > +	 * below.
> > +	 */
>
> 	This speeds up the namespace shutdown and ensures that after we
> 	have waited for all existing zombies there will be no more
> 	zombies to wait for.

Afaics, no, see below.

> > -	 * sys_wait4() above can't reap the TASK_DEAD children.
> > -	 * Make sure they all go away, see free_pid().
> > +	 * sys_wait4() above can't reap the TASK_DEAD children but we do not
> > +	 * really care, we could reparent them to the global init.
>
> We do care.

See above. I only meant that nothing bad can happen from the kernel
perspective.

> > +	 * But this namespace can also have other tasks injected by setns().
> > +	 * Again, we do not really need to wait until they are all reaped,
>
> We do, and setns does not matter here.  Injection was stopped way up above.

I think that setns() does matter.

Yes injection was stopped. But a task T can be already injected before
disable_pid_allocation() was called.

Now it is killed (or it could even exit before). To simplify, suppose it
is already EXIT_ZOMBIE, although this doesn't really matter.

The sys_wait4() loop can't see it, it is not our child.

Now suppose that its parent doesn't do wait() but exits. This means that
the exiting parent will try to reparent T to pid_ns->child_reaper.

child_reaper already sleeps in "wait for nr_hashed == init_pids" loop, it
can do nothing with T.

So we rely on autoreaping (forced by ignored SIGCHLD), and this is the
(technical) reason why child_reaper can't exit: pid_ns->child_reaper should
be valid.

No?

> > +	 * We rely on ignored SIGCHLD, an injected zombie must be autoreaped
> > +	 * if reparented.
>
> Your new comment is about 90% wrong.

See above.

Eric. I'd really ask you to take another look. But in any case: thanks for
looking at this.

Oleg.