Re: [PATCH 2/2] evtchn/fifo: don't enforce higher than necessary alignment

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.07.2020 16:02, Julien Grall wrote:
> On Wed, 15 Jul 2020, 14:42 Jan Beulich, <jbeulich@xxxxxxxx> wrote:
> 
>> On 15.07.2020 12:46, Julien Grall wrote:
>>> On Wed, 15 Jul 2020, 12:17 Jan Beulich, <jbeulich@xxxxxxxx> wrote:
>>>
>>>> Neither the code nor the original commit provide any justification for
>>>> the need to 8-byte align the struct in all cases. Enforce just as much
>>>> alignment as the structure actually needs - 4 bytes - by using alignof()
>>>> instead of a literal number.
>>>>
>>>> Take the opportunity and also
>>>> - add so far missing validation that native and compat mode layouts of
>>>>   the structures actually match,
>>>> - tie sizeof() expressions to the types of the fields we're actually
>>>>   after, rather than specifying the type explicitly (which in the
>>>>   general case risks a disconnect, even if there's close to zero risk in
>>>>   this particular case),
>>>> - use ENXIO instead of EINVAL for the two cases of the address not
>>>>   satisfying the requirements, which will make an issue here better
>>>>   stand out at the call site.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> ---
>>>> I question the need for the array_index_nospec() here. Or else I'd
>>>> expect map_vcpu_info() would also need the same.
>>>>
>>>> --- a/xen/common/event_fifo.c
>>>> +++ b/xen/common/event_fifo.c
>>>> @@ -504,6 +504,16 @@ static void setup_ports(struct domain *d
>>>>      }
>>>>  }
>>>>
>>>> +#ifdef CONFIG_COMPAT
>>>> +
>>>> +#include <compat/event_channel.h>
>>>> +
>>>> +#define xen_evtchn_fifo_control_block evtchn_fifo_control_block
>>>> +CHECK_evtchn_fifo_control_block;
>>>> +#undef xen_evtchn_fifo_control_block
>>>> +
>>>> +#endif
>>>> +
>>>>  int evtchn_fifo_init_control(struct evtchn_init_control *init_control)
>>>>  {
>>>>      struct domain *d = current->domain;
>>>> @@ -523,19 +533,20 @@ int evtchn_fifo_init_control(struct evtc
>>>>          return -ENOENT;
>>>>
>>>>      /* Must not cross page boundary. */
>>>> -    if ( offset > (PAGE_SIZE - sizeof(evtchn_fifo_control_block_t)) )
>>>> -        return -EINVAL;
>>>> +    if ( offset > (PAGE_SIZE - sizeof(*v->evtchn_fifo->control_block))
>> )
>>>> +        return -ENXIO;
>>>>
>>>>      /*
>>>>       * Make sure the guest controlled value offset is bounded even
>> during
>>>>       * speculative execution.
>>>>       */
>>>>      offset = array_index_nospec(offset,
>>>> -                           PAGE_SIZE -
>>>> sizeof(evtchn_fifo_control_block_t) + 1);
>>>> +                                PAGE_SIZE -
>>>> +                                sizeof(*v->evtchn_fifo->control_block)
>> +
>>>> 1);
>>>>
>>>> -    /* Must be 8-bytes aligned. */
>>>> -    if ( offset & (8 - 1) )
>>>> -        return -EINVAL;
>>>> +    /* Must be suitably aligned. */
>>>> +    if ( offset & (alignof(*v->evtchn_fifo->control_block) - 1) )
>>>> +        return -ENXIO;
>>>>
>>>
>>> A guest relying on this new alignment wouldn't work on older version of
>>> Xen. So I don't think a guest would ever be able to use it.
>>>
>>> Therefore is it really worth the change?
>>
>> That's the question. One of your arguments for using a literal number
>> also for the vCPU info mapping check was that here a literal number
>> is used. The goal isn't so much relaxation of the interface, but
>> making the code consistent as well as eliminating a (how I'd call it)
>> kludge.
>>
> 
> Your commit message lead to think the relaxation is the key motivation to
> change the code.

I've added a clarifying sentence.

>> Guests not caring to be able to run on older versions could also make
>> use of the relaxation (which may be more relevant in 10 y ears time
>> than it is now).
> 
> 
> That makes sense. However, I am a bit concerned that an OS developer may
> not notice the alignment problem with older version.
> 
> I would suggest to at least document the alignment expected in the public
> header.

Done for v2.

Jan