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

[Dmitry Safonov]

2017-03-27 19:19 GMT+03:00 Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>:
>
>
> 27.03.2017 18:10, Dmitry Safonov пишет:
>
>> 2017-03-27 18:47 GMT+03:00 Stanislav Kinsburskiy
>> <skinsbursky at virtuozzo.com>:
>>>
>>>
>>> 27.03.2017 17:13, Dmitry Safonov пишет:
>>>
>>>> 2017-03-27 17:25 GMT+03:00 Stanislav Kinsburskiy
>>>> <skinsbursky at virtuozzo.com>:
>>>>>
>>>>>
>>>>> 27.03.2017 16:07, Dmitry Safonov пишет:
>>>>>>
>>>>>> 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);
>>>>>
>>>> Well, I would vote for something like:
>>>>
>>>> compel_remote_mmap(struct parasite_ctl *ctl, unsigned long addr)
>>>> /* actually renamed compel_unmap() */
>>>> compel_remote_unmap(struct parasite_ctl *ctl, unsigned long addr)
>>>> compel_sigframe_prepare(struct rt_sigframe *raddr,
>>>>      user_regs_struct_t *regs, k_rtsigset_t* sig)
>>>> compel_sigframe_sigreturn(struct rt_sigframe *raddr)
>>>>
>>>> Maybe the naming could be improved.
>>>> This way we would have complete API for preparing sigframes for any
>>>> register set, blocked signals mask and for sigreturning to constructed
>>>> frames. All without parasite's help.
>>>>
>>>> Otherwise the original patch exports API for preparing sigframe only
>>>> at the point where an application was stopped and leaves reader's
>>>> exercise what to do with this sigframe afterward.
>>>>
>>>> What do you think, is it convenient API for your project?
>>>> I don't insist, but that looks logical to me avoiding inchoate APIs.
>>>>
>>> Well, you suggest to introduce 4 helpers. And add them to API.
>>> I suspect (almost sure) the following will happen:
>>> 1) Someone (or "The one", if you wish) will come and say, that names are
>>> bad
>>> and unacceptable.
>>
>> They can be adjusted, but those two: remote_mmap() and remote_unmap()
>> I quite like because they say where they perform and what.
>>
>>> 2) Then he will come once more and say, that "compel_remote_mmap/unmap"
>>> are
>>> stupid wrappers around compel_syscall(), which is ugly and unacceptable.
>>
>> That's not only wrappers around compel_syscall() - they can do more,
>> e.g., providing safety for unmap() syscall by choosing another rx vma
>> to execute syscall/int80 instruction instead the one being unmapped.
>> (or using service vma). Which is all not needed for other syscalls
>> non-mmap-like.
>> (this is aside from syscall-in-vdso idea which I'll realize soon)
>>
>
> "Another rx vma" has to be used for any syscall, because previous syscall
> can be "unmap".

Well, we know unmap() addr and size, so can easily follow with updates
to rx vmas list for a process.
The only thing - be sure that at least one rx vma left to perform syscalls.
(it may be service)

>
>>> And even if you somehow push this stuff, it won't be really valuable.
>>> Because there is no "fool protection" and user can easily shoot in his
>>> foot
>>> by passing any wrong address to compel_sigframe_prepare() as raddr,
>>> which will crash the tracee upon compel_sigframe_sigreturn() call.
>>
>> Of cause the user can. That's why I would like to see a test/example
>> how to use new API.
>
>
> You would like to see an example, how to shoot in the foot?
> Providing such interface doesn't make much sense from my POW, if it's not
> "foolproof".
>
>>> And one more thing to clarify, what you actually suggested:
>>> compel_sigframe_sigreturn() (or at least part of its logic) has to be
>>> executed in tracee.
>>> I see two ways to do this:
>>> 1) Link tracee with libcompel to take rt_sigreturn helper from there.
>>> "Link"
>>> here means load the library into tracee.
>>> 2) Be able to place some "rt_sigreturn" code to an arbitrary address
>>> within
>>> the process, and then jump to it.
>>
>> I may be wrong, but IIRC, we can omit code within the process.
>> Having raddr with formatted sigframe - we put it into %sp, use
>> compel_syscall() and call compel_remote_unmap() for vma with raddr.
>
>
> I think you miss the point of all this stuff. Call to sigreturn makes sense
> only from within the process, when it's running (not stopped).
> If you can use compel_syscall() - it's stopped and you don't need sigreturn.
> It's already implied upon syscall completion.
> But if you need to call this from within the process, you need the code.

Well, no - I was just thinking how to test the resulting sigframe from
a simple test case without inserting new code into tracee.
But I think, for this purpose, we don't need compel_sigframe_sigreturn()
wrapper and can do compel_syscall() by hands.

-- 
             Dmitry