| Message ID | 20241108121742.18889-5-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfio-pci support pasid attach/detach | expand |
On Fri, 8 Nov 2024 04:17:41 -0800 Yi Liu <yi.l.liu@intel.com> 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/ > __u64 xxx; > }; > > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > 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. > + 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; (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, Alex > + } > + > + if (xend > minsz) { > + if (header->argsz < xend) > + return -EINVAL; > + > + if (copy_from_user(buffer + minsz, > + arg + minsz, xend - minsz)) > + return -EFAULT; > + } > + > + return 0; > +} > + > /* > * Module/class support > */
On 2024/11/12 08:03, Alex Williamson wrote: > On Fri, 8 Nov 2024 04:17:41 -0800 > Yi Liu <yi.l.liu@intel.com> 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@redhat.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> --- >> 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. > > (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));
On Tue, 12 Nov 2024 17:18:02 +0800 Yi Liu <yi.l.liu@intel.com> wrote: > On 2024/11/12 08:03, Alex Williamson wrote: > > On Fri, 8 Nov 2024 04:17:41 -0800 > > Yi Liu <yi.l.liu@intel.com> 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@redhat.com> > >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> > >> --- > >> 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
On 2024/11/12 21:52, Alex Williamson wrote: >>>> +{ >>>> + 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. I see. My thought was sticking with using it in the cases that have extended fields. Given that would it be better to return minsz as you suggested to return ssize_t to caller. > >>> (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, yes, but I encounter a problem to account it. The below failed as when the _xend_array is a null pointer. It's due to the usage of ARRAY_SIZE macro. If it's not doable, perhaps we can have two wrappers, one for copying user data with array, this should enforce the array num check with flags. While, the another one is for copying user data without array, no array num check. How about your opinion? BUILD_BUG_ON((_xend_array != NULL) && (ARRAY_SIZE(_xend_array) < ilog2(_flags_mask))); Compiling fail snippet: In file included from <command-line>: ./include/linux/array_size.h:11:38: warning: division ‘sizeof (long unsigned int *) / sizeof (long unsigned int)’ does not compute the number of array elements [-Wsizeof-pointer-div] 11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) | ^ ././include/linux/compiler_types.h:497:23: note: in definition of macro ‘__compiletime_assert’ 497 | if (!(condition)) \ | ^~~~~~~~~ ././include/linux/compiler_types.h:517:9: note: in expansion of macro ‘_compiletime_assert’ 517 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’ 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
On Wed, 13 Nov 2024 15:22:02 +0800 Yi Liu <yi.l.liu@intel.com> wrote: > On 2024/11/12 21:52, Alex Williamson wrote: > > >>>> +{ > >>>> + 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. > > I see. My thought was sticking with using it in the cases that have > extended fields. Given that would it be better to return minsz as you > suggested to return ssize_t to caller. If the xend_array is NULL, then yes it would do the copy, validate argsz and flags, and return minsz. > >>> (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, > yes, but I encounter a problem to account it. The below failed as when > the _xend_array is a null pointer. It's due to the usage of ARRAY_SIZE > macro. If it's not doable, perhaps we can have two wrappers, one for > copying user data with array, this should enforce the array num check > with flags. While, the another one is for copying user data without > array, no array num check. How about your opinion? > > BUILD_BUG_ON((_xend_array != NULL) && (ARRAY_SIZE(_xend_array) < > ilog2(_flags_mask))); > > Compiling fail snippet: > > In file included from <command-line>: > ./include/linux/array_size.h:11:38: warning: division ‘sizeof (long > unsigned int *) / sizeof (long unsigned int)’ does not compute the number > of array elements [-Wsizeof-pointer-div] > 11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + > __must_be_array(arr)) > | ^ > ././include/linux/compiler_types.h:497:23: note: in definition of macro > ‘__compiletime_assert’ > 497 | if (!(condition)) > \ > | ^~~~~~~~~ > ././include/linux/compiler_types.h:517:9: note: in expansion of macro > ‘_compiletime_assert’ > 517 | _compiletime_assert(condition, msg, __compiletime_assert_, > __COUNTER__) > | ^~~~~~~~~~~~~~~~~~~ > ./include/linux/build_bug.h:39:37: note: in expansion of macro > ‘compiletime_assert’ > 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) TBH, I think this whole generalization is stalling the series. We're de-duplicating 20-ish lines of code implemented by adjacent functions with something more complicated, I think mostly to formalize the methodology of using flags to expand the ioctl data structure, which has been our plan all along. If it only addresses the duplication in these two functions, the added complexity isn't that compelling, but expanding it to be used more broadly is introducing scope creep. Given the momentum on the iommufd side, if this series is intended to be v6.13 material, we should probably defer this generalization to a follow-on series where we can evaluate a more broadly used helper. Thanks, Alex
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, + 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; + + /* 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]; + } + + if (xend > minsz) { + if (header->argsz < xend) + return -EINVAL; + + if (copy_from_user(buffer + minsz, + arg + minsz, xend - minsz)) + return -EFAULT; + } + + return 0; +} + /* * Module/class support */
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; __u64 xxx; }; Suggested-by: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/vfio/device_cdev.c | 81 +++++++++++++------------------------- drivers/vfio/vfio.h | 18 +++++++++ drivers/vfio/vfio_main.c | 55 ++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 54 deletions(-)