[CRIU] [PATCH] restore: define root_as_sibling before using it

[Pavel Emelyanov]

On 09/09/2014 11:44 PM, Tycho Andersen wrote:
> On Tue, Sep 09, 2014 at 11:23:47PM +0400, Pavel Emelyanov wrote:
>> On 09/09/2014 10:15 PM, Tycho Andersen wrote:
>>> root_as_sibling is 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.
>>>
>>> We also drop the pdeath_sig constraint from setting root_as_sibling when in
>>> --restore-detached mode; in --restore-detached we /always/ need to have
>>> root_as_sibling, but we only need to clone the parent if pdeath_sig is set and
>>> we want to restore the task as alive.
>>>
>>> v2: re-work the condition for CLONE_PARENT
>>>
>>> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
>>> ---
>>>  cr-restore.c | 30 ++++++++++++++++++++----------
>>>  1 file changed, 20 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/cr-restore.c b/cr-restore.c
>>> index 2735d0d..fe6c798 100644
>>> --- a/cr-restore.c
>>> +++ b/cr-restore.c
>>> @@ -956,25 +956,21 @@ 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
>>> +	 * root_as_sibling.
>>> +	 */
>>>  	if (opts.swrk_restore ||
>>>  	    (opts.restore_detach && ca->core->thread_core->pdeath_sig)) {
>>>  		/*
>>> -		 * 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.
>>>  		 */
>>>  		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");
>>> @@ -1792,6 +1788,20 @@ int cr_restore_tasks(void)
>>>  {
>>>  	int ret = -1;
>>>  
>>> +	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.
>>> +		 */
>>> +		root_as_sibling = 1;
>>
>> The root_as_sibling and item->rst->clone_flags above should come
>> in sync, they don't work one w/o another. Thus the whole complex
>> logic about whether or not to do it should sit here and the
>> maybe_clone_parent() should just check the root_as_sibling.
> 
> But isn't it wrong to --restore-detach and not set root_as_sibling?

If we --restore-detach and don't set this variable the root task
would be born as criu's child, then criu would exit and the fresh
task would get re-parented to init. Why is it wrong from your
point of view?

BTW, the --restore-detach is the CLI use-case. We also have the
scenario when we restore tasks from standalone service, in that
case restore tasks must get re-parented to init and thus must
be born as criu child. And we have the criu_restore_child() case
-- here the task must be born as criu sibling. %)

> Another problem is that we don't actually have the core when we are
> setting up the signals, those are loaded in fork_with_pid.

Damn :( And thus we don't have any ideas whether or not we're should
use the root_as_sibling. It's already midnight in MSK, so I need some
time-out to think what we can do about it, sorry. But if you have some 
good idea -- don't wait for me to wake up :)

Thanks,
Pavel