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