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 <[email protected]> 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)
> -----Original Message-----
> From: David Laight <[email protected]>
> Sent: 29 August 2023 22:10
> To: 'Stefan Hajnoczi' <[email protected]>; [email protected]
> Cc: [email protected]; Tian, Kevin <[email protected]>; Alex Williamson
> <[email protected]>; Jason Gunthorpe <[email protected]>
> 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 <[email protected]> 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)