[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] x86/dmop: Fix compat_dm_op() ABI

>>> On 31.01.17 at 20:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> The parameter to compat_dm_op() is a pointer to an array of
> compat_dm_op_buf_t's in guest RAM.
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one question (see below).

> What is the pupose of COMPAT_HANDLE_PARAM()? It is a packed structure of one
> and a half pointers, so isn't safe at all for use in the hypercall function
> APIs (depsite its naming making it look deceptively like it is the correct
> thing to use).

Stefano, you've added this in e7a527e100 ("xen: replace
appropriate"), without any user. I think it should simply be

> On a more serious note, why do we have all this macro infrastrucutre in the
> first place?  Having spent rather longer debugging this than I to admit
> (almost mainly from the userspace side) I have concluded that it is actively
> dangerous to use; all it does is hide what is going on.
> What does it actually give us that the Linux route of a real C pointers and a
> __user attribute doesn't, other than obfuscating the code on the hypercall
> boundary?

I think you refer to the whole handle machinery here, the main goal
of which is to avoid anyone mistakenly directly de-referencing a
pointer coming from a guest. Linux'es __user attribute doesn't
achieve this during the normal compilation stage, afaik.

> @@ -525,7 +525,7 @@ CHECK_dm_op_inject_msi;
>  int compat_dm_op(domid_t domid,
>                   unsigned int nr_bufs,
> -                 COMPAT_HANDLE_PARAM(compat_dm_op_buf_t) bufs)
> +                 XEN_GUEST_HANDLE_PARAM(void) bufs)

Why the change to void? I'd prefer if we kept correct types,
even if that's just for documentation purposes.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.