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

[Julien Grall <julien@xen.org>]

Hi Jan,

On 24/06/2020 11:12, Jan Beulich wrote:
> On 23.06.2020 19:26, Roger Pau Monné wrote:
>> On Tue, Jun 23, 2020 at 06:18:52PM +0200, Jan Beulich wrote:
>>> 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 confused. Couldn't we switch from uint64_aligned_t to plain
>> uint64_t (like it's currently on the Linux headers), and then use the
>> compat layer in Xen to handle the size difference when called from
>> 32bit environments?
> 
> And which size would we use there? The old or the new one (breaking
> future or existing callers respectively)? Meanwhile I think that if
> this indeed needs to not be tools-only (which I still question),

I think we now agreed on a subthread that the kernel needs to know the 
layout of the hypercall.

> then our only possible route is to add explicit padding for the
> 32-bit case alongside the change you're already making.

AFAICT Linux 32-bit doesn't have this padding. So wouldn't it make 
incompatible the two incompatible?

Cheers,

-- 
Julien Grall