<p dir="ltr"></p>
<p dir="ltr">On Jul 22, 2016 5:17 AM, "Pavel Emelyanov" <<a href="mailto:xemul@virtuozzo.com">xemul@virtuozzo.com</a>> wrote:<br>
><br>
> On 07/13/2016 06:10 PM, Tycho Andersen wrote:<br>
> > For example, if a zombie has a helper that sets up its session id, the<br>
> > zombie will be reparented to the init task, which will then potentially get<br>
> > a SIGCHLD for a task which isn't its direct child zombie, which we didn't<br>
> > handle. Instead, let's recursively find all the zombies for the init task,<br>
> > in case they get reparented this way.<br>
> ><br>
> > This commit is a little ugly: we are now passing three lists of pids<br>
> > (helpers, zombies, and things that are allowed to die) to the restorer blob<br>
> > for each task. We do need to have some way to distinguish, because:<br>
> ><br>
> > * we want to wait on helpers<br>
> > * we don't want to wait on zombies (i.e. we want to keep the pid dead), but<br>
> > confirm that it actually died<br>
> > * we don't want to wait on anything that gets reparented, but we don't want<br>
> > to error out if we get a SIGCHLD for it<br>
> ><br>
> > We could introduce a new struct:<br>
> ><br>
> > struct pid_info {<br>
> > pid_t pid;<br>
> > bool zombie;<br>
> > bool direct_child;<br>
> > }<br>
><br>
> Let's try to go this route, but slightly ... modified.<br>
><br>
> struct restorer_pid_info {<br>
> pid_t pid;<br>
> unsigned type;<br>
> }<br>
><br>
> The type would be ZOMBIE, HELPER or OTHER.<br>
><br>
> Next, instead of calling collect_zombie_pids, collect_helper_pids and<br>
> collect_allowed_to_die_pids, let's call one collect_restorer_pids()<br>
> that would fill the array of restorer_pid_info-s with types and, if<br>
> the caller is root_item, would dive into recursion only collecting for<br>
> TASK_DEAD entries.<br>
><br>
> In the restorer itself we would need to merge wait_zombies() and<br>
> wait_helpers() to only walk the list once and tune the sigchild helper<br>
> to allow for sigchilds from any pid in the array.<br>
><br>
> What do you think?</p>
<p dir="ltr">Sounds good, except I am on vacation starting today and through the end of next week :-). Feel free to revert the failing test and I can resend both when I finish this.</p>
<p dir="ltr">Tycho <br>
><br>
> -- Pavel<br>
><br>
> > to handle this instead of the three separate lists. I'm not sure which is<br>
> > cleaner, but I'd be happy to refactor to the other way if that's better.<br>
> ><br>
> > v2: only the zombies need to be recursively collected, helpers wait on<br>
> > their children before they exit and will never be reparented<br>
> ><br>
> > Signed-off-by: Tycho Andersen <<a href="mailto:tycho.andersen@canonical.com">tycho.andersen@canonical.com</a>><br>
> > ---<br>
> > criu/cr-restore.c | 31 ++++++++++++++++++++++++++-----<br>
> > criu/include/restorer.h | 3 +++<br>
> > criu/pie/restorer.c | 20 ++++++--------------<br>
> > 3 files changed, 35 insertions(+), 19 deletions(-)<br>
> ><br>
> > diff --git a/criu/cr-restore.c b/criu/cr-restore.c<br>
> > index 977e049..d8e4cf1 100644<br>
> > --- a/criu/cr-restore.c<br>
> > +++ b/criu/cr-restore.c<br>
> > @@ -487,14 +487,16 @@ static int prepare_sigactions(void)<br>
> > return ret;<br>
> > }<br>
> ><br>
> > -static int collect_child_pids(int state, unsigned int *n)<br>
> > +static int collect_child_pids(struct pstree_item *root, int state, unsigned int *n, bool recurse)<br>
> > {<br>
> > struct pstree_item *pi;<br>
> ><br>
> > - *n = 0;<br>
> > - list_for_each_entry(pi, &current->children, sibling) {<br>
> > + list_for_each_entry(pi, &root->children, sibling) {<br>
> > pid_t *child;<br>
> ><br>
> > + if (recurse && collect_child_pids(pi, state, n, recurse) < 0)<br>
> > + return -1;<br>
> > +<br>
> > if (pi->pid.state != state)<br>
> > continue;<br>
> ><br>
> > @@ -512,13 +514,28 @@ static int collect_child_pids(int state, unsigned int *n)<br>
> > static int collect_helper_pids(struct task_restore_args *ta)<br>
> > {<br>
> > ta->helpers = (pid_t *)rst_mem_align_cpos(RM_PRIVATE);<br>
> > - return collect_child_pids(TASK_HELPER, &ta->helpers_n);<br>
> > + ta->helpers_n = 0;<br>
> > + return collect_child_pids(current, TASK_HELPER, &ta->helpers_n, false);<br>
> > }<br>
> ><br>
> > static int collect_zombie_pids(struct task_restore_args *ta)<br>
> > {<br>
> > ta->zombies = (pid_t *)rst_mem_align_cpos(RM_PRIVATE);<br>
> > - return collect_child_pids(TASK_DEAD, &ta->zombies_n);<br>
> > + ta->zombies_n = 0;<br>
> > + return collect_child_pids(current, TASK_DEAD, &ta->zombies_n, false);<br>
> > +}<br>
> > +<br>
> > +static int collect_allowed_to_die_pids(struct task_restore_args *ta)<br>
> > +{<br>
> > + ta->allowed_to_die = (pid_t *)rst_mem_align_cpos(RM_PRIVATE);<br>
> > + ta->allowed_to_die_n = 0;<br>
> > +<br>
> > + if (collect_child_pids(current, TASK_DEAD, &ta->allowed_to_die_n, current == root_item) < 0)<br>
> > + return -1;<br>
> > + if (collect_child_pids(current, TASK_HELPER, &ta->allowed_to_die_n, false) < 0)<br>
> > + return -1;<br>
> > +<br>
> > + return 0;<br>
> > }<br>
> ><br>
> > static int open_core(int pid, CoreEntry **pcore)<br>
> > @@ -655,6 +672,9 @@ static int restore_one_alive_task(int pid, CoreEntry *core)<br>
> > if (collect_zombie_pids(ta) < 0)<br>
> > return -1;<br>
> ><br>
> > + if (collect_allowed_to_die_pids(ta) < 0)<br>
> > + return -1;<br>
> > +<br>
> > if (inherit_fd_fini() < 0)<br>
> > return -1;<br>
> ><br>
> > @@ -3003,6 +3023,7 @@ static int sigreturn_restore(pid_t pid, struct task_restore_args *task_args, uns<br>
> > RST_MEM_FIXUP_PPTR(task_args->helpers);<br>
> > RST_MEM_FIXUP_PPTR(task_args->zombies);<br>
> > RST_MEM_FIXUP_PPTR(task_args->seccomp_filters);<br>
> > + RST_MEM_FIXUP_PPTR(task_args->allowed_to_die);<br>
> ><br>
> > if (core->tc->has_seccomp_mode)<br>
> > task_args->seccomp_mode = core->tc->seccomp_mode;<br>
> > diff --git a/criu/include/restorer.h b/criu/include/restorer.h<br>
> > index 7c0336e..ac50bf0 100644<br>
> > --- a/criu/include/restorer.h<br>
> > +++ b/criu/include/restorer.h<br>
> > @@ -149,6 +149,9 @@ struct task_restore_args {<br>
> > struct sock_fprog *seccomp_filters;<br>
> > unsigned int seccomp_filters_n;<br>
> ><br>
> > + pid_t *allowed_to_die;<br>
> > + unsigned int allowed_to_die_n;<br>
> > +<br>
> > /* * * * * * * * * * * * * * * * * * * * */<br>
> ><br>
> > unsigned long task_size;<br>
> > diff --git a/criu/pie/restorer.c b/criu/pie/restorer.c<br>
> > index 424ccf3..d3a24d8 100644<br>
> > --- a/criu/pie/restorer.c<br>
> > +++ b/criu/pie/restorer.c<br>
> > @@ -57,10 +57,8 @@<br>
> ><br>
> > static struct task_entries *task_entries;<br>
> > static futex_t thread_inprogress;<br>
> > -static pid_t *helpers;<br>
> > -static int n_helpers;<br>
> > -static pid_t *zombies;<br>
> > -static int n_zombies;<br>
> > +static pid_t *allowed_to_die;<br>
> > +static int n_allowed_to_die;<br>
> ><br>
> > extern void cr_restore_rt (void) asm ("__cr_restore_rt")<br>
> > __attribute__ ((visibility ("hidden")));<br>
> > @@ -72,12 +70,8 @@ static void sigchld_handler(int signal, siginfo_t *siginfo, void *data)<br>
> ><br>
> > /* We can ignore helpers that die, we expect them to after<br>
> > * CR_STATE_RESTORE is finished. */<br>
> > - for (i = 0; i < n_helpers; i++)<br>
> > - if (siginfo->si_pid == helpers[i])<br>
> > - return;<br>
> > -<br>
> > - for (i = 0; i < n_zombies; i++)<br>
> > - if (siginfo->si_pid == zombies[i])<br>
> > + for (i = 0; i < n_allowed_to_die; i++)<br>
> > + if (siginfo->si_pid == allowed_to_die[i])<br>
> > return;<br>
> ><br>
> > if (siginfo->si_code & CLD_EXITED)<br>
> > @@ -1106,10 +1100,8 @@ long __export_restore_task(struct task_restore_args *args)<br>
> > #endif<br>
> ><br>
> > task_entries = args->task_entries;<br>
> > - helpers = args->helpers;<br>
> > - n_helpers = args->helpers_n;<br>
> > - zombies = args->zombies;<br>
> > - n_zombies = args->zombies_n;<br>
> > + allowed_to_die = args->allowed_to_die;<br>
> > + n_allowed_to_die = args->allowed_to_die_n;<br>
> > *args->breakpoint = rst_sigreturn;<br>
> ><br>
> > ksigfillset(&act.rt_sa_mask);<br>
> ><br>
></p>