[CRIU] [PATCH] restore: use root_as_sibling only after defining it

[Pavel Emelyanov]

On 09/10/2014 01:13 AM, Tycho Andersen wrote:
> root_as_sibling was used in criu_signals_setup(), but was only defined later
> (when forking the root task for the first time). This meant that the
> SA_NOCLDSTOP was never masked off, which meant SIGCHLD was never delivered
> after ptracing the root task. Thus, when the a child of the root task died
> (e.g. from cr_system), the root task sat in PTRACE_STOP, and the restore task
> never PTRACE_CONT'd, resulting in a deadlock.
> 
> Instead, we only unmask SA_NOCLDSTOP right before we PTRACE_SEIZE, after the
> value is defined.
> 
> v2: re-work the condition for CLONE_PARENT
> v3: move unmasking of SA_NOCLDSTOP to restore_root_task
> v4: keep all the comments in the original code
> 
> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>

Patch applied. Thanks a lot!

> ---
>  cr-restore.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/cr-restore.c b/cr-restore.c
> index 2735d0d..dd7b837 100644
> --- a/cr-restore.c
> +++ b/cr-restore.c
> @@ -956,6 +956,12 @@ struct cr_clone_arg {
>  static void maybe_clone_parent(struct pstree_item *item,
>  			      struct cr_clone_arg *ca)
>  {
> +	/*
> +	 * zdtm runs in kernel 3.11, which has the problem described below. We
> +	 * avoid this by including the pdeath_sig test. Once users/zdtm migrate
> +	 * off of 3.11, this condition can be simplified to just test the
> +	 * options and not have the pdeath_sig test.
> +	 */
>  	if (opts.swrk_restore ||
>  	    (opts.restore_detach && ca->core->thread_core->pdeath_sig)) {
>  		/*
> @@ -1169,18 +1175,6 @@ static int criu_signals_setup(void)
>  	}
>  
>  	act.sa_flags |= SA_NOCLDSTOP | SA_SIGINFO | SA_RESTART;
> -	if (root_as_sibling)
> -		/*
> -		 * Root task will be our sibling. This means, that
> -		 * we will not notice when (if) it dies in SIGCHLD
> -		 * handler, but we should. To do this -- attach to
> -		 * the guy with ptrace (below) and (!) make the kernel
> -		 * deliver us the signal when it will get stopped.
> -		 * It will in case of e.g. segfault before handling
> -		 * the signal.
> -		 */
> -		act.sa_flags &= ~SA_NOCLDSTOP;
> -
>  	act.sa_sigaction = sigchld_handler;
>  	sigemptyset(&act.sa_mask);
>  	sigaddset(&act.sa_mask, SIGCHLD);
> @@ -1668,6 +1662,20 @@ static int restore_root_task(struct pstree_item *init)
>  		return -1;
>  
>  	if (root_as_sibling) {
> +		struct sigaction act;
> +		/*
> +		 * Root task will be our sibling. This means, that
> +		 * we will not notice when (if) it dies in SIGCHLD
> +		 * handler, but we should. To do this -- attach to
> +		 * the guy with ptrace (below) and (!) make the kernel
> +		 * deliver us the signal when it will get stopped.
> +		 * It will in case of e.g. segfault before handling
> +		 * the signal.
> +		 */
> +		sigaction(SIGCHLD, NULL, &act);
> +		act.sa_flags &= ~SA_NOCLDSTOP;
> +		sigaction(SIGCHLD, &act, NULL);
> +
>  		if (ptrace(PTRACE_SEIZE, init->pid.real, 0, 0)) {
>  			pr_perror("Can't attach to init");
>  			goto out;
>