From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean Guyader Subject: Re: [PATCH 4/4] xen: Add V4V implementation Date: Thu, 4 Oct 2012 13:41:50 +0100 Message-ID: References: <1348141322-28657-1-git-send-email-jean.guyader@citrix.com> <1348141322-28657-5-git-send-email-jean.guyader@citrix.com> <505B2636020000780009CAA6@nat28.tlf.novell.com> <506D98FE020000780009FA23@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <506D98FE020000780009FA23@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: tim@xen.org, Jean Guyader , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 4 October 2012 13:11, Jan Beulich wrote: >>>> On 04.10.12 at 14:03, Jean Guyader wrote: >> On 20 September 2012 13:20, Jan Beulich wrote: >>>>>> On 20.09.12 at 13:42, Jean Guyader 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