Re: [PATCH] pvcalls: Document explicitly the padding for all arches

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.04.2020 01:27, Stefano Stabellini wrote:
> On Mon, 20 Apr 2020, Jan Beulich wrote:
>> On 20.04.2020 15:34, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 20/04/2020 09:04, Jan Beulich wrote:
>>>> On 19.04.2020 12:49, Julien Grall wrote:
>>>>> --- a/docs/misc/pvcalls.pandoc
>>>>> +++ b/docs/misc/pvcalls.pandoc
>>>>> @@ -246,9 +246,7 @@ The format is defined as follows:
>>>>>                   uint32_t domain;
>>>>>                   uint32_t type;
>>>>>                   uint32_t protocol;
>>>>> -                #ifdef CONFIG_X86_32
>>>>>                   uint8_t pad[4];
>>>>> -                #endif
>>>>>               } socket;
>>>>>               struct xen_pvcalls_connect {
>>>>>                   uint64_t id;
>>>>> @@ -257,16 +255,12 @@ The format is defined as follows:
>>>>>                   uint32_t flags;
>>>>>                   grant_ref_t ref;
>>>>>                   uint32_t evtchn;
>>>>> -                #ifdef CONFIG_X86_32
>>>>>                   uint8_t pad[4];
>>>>> -                #endif
>>>>>               } connect;
>>>>>               struct xen_pvcalls_release {
>>>>>                   uint64_t id;
>>>>>                   uint8_t reuse;
>>>>> -                #ifdef CONFIG_X86_32
>>>>>                   uint8_t pad[7];
>>>>> -                #endif
>>>>>               } release;
>>>>>               struct xen_pvcalls_bind {
>>>>>                   uint64_t id;
>>>>> @@ -276,9 +270,7 @@ The format is defined as follows:
>>>>>               struct xen_pvcalls_listen {
>>>>>                   uint64_t id;
>>>>>                   uint32_t backlog;
>>>>> -                #ifdef CONFIG_X86_32
>>>>>                   uint8_t pad[4];
>>>>> -                #endif
>>>>>               } listen;
>>>>>               struct xen_pvcalls_accept {
>>>>>                   uint64_t id;
>>>>
>>>> I wonder on what grounds these #ifdef-s had been there - they're
>>>> plain wrong with the types used in the public header.
>>>>
>>>>> --- a/xen/include/public/io/pvcalls.h
>>>>> +++ b/xen/include/public/io/pvcalls.h
>>>>> @@ -65,6 +65,7 @@ struct xen_pvcalls_request {
>>>>>               uint32_t domain;
>>>>>               uint32_t type;
>>>>>               uint32_t protocol;
>>>>> +            uint8_t pad[4];
>>>>>           } socket;
>>>>>           struct xen_pvcalls_connect {
>>>>>               uint64_t id;
>>>>> @@ -73,10 +74,12 @@ struct xen_pvcalls_request {
>>>>>               uint32_t flags;
>>>>>               grant_ref_t ref;
>>>>>               uint32_t evtchn;
>>>>> +            uint8_t pad[4];
>>>>>           } connect;
>>>>>           struct xen_pvcalls_release {
>>>>>               uint64_t id;
>>>>>               uint8_t reuse;
>>>>> +            uint8_t pad[7];
>>>>>           } release;
>>>>>           struct xen_pvcalls_bind {
>>>>>               uint64_t id;
>>>>> @@ -86,6 +89,7 @@ struct xen_pvcalls_request {
>>>>>           struct xen_pvcalls_listen {
>>>>>               uint64_t id;
>>>>>               uint32_t backlog;
>>>>> +            uint8_t pad[4];
>>>>>           } listen;
>>>>
>>>> I'm afraid we can't change these in such a way - your additions
>>>> change sizeof() for the respective sub-structures on 32-bit x86,
>>>> and hence this is not a backwards compatible adjustment. 
>>>
>>> This is a bit confusing, each structure contain a 64-bit field so
>>> I would have thought it the structure would be 8-byte aligned (as
>>> on 32-bit Arm). But looking at the spec, a uint64_t will only
>>> aligned to 4-byte.
>>>
>>> However, I am not sure why sizeof() matters here. I understand
>>> the value would be different, but AFAICT, this is not used as part
>>> of the protocol.
>>
>> Two independent components of a consumer of our interface could
>> have a function taking (pointer to) struct xen_pvcalls_connect.
>> If one component gets re-built with the new definition and the
>> other doesn't, they'll disagree on what range of memory needs
>> to be accessible. The instantiating side (using the old header)
>> may have ended up placing the struct immediately ahead of a
>> page boundary. The consuming side (using the changed header)
>> would then encounter a fault if it wanted to access the struct
>> as a whole (assignment, memcpy()).
> 
> Even if it was possible to use the sub-structs defined in the header
> that way, keep in mind that we also wrote:
> 
>         /* dummy member to force sizeof(struct xen_pvcalls_request)
>          * to match across archs */
>         struct xen_pvcalls_dummy {
>             uint8_t dummy[56];
>         } dummy;

This has nothing to do with how a consumer may use the structs.

> And the spec also clarifies that the size of each specific request is
> always 56 bytes.

Sure, and I didn't mean to imply that a consumer would be allowed
to break this requirement. Still something like this

int pvcall_new_socket(struct xen_pvcalls_socket *s) {
    struct xen_pvcalls_request req = {
        .req_id = REQ_ID,
        .cmd = PVCALLS_SOCKET,
        .u.socket = *s,
    };

    return pvcall(&req);
}

may break.

Jan