RE: [PATCH v2 13/14] vfio: Add ioctls for device cdev using iommufd

[Tian, Kevin]

> 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/