Re: + prctl-add-pr_setget_child_reaper-to-allow-simple-process-supervision .patch added to -mm tree

[Oleg Nesterov <oleg@redhat.com>]

On 08/17, Kay Sievers wrote:
>
> On Wed, Aug 17, 2011 at 13:55, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > I try to never argue with the new features. But to be honest, this
> > doesn't look very good to me.
> >
> > OK, a service manager M does prctl(PR_SET_CHILD_REAPER), then it forks
> > a service X which forks another child C and exits. Then C exits and
> > notifies M.
> >
> > But. How can M know that the service X should be restarted? It only
> > knows the pid.
>
> Legacy services write pid files and we read them, so we know the pid
> to watch for. Proper services never double-fork and reparent in a
> modern init environment.

OK. So, this patch can only help to handle the legacy services? And
the service should participate (write pid files for example). And,

>
> > What if wait(WEXITED) succeeds because C in turn does
> > fork + exit?
>
> Nothing is really doing this.

OK. But this means you propose this patch to solve the very specific
problems. IOW, imho this doesn't look very useful "in general" to me.

May be we need something else instead... And iiuc you don't really
need to change the reparenting, you only want the notification if
the process exits.



> >> @@ -1296,6 +1296,8 @@ struct task_struct {
> >>                                * execve */
> >>       unsigned in_iowait:1;
> >>
> >> +     /* Reparent child processes to this process instead of pid 1. */
> >> +     unsigned child_reaper:1;
> >
> > First of all - this is already very wrong imho. This should be
> > per-process, not per-thread.
>
> What do you mean? That would go where instead?

You should mark the whole process as sub-reaper, not a single thread
which does prctl(). The parent/child relationship is process-wide.

If nothing else. Suppose that application does pthread_create(), the
new thread does prctl(REAPER) and exits.

> >> +     /* find the first ancestor which is marked as child_reaper */
> >> +     for (reaper = father->parent;
> >> +          reaper != &init_task && reaper != pid_ns->child_reaper;
> >> +          reaper = reaper->parent)
> >
> > This loop can never reach init_task/child_reaper and crash the kernel.
>
> You mean: *if* this loop can never ...?

Yes.

> > For example, father->parent can point to init_task's sub-thread.
> >
> > OTOH you shouldn't use init_task at all.
>
> What would we use instead?

You should check ->child_reaper only. But see above, it can be multithreaded.

> > Also. You shouldn't do this if the sub-namespace init exits, this is
> > wrong.
>
> It we find a sub-init, before the namespace PID1, why wouldn't we return it?

Ah, I meant pid_ns->child_reaper, not task->child_reaper.

If pid_ns->child_reaper exits we should never try to "reparent" its
children, see zap_pid_ns_processes() in particular. IOW, this should
go into the "else" branch of "if (pid_ns->child_reaper == father)"

Oleg.