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

Mon Mar 27 10:36:25 PDT 2017

27 марта 2017 г. 7:26 PM пользователь Dmitry Safonov <0x7f454c46 at gmail.com> написал:
2017-03-27 20:03 GMT+03:00 Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>:
>
>
> 27.03.2017 18:57, 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)
>>
>
> Well, it's funny. The only reason why compel needs this list, AFAIU, is to
> find the mapping to load code.
> It so, and if you have the service mapping, why do you need the list at all?

To create service mapping you'll still need this list.
So, we can omit creating service mapping untill compel_remote_unmap()
call which will remove the last rx mapping and only then create service one.
This way we will not need to create/destroy service mapping for the most
cases, except the one where the last rx is destroyed by compel user.
I think that's quite unlikely case, so we would skip the cost of creating
and destroying it usally.

Well, you don't need the list. You need one executable mapping to be found to place only one system call in the early beginning. Moreover, one service mapping is always created, when plugin is used. It can be reused as a place for syscall.
Maintaining the list instead of paying for map/unmap call...
You will save a bit, but I'm afraid that this is not the best place, where performance has to be improved. And you will raise code complexity, introducing more bugs. As for me, CRIU is full of complexity and bugs already.
But I don't insist of course. Up to you.

>>>>> 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.
>>
>
> Well, not really. Compel_syscall() expects, that process will trap itself,
> while it won't in case of sigreturn.
> You can place "int3" on process return point, of course.
> But yet again, I don't see a valuable point in all these helpers.

Hmm, well, maybe adding a meaningful test for this API is more complicated
than I've initially thought. So, if we test this code by reusing it in CRIU,
I guess it would be enough.

--
             Dmitry

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20170327/708fd4a7/attachment-0001.html>