Re: [PATCH 4/4] xen: Add V4V implementation

[Jean Guyader <jean.guyader@gmail.com>]

On 4 October 2012 13:11, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 04.10.12 at 14:03, Jean Guyader <jean.guyader@gmail.com> wrote:
>> On 20 September 2012 13:20, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@citrix.com> wrote:
>>>>+        case V4VOP_register_ring:
>>>>+            {
>>>>+                XEN_GUEST_HANDLE(v4v_ring_t) ring_hnd =
>>>>+                    guest_handle_cast(arg1, v4v_ring_t);
>>>>+                XEN_GUEST_HANDLE(xen_pfn_t) pfn_hnd =
>>>>+                    guest_handle_cast(arg2, xen_pfn_t);
>>>>+                uint32_t npage = arg3;
>>>>+                if ( unlikely(!guest_handle_okay(ring_hnd, 1)) )
>>>>+                    goto out;
>>>>+                if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) )
>>>>+                    goto out;
>>>
>>> Here and below - this isn't sufficient for compat guests, or else
>>> you give them a way to point into the compat m2p table.
>>>
>>
>> I'll probably switch to uint64_t for the v4v mfn list instead of using
>> xen_pfn_t which
>> are unsigned long. That way I can save the need for a compat wrapper.
>
> But that comment of yours doesn't address the problem I pointed
> out.
>
>>>>+                if ( unlikely(!guest_handle_okay(addr_hnd, 1)) )
>>>>+                    goto out;
>>>>+                if ( copy_from_guest(&addr, addr_hnd, 1) )
>>>>+                    goto out;
>>>
>>> The former is redundant with the latter. Also, if you already
>>> used guest_handle_okay() on an array, you then can (and
>>> should) use the cheaper __copy_{to,from}_guest() functions.
>>> While looking at this, I also spotted at least on guest access
>>> without error checking. Plus many guest accesses happen
>>> with some sort of lock held - that'll cause problems when the
>>> guest memory you're trying to access was paged out (quite
>>> likely you would be able to point out other such cases, but
>>> those likely predate paging, and hence are an oversight of
>>> whoever introduced the paging code).
>>>
>>
>> There are plenty of other place in Xen where we copy data from
>> a guest with some sort of lock held.
>>
>> I understand that the new code should do it's best to make sure
>> it works correctly with the Xen paging code.
>>
>> The best way to solve this might not be try to to avoid copying from
>> guest memory under a lock. I've discussed this with Tim and maybe
>> we could introduce a new copy_from_guest which is aware of the paging
>> and returns a specific error, and then we could cleanly unlock everything
>> and issue a continuation that will fixup the memory.
>
> That's certainly an alternative.
>

Another way (probably a easier way) would be to add a "mlock" mechanism.

Some of the pages could be lock which mean that we know before hand that
a copy_from_guest won't raise a paging error.

In my case it would be ideal. I can comment it out in the
register/unregister ring
code, so when we have such mechanism in place the v4v code could be updated.

Jean