[v2,0/3] vfio: use __aligned_u64 for ioctl structs

Message

Stefan Hajnoczi Aug. 29, 2023, 6:27 p.m. UTC

v2:
- Rebased onto https://github.com/awilliam/linux-vfio.git next to get the
  vfio_iommu_type1_info pad field [Kevin]
- Fixed min(minsz, sizeof(dmabuf)) -> min(dmabuf.argsz, sizeof(dmabuf)) [Jason, Kevin]
- Squashed Patch 3 (vfio_iommu_type1_info) into Patch 1 since it is trivial now
  that the padding field is already there.

Jason Gunthorpe <[e-mail]> pointed out that u64 VFIO ioctl struct fields
have architecture-dependent alignment. iommufd already uses __aligned_u64 to
avoid this problem.

See the __aligned_u64 typedef in <uapi/linux/types.h> for details on why it is
a good idea for kernel<->user interfaces.

This series modifies the VFIO ioctl structs to use __aligned_u64. Some of the
changes preserve the existing memory layout on all architectures, so I put them
together into the first patch. The remaining patches are for structs where
explanation is necessary about why changing the memory layout does not break
the uapi.

Stefan Hajnoczi (3):
  vfio: trivially use __aligned_u64 for ioctl structs
  vfio: use __aligned_u64 in struct vfio_device_gfx_plane_info
  vfio: use __aligned_u64 in struct vfio_device_ioeventfd

 include/uapi/linux/vfio.h        | 26 ++++++++++++++------------
 drivers/gpu/drm/i915/gvt/kvmgt.c |  4 +++-
 samples/vfio-mdev/mbochs.c       |  6 ++++--
 samples/vfio-mdev/mdpy.c         |  4 +++-
 4 files changed, 24 insertions(+), 16 deletions(-)

Comments

David Laight Aug. 29, 2023, 9:10 p.m. UTC | #1

From: Stefan Hajnoczi
> Sent: 29 August 2023 19:27
> 
> v2:
> - Rebased onto https://github.com/awilliam/linux-vfio.git next to get the
>   vfio_iommu_type1_info pad field [Kevin]
> - Fixed min(minsz, sizeof(dmabuf)) -> min(dmabuf.argsz, sizeof(dmabuf)) [Jason, Kevin]

You managed to use min_t() instead of fixing the types to match.

> - Squashed Patch 3 (vfio_iommu_type1_info) into Patch 1 since it is trivial now
>   that the padding field is already there.
> 
> Jason Gunthorpe <[e-mail]> pointed out that u64 VFIO ioctl struct fields
> have architecture-dependent alignment. iommufd already uses __aligned_u64 to
> avoid this problem.
> 
> See the __aligned_u64 typedef in <uapi/linux/types.h> for details on why it is
> a good idea for kernel<->user interfaces.
> 
> This series modifies the VFIO ioctl structs to use __aligned_u64. Some of the
> changes preserve the existing memory layout on all architectures, so I put them
> together into the first patch. The remaining patches are for structs where
> explanation is necessary about why changing the memory layout does not break
> the uapi.

But you are extending a field in the middle of the uapi structure.
This completely breaks any applications.

You could add code to detect the length of the user-provided
structure and use the correct kernel structure that matches
the length of the user-provided one.
That needs the opposite of __aligned_u64 - a 64bit integer with
32bit alignment on x64-64.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

David Laight Aug. 30, 2023, 8:32 a.m. UTC | #2

> -----Original Message-----
> From: David Laight <[e-mail]>
> Sent: 29 August 2023 22:10
> To: 'Stefan Hajnoczi' <[e-mail]>; [e-mail]
> Cc: [e-mail]; Tian, Kevin <[e-mail]>; Alex Williamson
> <[e-mail]>; Jason Gunthorpe <[e-mail]>
> Subject: RE: [PATCH v2 0/3] vfio: use __aligned_u64 for ioctl structs
> 
> From: Stefan Hajnoczi
> > Sent: 29 August 2023 19:27
> >
> > v2:
> > - Rebased onto https://github.com/awilliam/linux-vfio.git next to get the
> >   vfio_iommu_type1_info pad field [Kevin]
> > - Fixed min(minsz, sizeof(dmabuf)) -> min(dmabuf.argsz, sizeof(dmabuf)) [Jason, Kevin]
> 
> You managed to use min_t() instead of fixing the types to match.
> 
> > - Squashed Patch 3 (vfio_iommu_type1_info) into Patch 1 since it is trivial now
> >   that the padding field is already there.
> >
> > Jason Gunthorpe <[e-mail]> pointed out that u64 VFIO ioctl struct fields
> > have architecture-dependent alignment. iommufd already uses __aligned_u64 to
> > avoid this problem.
> >
> > See the __aligned_u64 typedef in <uapi/linux/types.h> for details on why it is
> > a good idea for kernel<->user interfaces.
> >
> > This series modifies the VFIO ioctl structs to use __aligned_u64. Some of the
> > changes preserve the existing memory layout on all architectures, so I put them
> > together into the first patch. The remaining patches are for structs where
> > explanation is necessary about why changing the memory layout does not break
> > the uapi.
> 
> But you are extending a field in the middle of the uapi structure.
> This completely breaks any applications.

I've had a closer look this morning.
Your explanations aren't very good.
The structures all have the 64bit fields on their natural boundary
so the memory layout isn't really changed - just extra padding
at the end.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Stefan Hajnoczi Aug. 30, 2023, 9:53 p.m. UTC | #3

On Tue, Aug 29, 2023 at 09:10:06PM +0000, David Laight wrote:
> From: Stefan Hajnoczi
> > Sent: 29 August 2023 19:27
> > 
> > v2:
> > - Rebased onto https://github.com/awilliam/linux-vfio.git next to get the
> >   vfio_iommu_type1_info pad field [Kevin]
> > - Fixed min(minsz, sizeof(dmabuf)) -> min(dmabuf.argsz, sizeof(dmabuf)) [Jason, Kevin]
> 
> You managed to use min_t() instead of fixing the types to match.

I'm not sure what you're referring to here. Can you explain which line
of code and what you'd like to see changed?

Stefan