> From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Sent: Monday, February 6, 2023 5:06 PM
>
> +long vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df,
> + unsigned long arg)
> +{
> + struct vfio_device *device = df->device;
> + struct vfio_device_bind_iommufd bind;
> + struct iommufd_ctx *iommufd = NULL;
> + unsigned long minsz;
> + struct fd f;
> + int ret;
> +
> + minsz = offsetofend(struct vfio_device_bind_iommufd, iommufd);
shouldn't the minsz include out_devid?
> + /*
> + * Before the first device open, get the KVM pointer currently
> + * associated with the device file (if there is) and obtain a
> + * reference. This reference is held until device closed. Save
> + * the pointer in the device for use by drivers.
> + */
> + vfio_device_get_kvm_safe(df);
there is no multi-open for cdev so "before the first device open"
doesn't make sense here.
> +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> + void __user *arg)
> +{
> + struct vfio_device *device = df->device;
> + struct vfio_device_attach_iommufd_pt attach;
> + int ret;
> +
> + if (copy_from_user(&attach, (void __user *)arg, sizeof(attach)))
> + return -EFAULT;
lack of a minsz check
> +
> + if (attach.flags || attach.pt_id == IOMMUFD_INVALID_ID)
> + return -EINVAL;
> +
> + if (!device->ops->bind_iommufd)
> + return -ENODEV;
> +
> + mutex_lock(&device->dev_set->lock);
> + if (df->noiommu)
> + return -ENODEV;
-EINVAL
> +
> +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> + void __user *arg)
> +{
> + struct vfio_device *device = df->device;
> + struct vfio_device_detach_iommufd_pt detach;
> +
> + if (copy_from_user(&detach, (void __user *)arg, sizeof(detach)))
> + return -EFAULT;
lack of minsz check
> + mutex_lock(&device->dev_set->lock);
> + if (df->noiommu)
> + return -ENODEV;
-EINVAL
> @@ -442,16 +443,41 @@ static int vfio_device_first_open(struct
> vfio_device_file *df,
> {
> struct vfio_device *device = df->device;
> struct iommufd_ctx *iommufd = df->iommufd;
> - int ret;
> + int ret = 0;
>
> lockdep_assert_held(&device->dev_set->lock);
>
> + /* df->iommufd and df->noiommu should be exclusive */
> + if (WARN_ON(iommufd && df->noiommu))
pointless comment
> + return -EINVAL;
> +
> if (!try_module_get(device->dev->driver->owner))
> return -ENODEV;
>
> + /*
> + * For group path, iommufd pointer is NULL when comes into this
"For group/container path"
> + * helper. Its noiommu support is in container.c.
> + *
> + * For iommufd compat mode, iommufd pointer here is a valid value.
> + * Its noiommu support is supposed to be in vfio_iommufd_bind().
remove "supposed to be"
> + *
> + * For device cdev path, iommufd pointer here is a valid value for
> + * normal cases, but it is NULL if it's noiommu. The reason is
> + * that userspace uses iommufd==-1 to indicate noiommu mode in
> this
"iommufd < 0"
> + * path. So caller of this helper will pass in a NULL iommufd
I don't think that is the reason. Instead the caller is required to pass
NULL iommufd pointer to indicate noiommu. Just remove the reason part.
> + * pointer. To differentiate it from the group path which also
> + * passes NULL iommufd pointer in, df->noiommu is used. For cdev
> + * noiommu, df->noiommu would be set to mark noiommu case for
> cdev
> + * path.
"To differentiate ..., check df->noiommu which is set only in the cdev path"
> + *
> + * So if df->noiommu is set then this helper just goes ahead to
> + * open device. If not, it depends on if iommufd pointer is NULL
> + * to handle the group path, iommufd compat mode, normal cases in
> + * the cdev path.
this doesn't match the code order. Probably can be removed after earlier
explanation.
> + * @argsz: user filled size of this data.
> + * @flags: reserved for future extension.
> + * @dev_cookie: a per device cookie provided by userspace.
> + * @iommufd: iommufd to bind. iommufd < 0 means noiommu.
s/iommufd < 0/a negative value/
