On Tue, 12 Nov 2024 17:18:02 +0800
Yi Liu <yi.l.liu@xxxxxxxxx> wrote:
> On 2024/11/12 08:03, Alex Williamson wrote:
> > On Fri, 8 Nov 2024 04:17:41 -0800
> > Yi Liu <yi.l.liu@xxxxxxxxx> wrote:
> >
> >> This generalizes the logic of copying user data when the user struct
> >> Have new fields introduced. The helpers can be used by the vfio uapis
> >> that have the argsz and flags fields in the beginning 8 bytes.
> >>
> >> As an example, the vfio_device_{at|de}tach_iommufd_pt paths are updated
> >> to use the helpers.
> >>
> >> The flags may be defined to mark a new field in the structure, reuse
> >> reserved fields, or special handling of an existing field. The extended
> >> size would differ for different flags. Each user API that wants to use
> >> the generalized helpers should define an array to store the corresponding
> >> extended sizes for each defined flag.
> >>
> >> For example, we start out with the below, minsz is 12.
> >>
> >> struct vfio_foo_struct {
> >> __u32 argsz;
> >> __u32 flags;
> >> __u32 pt_id;
> >> };
> >>
> >> And then here it becomes:
> >>
> >> struct vfio_foo_struct {
> >> __u32 argsz;
> >> __u32 flags;
> >> #define VFIO_FOO_STRUCT_PASID (1 << 0)
> >> __u32 pt_id;
> >> __u32 pasid;
> >> };
> >>
> >> The array is { 16 }.
> >>
> >> If the next flag is simply related to the processing of @pt_id and
> >> doesn't require @pasid, then the extended size of the new flag is
> >> 12. The array become { 16, 12 }
> >>
> >> struct vfio_foo_struct {
> >> __u32 argsz;
> >> __u32 flags;
> >> #define VFIO_FOO_STRUCT_PASID (1 << 0)
> >> #define VFIO_FOO_STRUCT_SPECICAL_PTID (1 << 1)
> >> __u32 pt_id;
> >> __u32 pasid;
> >> };
> >>
> >> Similarly, rather than adding new field, we might have reused a previously
> >> reserved field, for instance what if we already expanded the structure
> >> as the below, array is already { 24 }.
> >>
> >> struct vfio_foo_struct {
> >> __u32 argsz;
> >> __u32 flags;
> >> #define VFIO_FOO_STRUCT_XXX (1 << 0)
> >> __u32 pt_id;
> >> __u32 reserved;
> >> __u64 xxx;
> >> };
> >>
> >> If we then want to add @pasid, we might really prefer to take advantage
> >> of that reserved field and the array becomes { 24, 16 }.
> >>
> >> struct vfio_foo_struct {
> >> __u32 argsz;
> >> __u32 flags;
> >> #define VFIO_FOO_STRUCT_XXX (1 << 0)
> >> #define VFIO_FOO_STRUCT_PASID (1 << 1)
> >> __u32 pt_id;
> >> __u32 reserved;
> >
> > I think this was supposed to be s/reserved/pasid/
>
> you are right.
>
> >> __u64 xxx;
> >> };
> >>
> >> Suggested-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> >> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> >> ---
> >> drivers/vfio/device_cdev.c | 81 +++++++++++++-------------------------
> >> drivers/vfio/vfio.h | 18 +++++++++
> >> drivers/vfio/vfio_main.c | 55 ++++++++++++++++++++++++++
> >> 3 files changed, 100 insertions(+), 54 deletions(-)
> >>
> >> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> >> index 4519f482e212..35c7664b9a97 100644
> >> --- a/drivers/vfio/device_cdev.c
> >> +++ b/drivers/vfio/device_cdev.c
> >> @@ -159,40 +159,33 @@ void vfio_df_unbind_iommufd(struct vfio_device_file *df)
> >> vfio_device_unblock_group(device);
> >> }
> >>
> >> +#define VFIO_ATTACH_FLAGS_MASK VFIO_DEVICE_ATTACH_PASID
> >> +static unsigned long
> >> +vfio_attach_xends[ilog2(VFIO_ATTACH_FLAGS_MASK) + 1] = {
> >> + XEND_SIZE(VFIO_DEVICE_ATTACH_PASID,
> >> + struct vfio_device_attach_iommufd_pt, pasid),
> >> +};
> >> +
> >> +#define VFIO_DETACH_FLAGS_MASK VFIO_DEVICE_DETACH_PASID
> >> +static unsigned long
> >> +vfio_detach_xends[ilog2(VFIO_DETACH_FLAGS_MASK) + 1] = {
> >> + XEND_SIZE(VFIO_DEVICE_DETACH_PASID,
> >> + struct vfio_device_detach_iommufd_pt, pasid),
> >> +};
> >> +
> >> int vfio_df_ioctl_attach_pt(struct vfio_device_file *df,
> >> struct vfio_device_attach_iommufd_pt __user *arg)
> >> {
> >> struct vfio_device_attach_iommufd_pt attach;
> >> struct vfio_device *device = df->device;
> >> - unsigned long minsz, xend = 0;
> >> int ret;
> >>
> >> - minsz = offsetofend(struct vfio_device_attach_iommufd_pt, pt_id);
> >> -
> >> - if (copy_from_user(&attach, arg, minsz))
> >> - return -EFAULT;
> >> -
> >> - if (attach.argsz < minsz)
> >> - return -EINVAL;
> >> -
> >> - if (attach.flags & (~VFIO_DEVICE_ATTACH_PASID))
> >> - return -EINVAL;
> >> -
> >> - if (attach.flags & VFIO_DEVICE_ATTACH_PASID)
> >> - xend = offsetofend(struct vfio_device_attach_iommufd_pt, pasid);
> >> -
> >> - /*
> >> - * xend may be equal to minsz if a flag is defined for reusing a
> >> - * reserved field or a special usage of an existing field.
> >> - */
> >> - if (xend > minsz) {
> >> - if (attach.argsz < xend)
> >> - return -EINVAL;
> >> -
> >> - if (copy_from_user((void *)&attach + minsz,
> >> - (void __user *)arg + minsz, xend - minsz))
> >> - return -EFAULT;
> >> - }
> >> + ret = vfio_copy_user_data((void __user *)arg, &attach,
> >> + struct vfio_device_attach_iommufd_pt,
> >> + pt_id, VFIO_ATTACH_FLAGS_MASK,
> >> + vfio_attach_xends);
> >> + if (ret)
> >> + return ret;
> >>
> >> if ((attach.flags & VFIO_DEVICE_ATTACH_PASID) &&
> >> !device->ops->pasid_attach_ioas)
> >> @@ -227,34 +220,14 @@ int vfio_df_ioctl_detach_pt(struct vfio_device_file *df,
> >> {
> >> struct vfio_device_detach_iommufd_pt detach;
> >> struct vfio_device *device = df->device;
> >> - unsigned long minsz, xend = 0;
> >> -
> >> - minsz = offsetofend(struct vfio_device_detach_iommufd_pt, flags);
> >> -
> >> - if (copy_from_user(&detach, arg, minsz))
> >> - return -EFAULT;
> >> -
> >> - if (detach.argsz < minsz)
> >> - return -EINVAL;
> >> -
> >> - if (detach.flags & (~VFIO_DEVICE_DETACH_PASID))
> >> - return -EINVAL;
> >> -
> >> - if (detach.flags & VFIO_DEVICE_DETACH_PASID)
> >> - xend = offsetofend(struct vfio_device_detach_iommufd_pt, pasid);
> >> -
> >> - /*
> >> - * xend may be equal to minsz if a flag is defined for reusing a
> >> - * reserved field or a special usage of an existing field.
> >> - */
> >> - if (xend > minsz) {
> >> - if (detach.argsz < xend)
> >> - return -EINVAL;
> >> + int ret;
> >>
> >> - if (copy_from_user((void *)&detach + minsz,
> >> - (void __user *)arg + minsz, xend - minsz))
> >> - return -EFAULT;
> >> - }
> >> + ret = vfio_copy_user_data((void __user *)arg, &detach,
> >> + struct vfio_device_detach_iommufd_pt,
> >> + flags, VFIO_DETACH_FLAGS_MASK,
> >> + vfio_detach_xends);
> >> + if (ret)
> >> + return ret;
> >>
> >> if ((detach.flags & VFIO_DEVICE_DETACH_PASID) &&
> >> !device->ops->pasid_detach_ioas)
> >> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> >> index 50128da18bca..87bed550c46e 100644
> >> --- a/drivers/vfio/vfio.h
> >> +++ b/drivers/vfio/vfio.h
> >> @@ -34,6 +34,24 @@ void vfio_df_close(struct vfio_device_file *df);
> >> struct vfio_device_file *
> >> vfio_allocate_device_file(struct vfio_device *device);
> >>
> >> +int vfio_copy_from_user(void *buffer, void __user *arg,
> >> + unsigned long minsz, u32 flags_mask,
> >> + unsigned long *xend_array);
> >> +
> >> +#define vfio_copy_user_data(_arg, _local_buffer, _struct, _min_last, \
> >> + _flags_mask, _xend_array) \
> >> + vfio_copy_from_user(_local_buffer, _arg, \
> >> + offsetofend(_struct, _min_last) + \
> >> + BUILD_BUG_ON_ZERO(offsetof(_struct, argsz) != \
> >> + 0) + \
> >> + BUILD_BUG_ON_ZERO(offsetof(_struct, flags) != \
> >> + sizeof(u32)), \
> >> + _flags_mask, _xend_array)
> >> +
> >> +#define XEND_SIZE(_flag, _struct, _xlast) \
> >> + [ilog2(_flag)] = offsetofend(_struct, _xlast) + \
> >> + BUILD_BUG_ON_ZERO(_flag == 0) \
> >> +
> >> extern const struct file_operations vfio_device_fops;
> >>
> >> #ifdef CONFIG_VFIO_NOIOMMU
> >> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> >> index a5a62d9d963f..c61336ea5123 100644
> >> --- a/drivers/vfio/vfio_main.c
> >> +++ b/drivers/vfio/vfio_main.c
> >> @@ -1694,6 +1694,61 @@ int vfio_dma_rw(struct vfio_device *device, dma_addr_t iova, void *data,
> >> }
> >> EXPORT_SYMBOL(vfio_dma_rw);
> >>
> >> +/**
> >> + * vfio_copy_from_user - Copy the user struct that may have extended fields
> >> + *
> >> + * @buffer: The local buffer to store the data copied from user
> >> + * @arg: The user buffer pointer
> >> + * @minsz: The minimum size of the user struct
> >> + * @flags_mask: The combination of all the falgs defined
> >> + * @xend_array: The array that stores the xend size for set flags.
> >> + *
> >> + * This helper requires the user struct put the argsz and flags fields in
> >> + * the first 8 bytes.
> >> + *
> >> + * Return 0 for success, otherwise -errno
> >> + */
> >> +int vfio_copy_from_user(void *buffer, void __user *arg,
> >
> > This should probably be prefixed with an underscore and note that
> > callers should use the wrapper function to impose the parameter
> > checking.
>
> got it.
>
> >
> >> + unsigned long minsz, u32 flags_mask,
> >> + unsigned long *xend_array)
> >> +{
> >> + unsigned long xend = minsz;
> >> + struct user_header {
> >> + u32 argsz;
> >> + u32 flags;
> >> + } *header;
> >> + unsigned long flags;
> >> + u32 flag;
> >> +
> >> + if (copy_from_user(buffer, arg, minsz))
> >> + return -EFAULT;
> >> +
> >> + header = (struct user_header *)buffer;
> >> + if (header->argsz < minsz)
> >> + return -EINVAL;
> >> +
> >> + if (header->flags & ~flags_mask)
> >> + return -EINVAL;
> >
> > I'm already wrestling with whether this is an over engineered solution
> > to remove a couple dozen lines of mostly duplicate logic between attach
> > and detach, but a couple points that could make it more versatile:
> >
> > (1) Test xend_array here:
> >
> > if (!xend_array)
> > return 0;
>
> Perhaps we should return error if the header->flags has any bit set. Such
> cases require a valid xend_array.
I don't think that's true. For example if we want to drop this into
existing cases where the structure size has not expanded and flags are
used for other things, I don't think we want the overhead of declaring
an xend_array.
> > (2) Return ssize_t/-errno for the caller to know the resulting copy
> > size.
> >
> >> +
> >> + /* Loop each set flag to decide the xend */
> >> + flags = header->flags;
> >> + for_each_set_bit(flag, &flags, BITS_PER_TYPE(u32)) {
> >> + if (xend_array[flag] > xend)
> >> + xend = xend_array[flag];
> >
> > Can we craft a BUILD_BUG in the wrapper to test that xend_array is at
> > least long enough to match the highest bit in flags? Thanks,
>
> yes. I would add a BUILD_BUG like the below.
>
> BUILD_BUG_ON(ARRAY_SIZE(_xend_array) < ilog2(_flags_mask));
So this would need to account that _xend_array can be NULL regardless
of _flags_mask. Thanks,
Alex
