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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.06.2020 17:56, Roger Pau Monné wrote:
> On Tue, Jun 23, 2020 at 05:02:04PM +0200, Jan Beulich wrote:
>> 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.
> 
> Yes, there's already compat translation for the frame_list array.
> 
>> 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.
> 
> Right, this is the layout with frame aligned to 8 bytes:
> 
> struct xen_mem_acquire_resource {
>       uint16_t                   domid;                /*     0     2 */
>       uint16_t                   type;                 /*     2     2 */
>       uint32_t                   id;                   /*     4     4 */
>       uint32_t                   nr_frames;            /*     8     4 */
>       uint32_t                   pad;                  /*    12     4 */
>       uint64_t                   frame;                /*    16     8 */
>       long unsigned int *        frame_list;           /*    24     4 */
> 
>       /* size: 32, cachelines: 1, members: 7 */
>       /* padding: 4 */
>       /* last cacheline: 32 bytes */
> };
> 
> And this is without:
> 
> struct xen_mem_acquire_resource {
>       uint16_t                   domid;                /*     0     2 */
>       uint16_t                   type;                 /*     2     2 */
>       uint32_t                   id;                   /*     4     4 */
>       uint32_t                   nr_frames;            /*     8     4 */
>       uint32_t                   pad;                  /*    12     4 */
>       uint64_t                   frame;                /*    16     8 */
>       long unsigned int *        frame_list;           /*    24     4 */
> 
>       /* size: 28, cachelines: 1, members: 7 */
>       /* last cacheline: 28 bytes */
> };
> 
> Note Xen has already been copying those extra 4 padding bytes from
> 32bit Linux kernels AFAICT, as the struct declaration in Linux has
> dropped the aligned attribute.
> 
>> 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.
> 
> I guess we are trapped with what Linux has on it's headers now, so we
> should handle the struct size difference in Xen?

How do you mean to notice the difference in Xen? You don't know
what the caller's environment's sizeof() would yield.

> I'm very tempted to just say switch the XEN_GUEST_HANDLE to
> XEN_GUEST_HANDLE_64, but I guess it's risky.

Plus, like uint64_aligned_t, iirc it's a construct exposed through
tool-stack-only interfaces, but not generally.

>>> --- 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.
> 
> Hm, I'm not so sure. This is indeed used by Xen related tools (or
> emulators) and then those domains running such tools would also be
> tools domains?
> 
> Anyway, I don't mind removing it, but I also don't think it's so
> off.

Well - if this isn't a tool stack interface (see also my 2nd reply
to your original patch), then the comment shouldn't suggest it is.

Jan