On Wed, 15 Jul 2020, 14:42 Jan Beulich, <jbeulich@suse.com> wrote:

> On 15.07.2020 12:46, Julien Grall wrote:
> > On Wed, 15 Jul 2020, 12:17 Jan Beulich, <jbeulich@suse.com> 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@suse.com>
> >> ---
> >> 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.



> 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.



> Jan
>