[CRIU] [PATCH] compel: compel_emergency_sigframe() helper introduced

[Stanislav Kinsburskiy]

27.03.2017 16:07, Dmitry Safonov пишет:
> 2017-03-27 16:26 GMT+03:00 Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>:
>>
>> 27.03.2017 15:15, Dmitry Safonov пишет:
>>
>>> 2017-03-27 15:57 GMT+03:00 Stanislav Kinsburskiy
>>> <skinsbursky at virtuozzo.com>:
>>>>
>>>> 24.03.2017 11:14, Dmitry Safonov пишет:
>>>>
>>>>> 2017-03-24 12:32 GMT+03:00 Stanislav Kinsburskiy
>>>>> <skinsbursky at virtuozzo.com>:
>>>>>>
>>>>>> 24.03.2017 00:13, Andrei Vagin пишет:
>>>>>>
>>>>>>> On Fri, Mar 17, 2017 at 01:17:58PM +0300, Stanislav Kinsburskiy wrote:
>>>>>>>> This helper constructs sigframe, which can be used to resume process
>>>>>>>> from
>>>>>>>> the
>>>>>>>> point where it was stopped.
>>>>>>>> Result has to be copied to the process manually and then used with
>>>>>>>> rt_sigreturn.
>>>>>>>>
>>>>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>
>>>>> [...]
>>>>>>>> --- a/compel/src/lib/infect.c
>>>>>>>> +++ b/compel/src/lib/infect.c
>>>>>>>> @@ -1162,6 +1162,20 @@ static int make_sigframe_plain(void *from,
>>>>>>>> struct
>>>>>>>> rt_sigframe *f, struct rt_sigf
>>>>>>>>            return 0;
>>>>>>>>      }
>>>>>>>>      +int compel_emergency_sigframe(struct parasite_ctl *ctl, struct
>>>>>>>> rt_sigframe *f,
>>>>>>>> +                             struct rt_sigframe *rtf)
>>>>>>>> +{
>>>>>>>> +       pid_t pid = ctl->rpid;
>>>>>>>> +       struct thread_ctx *thread = &ctl->orig;
>>>>>>>> +       struct plain_regs_struct regs;
>>>>>>>> +
>>>>>>>> +       if (get_task_regs(pid, &thread->regs, save_regs_plain,
>>>>>>>> &regs))
>>>>>>>> {
>>>>>>>> +               pr_err("Can't obtain regs for thread %d\n", pid);
>>>>>>>> +               return -1;
>>>>>>>> +       }
>>>>>>>> +       return make_sigframe_plain(&regs, f, rtf, &thread->sigmask);
>>>>>>> make_sigframe_plain is called from parasite_start_daemon(), why it
>>>>>>> doesn't work for you. Could you describe your usecase for these
>>>>>>> changes?
>>>>>>>
>>>>>> Because I don't have parasite plugin, on which parasite_start_daemon
>>>>>> relies.
>>>>>> But it makes sense to use this new helper in parasite_start_daemon to
>>>>>> make
>>>>>> sure it's tested.
>>>>> Maybe it worth not providing additional interfaces, but splitting the
>>>>> existing.
>>>>> If we can make non-parasite version of compel_infect() and reuse its
>>>>> code
>>>>> in parasite's one, so it'll suit criu's needs and needs of projects
>>>>> without parasite.
>>>>> Does it make sense?
>>>>
>>>> Definitely.
>>>>
>>>>> Also, this looks incomplete without curing part. How do you use
>>>>> prepared sigframe
>>>>> afterward? Calling rt_sigreturn() with the help of compel_syscall() or
>>>>> somehow alike?
>>>>
>>>> Yes. Somehow alike.
>>>>
>>>>> I guess, there should be also a patch to complement API.
>>>>> And (by @avagin's rules) - adding new API/features means adding tests
>>>>> for
>>>>> it,
>>>>> looks like it wouldn't take much time to write a test for this.
>>>>
>>>> If this peace of code will be used by CRIU itself, then it looks like
>>>> sufficient testing. No?
>>>>
>>> Ok for me, but the test also plays as an example how-to-use such API.
>>> I don't insist, anyway.
>>>
>> Calling sigreturn makes sense when called from within of the process
>> context.
>> BTW, it's done already if libcompel plugins in case of fatal error on CRIU
>> side.
>> Thus, calling this from plugin explicitly doesn't make mush sense.
>> And I don't see other ways to show it's usability, supported by libcompel.
> Yes, but fini_sigreturn() at this moment is called only for parasite-based
> infection. While the patch exports API for non-parasite case of infection.
> So, as I've asked before, can we complement it with usage of the formed
> sigframe in the case of non-parasite infection?
> It may be just a helper that has inside compel_syscall() with rt_sigreturn.
> I mean, if anyone is going to use API for forming sigframe, he will
> have to write such helper. Maybe it's better to provide one for integrity?
>

It's not that simple.
User has to provide address *within* process address space, where 
sigframe must be placed, *to* sigframe creation routine.
IOW, one need:
1) Some service mapping within process (doesn't exist in non-parasite 
case of infection), which he can use to store sigframe
2) Some way to call sigreturn from *within* the process (doesn't exist 
in non-parasite case of infection)
3) Calling to "rt_sigreturn" needs this sigframe address.

Thus, from my POW, libcompel can to provide this helper "as is".
Or this facility has to be significantly improved by hiding all this 
remote mapping creation, sigrame construction and rt_sigreturn call to 
some kind of helpers like below:

int compel_put_some_straw(struct parasite_ctl *ctl);

and paired (static, inlined, whatever else to prevent linking with 
libcompel):

int compel_fall_from_the tree(void);