Re: [PATCH v12 19/24] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT

[Jason Gunthorpe <jgg@xxxxxxxxxx>]

On Fri, Jun 02, 2023 at 05:16:48AM -0700, Yi Liu wrote:
> This adds ioctl for userspace to attach device cdev fd to and detach
> from IOAS/hw_pagetable managed by iommufd.
> 
>     VFIO_DEVICE_ATTACH_IOMMUFD_PT: attach vfio device to IOAS, hw_pagetable
> 				   managed by iommufd. Attach can be
> 				   undo by VFIO_DEVICE_DETACH_IOMMUFD_PT
> 				   or device fd close.
>     VFIO_DEVICE_DETACH_IOMMUFD_PT: detach vfio device from the current attached
> 				   IOAS or hw_pagetable managed by iommufd.
> 
> noiommu devices do not support [AT|DE]TACH, if user invokes the two ioctls
> on such devices, shall fail.

Stale comment

> Tested-by: Yanting Jiang <yanting.jiang@xxxxxxxxx>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx>
> Tested-by: Terrence Xu <terrence.xu@xxxxxxxxx>
> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> ---
>  drivers/vfio/device_cdev.c | 66 ++++++++++++++++++++++++++++++++++++++
>  drivers/vfio/vfio.h        | 16 +++++++++
>  drivers/vfio/vfio_main.c   |  8 +++++
>  include/uapi/linux/vfio.h  | 42 ++++++++++++++++++++++++
>  4 files changed, 132 insertions(+)
> 
> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> index a4498ddbe774..6e1d499ee160 100644
> --- a/drivers/vfio/device_cdev.c
> +++ b/drivers/vfio/device_cdev.c
> @@ -167,6 +167,72 @@ long vfio_df_ioctl_bind_iommufd(struct vfio_device_file *df,
>  	return ret;
>  }
>  
> +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;
> +	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 || attach.flags)
> +		return -EINVAL;
> +
> +	/* ATTACH only allowed for cdev fds */
> +	if (df->group)
> +		return -EINVAL;

I feel like vfio_device_fops_unl_ioctl() should do these group tests
for the whole lot

@@ -1187,19 +1187,24 @@ static long vfio_device_fops_unl_ioctl(struct file *filep,
        if (ret)
                return ret;
 
+       /* cdev only ioctls */
+       if (IS_ENABLED(CONFIG_VFIO_DEVICE_CDEV) && !df->group) {
+               switch (cmd) {
+               case VFIO_DEVICE_ATTACH_IOMMUFD_PT:
+                       ret = vfio_df_ioctl_attach_pt(df, (void __user *)arg);
+                       goto out;
+
+               case VFIO_DEVICE_DETACH_IOMMUFD_PT:
+                       ret = vfio_df_ioctl_detach_pt(df, (void __user *)arg);
+                       goto out;
+               }
+       }
+
        switch (cmd) {
        case VFIO_DEVICE_FEATURE:
                ret = vfio_ioctl_device_feature(device, (void __user *)arg);
                break;
 
-       case VFIO_DEVICE_ATTACH_IOMMUFD_PT:
-               ret = vfio_df_ioctl_attach_pt(df, (void __user *)arg);
-               break;
-
-       case VFIO_DEVICE_DETACH_IOMMUFD_PT:
-               ret = vfio_df_ioctl_detach_pt(df, (void __user *)arg);
-               break;
-
        default:
                if (unlikely(!device->ops->ioctl))
                        ret = -EINVAL;

And also make a local var for void __user * to avoid the repeated
casts.

Also this construction avoids the stub static inlines since the
IS_ENABLED will compile out the call. Just use a normal function
prototype outside any ifdef.

> +
> +	mutex_lock(&device->dev_set->lock);
> +	ret = device->ops->attach_ioas(device, &attach.pt_id);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = copy_to_user(&arg->pt_id, &attach.pt_id,
> +			   sizeof(attach.pt_id)) ? -EFAULT : 0;
> +	if (ret)
> +		goto out_detach;

Don't use the ?:

if (copy_to_user()..) {
    ret = -EFAULT;
    goto out_detach;
}

Jason