From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 4/4] xen: Add V4V implementation Date: Thu, 20 Sep 2012 13:20:38 +0100 Message-ID: <505B2636020000780009CAA6@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1348141322-28657-5-git-send-email-jean.guyader@citrix.com> 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, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> 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. >+ 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). >+struct v4v_ring_message_header >+{ >+ uint32_t len; >+ uint32_t pad0; Why? v4v_addr_t is 4-byte aligned. >+ v4v_addr_t source; >+ uint32_t message_type; >+ uint32_t pad1; And again, why? >+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L >+ uint8_t data[]; >+#elif defined(__GNUC__) >+ uint8_t data[0]; >+#endif >+}; ... >+/* >+ * V4VOP_register_ring >+ * >+ * Registers a ring with Xen. If a ring with the same v4v_ring_id exists, >+ * the hypercall will return -EEXIST. >+ * >+ * do_v4v_op(V4VOP_unregister_ring, >+ * XEN_GUEST_HANDLE(v4v_ring_t), XEN_GUEST_HANDLE(xen_pfn_t), >+ * npage, 0) >+ */ >+#define V4VOP_register_ring 1 "register" or "unregister"? Also, note the use of v4v_ring_t here, but ... >+/* >+ * V4VOP_unregister_ring >+ * >+ * Unregister a ring. >+ * >+ * do_v4v_op(V4VOP_unregister_ring, >+ * XEN_GUEST_HANDLE(v4v_ring), >+ * NULL, 0, 0) >+ */ >+#define V4VOP_unregister_ring 2 ... not here. Please be consistent, even if this is just comments. (Again at least for V4VOP_sendv.) >+/* >+ * V4VOP_viptables_list >+ * >+ * Delete a filtering rules at a position or the rule Delete? Also, here and above, make clear whether you talk about a single or multiple rules. >+ * that matches "rule". >+ * >+ * do_v4v_op(V4VOP_viptables_list, >+ * XEN_GUEST_HANDLE(v4v_vitpables_list_t) list, >+ * NULL, 0, 0) >+ */ >+#define V4VOP_viptables_list 8 Also, with all of the padding fields and unused arguments (for certain sub-ops) - if you see the slightest chance of them ever getting used for some extension, you will want to make sure they got zero-filled by the guest. Jan