Re: [Xen-devel] [PATCH 4/4] xen: Add V4V implementation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.10.12 at 14:03, Jean Guyader <jean.guyader@xxxxxxxxx> wrote:
> On 20 September 2012 13:20, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 20.09.12 at 13:42, Jean Guyader <jean.guyader@xxxxxxxxxx> 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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel