From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 4/4] xen: Add V4V implementation Date: Thu, 04 Oct 2012 13:11:10 +0100 Message-ID: <506D98FE020000780009FA23@nat28.tlf.novell.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jean Guyader Cc: tim@xen.org, Jean Guyader , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> 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. Jan