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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

I'm very tempted to just say switch the XEN_GUEST_HANDLE to
XEN_GUEST_HANDLE_64, but I guess it's risky. I'm not sure how much
people still run a toolstack from a 32bit domain however.

> > Fixes: 3f8f12281dd20 ('x86/mm: add HYPERVISOR_memory_op to acquire guest 
> > resources')
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Would be good to get this fixed before the release in order to avoid
> > shipping bogus headers. Should also be backported.
> 
> This was already part of 4.13, as you imply by requesting a backport.
> Hence the bogus header had already been shipped.

Indeed. I would however try to prevent from shipping it in 4.14.

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

Roger.