On Mon, 4 Nov 2024 05:27:31 -0800
Yi Liu <yi.l.liu@xxxxxxxxx> wrote:
> This extends the VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT ioctls to attach/detach
> a given pasid of a vfio device to/from an IOAS/HWPT.
>
> vfio_copy_from_user() is added to copy the user data for the case in which
> the existing user struct has introduced new fields. The rule is not breaking
> the existing usersapce. The kernel only copies the new fields when the
> corresponding flag is set by the userspace. For the case that has multiple
> new fields marked by different flags, kernel checks the flags one by one to
> get the correct size to copy besides the minsz. Such logics can be shared by
> the other uapi extensions, hence add a helper for it.
>
> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> ---
> drivers/vfio/device_cdev.c | 62 +++++++++++++++++++++++++++-----------
> drivers/vfio/vfio.h | 18 +++++++++++
> drivers/vfio/vfio_main.c | 55 +++++++++++++++++++++++++++++++++
> include/uapi/linux/vfio.h | 29 ++++++++++++------
> 4 files changed, 136 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> index bb1817bd4ff3..bd13ddbfb9e3 100644
> --- a/drivers/vfio/device_cdev.c
> +++ b/drivers/vfio/device_cdev.c
> @@ -159,24 +159,44 @@ 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),
> +};
Doesn't this rather imply that every valid flag bit indicates some new
structure field?
For example, we start out with:
struct vfio_device_attach_iommufd_pt {
__u32 argsz;
__u32 flags;
__u32 pt_id;
};
And then here it becomes:
struct vfio_device_attach_iommufd_pt {
__u32 argsz;
__u32 flags;
#define VFIO_DEVICE_ATTACH_PASID (1 << 0)
__u32 pt_id;
__u32 pasid;
};
What if the next flag is simply related to the processing of @pt_id and
doesn't require @pasid?
The xend array necessarily expands, but what's the value? Logically it
would be offsetofend(, pt_id), so the array becomes { 16, 12 }.
Similarly, rather than pasid we might have reused a previously
reserved field, for instance what if we already expanded the structure
as:
struct vfio_device_attach_iommufd_pt {
__u32 argsz;
__u32 flags;
#define VFIO_DEVICE_ATTACH_FOO (1 << 0)
__u32 pt_id;
__u32 reserved;
__u64 foo;
};
If we then want to add @pasid, we might really prefer to take advantage
of that reserved field and the array becomes { 24, 16 }.
I think these can work (see below), but this seems like a pretty
complicated generalization. It might make sense to initially open code
the handling for @pasid with a follow-on patch with this sort of
generalization so we can evaluate them separately.
BTW, don't feel obligated to use "xend" based on my email sample code.
> +
> int vfio_df_ioctl_attach_pt(struct vfio_device_file *df,
> struct vfio_device_attach_iommufd_pt __user *arg)
> {
> - struct vfio_device *device = df->device;
> struct vfio_device_attach_iommufd_pt attach;
> - unsigned long minsz;
> + struct vfio_device *device = df->device;
> int ret;
>
> - minsz = offsetofend(struct vfio_device_attach_iommufd_pt, pt_id);
> -
> - if (copy_from_user(&attach, arg, 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.argsz < minsz || attach.flags)
> - return -EINVAL;
> + if ((attach.flags & VFIO_DEVICE_ATTACH_PASID) &&
> + !device->ops->pasid_attach_ioas)
> + return -EOPNOTSUPP;
>
> mutex_lock(&device->dev_set->lock);
> - ret = device->ops->attach_ioas(device, &attach.pt_id);
> + if (attach.flags & VFIO_DEVICE_ATTACH_PASID)
> + ret = device->ops->pasid_attach_ioas(device, attach.pasid,
> + &attach.pt_id);
> + else
> + ret = device->ops->attach_ioas(device, &attach.pt_id);
> if (ret)
> goto out_unlock;
>
> @@ -198,20 +218,26 @@ int vfio_df_ioctl_attach_pt(struct vfio_device_file *df,
> int vfio_df_ioctl_detach_pt(struct vfio_device_file *df,
> struct vfio_device_detach_iommufd_pt __user *arg)
> {
> - struct vfio_device *device = df->device;
> struct vfio_device_detach_iommufd_pt detach;
> - unsigned long minsz;
> -
> - minsz = offsetofend(struct vfio_device_detach_iommufd_pt, flags);
> + struct vfio_device *device = df->device;
> + int ret;
>
> - if (copy_from_user(&detach, arg, 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.argsz < minsz || detach.flags)
> - return -EINVAL;
> + if ((detach.flags & VFIO_DEVICE_DETACH_PASID) &&
> + !device->ops->pasid_detach_ioas)
> + return -EOPNOTSUPP;
>
> mutex_lock(&device->dev_set->lock);
> - device->ops->detach_ioas(device);
> + if (detach.flags & VFIO_DEVICE_DETACH_PASID)
> + device->ops->pasid_detach_ioas(device, detach.pasid);
> + else
> + device->ops->detach_ioas(device);
> mutex_unlock(&device->dev_set->lock);
>
> return 0;
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index 50128da18bca..9f081cf01c5a 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)
We have a precedence in vfio_alloc_device() that macros wrapping
functions don't need to be all caps.
> +
> +#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..7df94bf121fd 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, it should never bump up.
> + * @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 = 0;
> + 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_LONG) {
I suppose it doesn't matter, but there's a logical inconsistency
searching BITS_PER_LONG on a buffer initialized by a u32.
> + if (xend_array[flag])
Given the earlier concern, this should be:
if (xend_array[flags] > xend)
Thanks,
Alex
> + xend = xend_array[flag];
> + }
> +
> + if (xend) {
> + if (header->argsz < xend)
> + return -EINVAL;
> +
> + if (copy_from_user(buffer + minsz,
> + arg + minsz, xend - minsz))
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> /*
> * Module/class support
> */
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 2b68e6cdf190..40b414e642f5 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -931,29 +931,34 @@ struct vfio_device_bind_iommufd {
> * VFIO_DEVICE_ATTACH_IOMMUFD_PT - _IOW(VFIO_TYPE, VFIO_BASE + 19,
> * struct vfio_device_attach_iommufd_pt)
> * @argsz: User filled size of this data.
> - * @flags: Must be 0.
> + * @flags: Flags for attach.
> * @pt_id: Input the target id which can represent an ioas or a hwpt
> * allocated via iommufd subsystem.
> * Output the input ioas id or the attached hwpt id which could
> * be the specified hwpt itself or a hwpt automatically created
> * for the specified ioas by kernel during the attachment.
> + * @pasid: The pasid to be attached, only meaningful when
> + * VFIO_DEVICE_ATTACH_PASID is set in @flags
> *
> * Associate the device with an address space within the bound iommufd.
> * Undo by VFIO_DEVICE_DETACH_IOMMUFD_PT or device fd close. This is only
> * allowed on cdev fds.
> *
> - * If a vfio device is currently attached to a valid hw_pagetable, without doing
> - * a VFIO_DEVICE_DETACH_IOMMUFD_PT, a second VFIO_DEVICE_ATTACH_IOMMUFD_PT ioctl
> - * passing in another hw_pagetable (hwpt) id is allowed. This action, also known
> - * as a hw_pagetable replacement, will replace the device's currently attached
> - * hw_pagetable with a new hw_pagetable corresponding to the given pt_id.
> + * If a vfio device or a pasid of this device is currently attached to a valid
> + * hw_pagetable (hwpt), without doing a VFIO_DEVICE_DETACH_IOMMUFD_PT, a second
> + * VFIO_DEVICE_ATTACH_IOMMUFD_PT ioctl passing in another hwpt id is allowed.
> + * This action, also known as a hw_pagetable replacement, will replace the
> + * currently attached hwpt of the device or the pasid of this device with a new
> + * hwpt corresponding to the given pt_id.
> *
> * Return: 0 on success, -errno on failure.
> */
> struct vfio_device_attach_iommufd_pt {
> __u32 argsz;
> __u32 flags;
> +#define VFIO_DEVICE_ATTACH_PASID (1 << 0)
> __u32 pt_id;
> + __u32 pasid;
> };
>
> #define VFIO_DEVICE_ATTACH_IOMMUFD_PT _IO(VFIO_TYPE, VFIO_BASE + 19)
> @@ -962,17 +967,21 @@ struct vfio_device_attach_iommufd_pt {
> * VFIO_DEVICE_DETACH_IOMMUFD_PT - _IOW(VFIO_TYPE, VFIO_BASE + 20,
> * struct vfio_device_detach_iommufd_pt)
> * @argsz: User filled size of this data.
> - * @flags: Must be 0.
> + * @flags: Flags for detach.
> + * @pasid: The pasid to be detached, only meaningful when
> + * VFIO_DEVICE_DETACH_PASID is set in @flags
> *
> - * Remove the association of the device and its current associated address
> - * space. After it, the device should be in a blocking DMA state. This is only
> - * allowed on cdev fds.
> + * Remove the association of the device or a pasid of the device and its current
> + * associated address space. After it, the device or the pasid should be in a
> + * blocking DMA state. This is only allowed on cdev fds.
> *
> * Return: 0 on success, -errno on failure.
> */
> struct vfio_device_detach_iommufd_pt {
> __u32 argsz;
> __u32 flags;
> +#define VFIO_DEVICE_DETACH_PASID (1 << 0)
> + __u32 pasid;
> };
>
> #define VFIO_DEVICE_DETACH_IOMMUFD_PT _IO(VFIO_TYPE, VFIO_BASE + 20)
