Messages in this thread | | | From | "Tian, Kevin" <> | Subject | RE: [PATCH v2 13/14] vfio: Add ioctls for device cdev using iommufd | Date | Tue, 7 Feb 2023 09:17:29 +0000 |
| |
> From: Liu, Yi L <yi.l.liu@intel.com> > 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/
| |