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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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
XEN_GUEST_HANDLE with XEN_GUEST_HANDLE_PARAM when
appropriate"), without any user. I think it should simply be
removed.

> 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.

Jan


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