[CRIU] [PATCH] restore: Set CLONE_PARENT iif pdeath_sig is present, v3

[Andrew Vagin]

On Thu, Aug 14, 2014 at 09:40:17PM +0400, Cyrill Gorcunov wrote:
> On Thu, Aug 14, 2014 at 09:31:21PM +0400, Andrew Vagin wrote:
> > >  static inline int fork_with_pid(struct pstree_item *item)
> > >  {
> > >  	int ret = -1, fd;
> > > @@ -951,6 +985,8 @@ static inline int fork_with_pid(struct pstree_item *item)
> > >  			pr_err("Unknown task state %d\n", item->state);
> > >  			return -1;
> > >  		}
> > > +
> > > +		maybe_clone_parent(item, &ca);
> > 
> > I think we need to execute this function only for the root task, doesn't
> > we?
> 
> yes, and we do. I move condition one level up for clarity, attached.

> From ccfa9ed9694c311a9b10981ac050bdf36041fcef Mon Sep 17 00:00:00 2001
> From: Cyrill Gorcunov <gorcunov at openvz.org>
> Date: Thu, 14 Aug 2014 19:34:30 +0400
> Subject: [PATCH] restore: Set CLONE_PARENT iif pdeath_sig is present, v3
> 
> It's been discovered that on 3.11 we might fail on restore
> if pass @CLONE_PARENT flag into clone() call due to kernel
> limitations.
> 
> Because we're treating 3.11 as a base working kernel lets
> do a trick instead
> 
>  - setup this flag iif pdeath_sig is present
>  - if CLONE_NEWPID is passed warn a user about
>    potential consequences.
> 
> v2 (by xemul@):
> 
>  - because we need to carry the condition in attach_to_tasks
>    call, introduce @root_as_sibling variable for this.
> 
> v3 (by avagin@):
> 
>  - move condition out of helper
> 
> CC: Tycho Andersen <tycho.andersen at canonical.com>
> CC: Andrey Vagin <avagin at openvz.org>
> CC: Pavel Emelyanov <xemul at parallels.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> ---
>  cr-restore.c | 49 +++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/cr-restore.c b/cr-restore.c
> index bd16b1d81ca7..1e9aa258d00b 100644
> --- a/cr-restore.c
> +++ b/cr-restore.c
> @@ -93,6 +93,8 @@ static int prepare_rlimits(int pid, CoreEntry *core);
>  static int prepare_posix_timers(int pid, CoreEntry *core);
>  static int prepare_signals(int pid);
>  
> +static int root_as_sibling;
> +
>  static int shmem_remap(void *old_addr, void *new_addr, unsigned long size)
>  {
>  	void *ret;
> @@ -922,6 +924,35 @@ struct cr_clone_arg {
>  	CoreEntry *core;
>  };
>  
> +static void maybe_clone_parent(struct pstree_item *item,
> +			      struct cr_clone_arg *ca)
> +{
> +	if (opts.swrk_restore || opts.restore_detach) {
> +		/*
> +		 * This means we're called from lib's criu_restore_child().
> +		 * In that case create the root task as the child one to+
> +		 * the caller. This is the only way to correctly restore the
> +		 * pdeath_sig of the root task. But also looks nice.
> +		 *
> +		 * Alternatively, if we are --restore-detached, a similar trick is
> +		 * needed to correctly restore pdeath_sig and prevent processes from
> +		 * dying once restored.
> +		 *
> +		 * There were a problem in kernel 3.11 -- CLONE_PARENT can't be
> +		 * set together with CLONE_NEWPID, which has been solved in further
> +		 * versions of the kernels, but we treat 3.11 as a base, so at
> +		 * least warn a user about potential problems.
> +		 */
> +		if (ca->core->thread_core->pdeath_sig) {
> +			item->rst->clone_flags |= CLONE_PARENT;
> +			root_as_sibling = 1;
> +			if (item->rst->clone_flags & CLONE_NEWPID)
> +				pr_warn("Set CLONE_PARENT | CLONE_NEWPID but it might cause restore problem,"
> +					"because not all kernels support such clone flags combinations!\n");
> +		}
> +	}
> +}
> +
>  static inline int fork_with_pid(struct pstree_item *item)
>  {
>  	int ret = -1, fd;
> @@ -951,6 +982,9 @@ static inline int fork_with_pid(struct pstree_item *item)
>  			pr_err("Unknown task state %d\n", item->state);
>  			return -1;
>  		}
> +
> +		if (unlikely(item->pid.virt != INIT_PID))

== ?

> +			maybe_clone_parent(item, &ca);
>  	} else {
>  		/*
>  		 * Helper entry will not get moved around and thus
> @@ -1587,19 +1621,6 @@ static int restore_root_task(struct pstree_item *init)
>  	futex_set(&task_entries->nr_in_progress,
>  			stage_participants(CR_STATE_RESTORE_NS));
>  
> -	/*
> -	 * This means we're called from lib's criu_restore_child().
> -	 * In that case create the root task as the child one to+
> -	 * the caller. This is the only way to correctly restore the
> -	 * pdeath_sig of the root task. But also looks nice.
> -	 *
> -	 * Alternatively, if we are --restore-detached, a similar trick is
> -	 * needed to correctly restore pdeath_sig and prevent processes from
> -	 * dying once restored.
> -	 */
> -	if (opts.swrk_restore || opts.restore_detach)
> -		init->rst->clone_flags |= CLONE_PARENT;
> -
>  	ret = fork_with_pid(init);
>  	if (ret < 0)
>  		return -1;
> @@ -1663,7 +1684,7 @@ static int restore_root_task(struct pstree_item *init)
>  
>  	timing_stop(TIME_RESTORE);
>  
> -	ret = attach_to_tasks(opts.swrk_restore);
> +	ret = attach_to_tasks(root_as_sibling);
>  
>  	pr_info("Restore finished successfully. Resuming tasks.\n");
>  	futex_set_and_wake(&task_entries->start, CR_STATE_COMPLETE);
> -- 
> 1.9.3
>