Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.06.2020 15:52, Roger Pau Monne wrote:
> XENMEM_acquire_resource and it's related structure is currently inside
> a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the
> hypervisor or the toolstack only. This is wrong as the hypercall is
> already being used by the Linux kernel at least, and as such needs to
> be public.
> 
> Also switch the usage of uint64_aligned_t to plain uint64_t, as
> uint64_aligned_t is only to be used by the toolstack. Note that the
> layout of the structure will be the same, as the field is already
> naturally aligned to a 8 byte boundary.

I'm afraid it's more complicated, and hence ...

> No functional change expected.

... this doesn't hold: As I notice only now the struct also wrongly
(if it was meant to be a tools-only interface) failed to use
XEN_GUEST_HANDLE_64(), which would in principle have been a problem
for 32-bit callers (passing garbage in the upper half of a handle).
It's not an actual problem because, unlike would typically be the
case for tools-only interfaces, there is compat translation for this
struct.

With this, however, the problem of your change becomes noticeable:
The structure's alignment for 32-bit x86, and with it the structure's
sizeof(), both change. You should be able to see this by comparing
xen/common/compat/memory.c's disassembly before and after your
change - the number of bytes to copy by the respective
copy_from_guest() ought to have changed.

The question now is whether we're willing to accept such a (marginal)
incompatibility. The chance of a 32-bit guest actually running into
it shouldn't be very high, but with the right placement of an
instance of the struct at the end of a page it would be possible to
observe afaict.

Or whether we go the alternative route and pad the struct suitably
for 32-bit.

> Fixes: 3f8f12281dd20 ('x86/mm: add HYPERVISOR_memory_op to acquire guest 
> resources')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Would be good to get this fixed before the release in order to avoid
> shipping bogus headers. Should also be backported.

This was already part of 4.13, as you imply by requesting a backport.
Hence the bogus header had already been shipped.

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -607,6 +607,8 @@ struct xen_reserved_device_memory_map {
>  typedef struct xen_reserved_device_memory_map 
> xen_reserved_device_memory_map_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
>  
> +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> +
>  /*
>   * Get the pages for a particular guest resource, so that they can be
>   * mapped directly by a tools domain.

This comment is now stale.

Jan