Re: [PATCH v3 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/01/17 12:47, Jan Beulich wrote:
>>>>> The kernel already has to parse this structure anyway, and will know the
>>>>> bitness of its userspace process.  We could easily (at this point)
>>>>> require the kernel to turn it into the kernels bitness for forwarding on
>>>>> to Xen, which covers the 32bit userspace under a 64bit kernel problem,
>>>>> in a way which won't break the hypercall ABI when 128bit comes along.
>>> But that won't cover a 32-bit kernel.
>> Yes it will.
> How that, without a compat translation layer in Xen?

Why shouldn't there be a compat layer?

>
>>> And I'm not sure we really need to bother considering hypothetical
>>> 128-bit architectures at this point in time.
>> Because considering this case will avoid us painting ourselves into a
>> corner.
> Why would we consider this case here, when all other part of the
> public interface don't do so?

This is asking why we should continue to shoot ourselves in the foot,
ABI wise, rather than trying to do something better.

And the answer is that I'd prefer that we started fixing the problem,
rather than making it worse.

>>>> As discussed...
>>>>
>>>> xen_dm_op_buf is currently used directly in the tools code because there is 
>>>> no explicit support for DMOPs in privcmd. When explicit support is added then 
>>>> this can change and we can use a void ptr and size_t as you say.
>>>>
>>>> For the privcmd -> Xen interface that using XEN_GUEST_HANDLE_64 does indeed 
>>>> limit us and so using XEN_GUEST_HANDLE would indeed seem more sensible (and 
>>>> we won't need a 32-bit compat wrapper for DMOP because it's a tools-only 
>>>> hypercall).
>>> Just like with other tools only interfaces, I think this should remain a
>>> 64-bit handle, to avoid (as you say, but wrongly in conjunction with a
>>> "normal" handle) the need for a compat wrapper.
>> I disagree.  It is a layering violation. (and apologies if this appears
>> to rant, but it seems that all major development I do for Xen is
>> removing layering violations which could have been more easily avoided
>> if v1 had been designed more sensibly.)
> Whether this is a layering violation really depends on the position
> you take: If tools only interfaces are meant to be a direct channel
> from user space to hypervisor, then the kernel has no business
> interposing a translation layer, the more that such a layer would
> need updating every time a new operation got added to that
> tools only interface.

"tools-only" is an illusion.  Hypercalls are a kernel-only operation,
and are interpreted with an ABI determined by the width of the kernel.

It is for this precise reason that a 32bit toolstack under a 64bit
kernel doesn't function correctly.

Despite privcmd's implementation being a straight "throw it over the
wall to Xen", the hypercall ABI is strictly between Xen and the kernel,
not Xen and the toolstack.

>  Even worse, the "tools only" property would
> be partially lost the moment the kernel had a valid reason to look
> at those structures.

This is an ABI which, by its very design, *must* be inspected and
audited by the kernel.  dmop cannot be a "tools interface" like
domctl/sysctl/etc.

The userspace/kernel interaction is going to be in terms of void * and
size_t's, so the kernel can sensibly audit the memory ranges.

>> The 64bit guest handle infrastructure has always been a kludge.
> It has been the better alternative to a compat translation layer
> first of all.

I disagree on calling it better.

In a hypothetical world x86_128 world, how would the current scheme
function?  How would you distinguish a piece of 128bit userspace from a
64 or 32bit userspace under a 128bit kernel?

>
>> Xen and the kernel speak an ABI, which is based on the width of the
>> kernel.  These take a GUEST_HANDLE() as a wrapper around void *, to
>> indicate the size of a pointer used by the kernel.  Under this ABI, Xen
>> is at least the width of the kernel, and must translate a smaller
>>
>> The kernel and its userspace speak an ABI which is based on the width of
>> userspace, and the kernel (for things other than privcmd) translates via
>> its compat layer.
> Which is a fuzzy thing by itself already, when you take into
> consideration processes running both 64- and 32-bit code.

It is only fuzzy because we don't have a rational layer separation to
start with.

If you really don't want the kernel inspecting the internal content of
the call (which, is a property I do agree with), then the correct ABI
would have been to wrap userspace hypercalls (multicall style)
indicating the ABI with which userspace was compiled.  This would at
least allow Xen to interpret the contents in an unambiguous manner.

~Andrew

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