Messages in this thread | | | Date | Wed, 17 Aug 2011 13:55:43 +0200 | From | Oleg Nesterov <> | Subject | Re: + prctl-add-pr_setget_child_reaper-to-allow-simple-process-supervision .patch added to -mm tree |
| |
On 08/16, Andrew Morton wrote: > > From: Lennart Poettering <lennart@poettering.net> > > Userspace service managers/supervisors need to track their started > services. Many services daemonize by double-forking and get implicitely > re-parented to PID 1. The process manager will no longer be able to > receive the SIGCHLD signals for them. > > With this prctl, a service manager can mark itself as a sort of 'sub-init' > process, able to stay as the parent process for all processes created by > the started services. All SIGCHLD signals will be delivered to the > service manager.
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. What if wait(WEXITED) succeeds because C in turn does fork + exit? What M has 2 or more services?
Anyway, the implementation is certainly buggy.
> @@ -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.
> + /* 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. For example, father->parent can point to init_task's sub-thread.
OTOH you shouldn't use init_task at all.
Also. You shouldn't do this if the sub-namespace init exits, this is wrong.
> + if (reaper->child_reaper) > + return reaper;
No, we can't blindly return this task, it can be dead/exiting. More precisely, we must not do this if it has already passed forget_original_parent(). That is why the code above checks PF_EXITING.
Oleg.
| |