[PATCH v6 13/24] vfio/iommufd: Split the compat_ioas attach out from vfio_iommufd_bind()

[Liu, Yi L]

Hi Keivn,

> From: Liu, Yi L <yi.l.liu at intel.com>
> Sent: Friday, March 10, 2023 4:22 PM
> 
> > From: Tian, Kevin <kevin.tian at intel.com>
> > Sent: Friday, March 10, 2023 4:08 PM
> >
> > > From: Liu, Yi L <yi.l.liu at intel.com>
> > > Sent: Wednesday, March 8, 2023 9:29 PM
> > >
> > > @@ -177,7 +177,7 @@ static int vfio_device_group_open(struct
> > > vfio_device_file *df)
> > >  	mutex_lock(&device->group->group_lock);
> > >  	if (!vfio_group_has_iommu(device->group)) {
> > >  		ret = -EINVAL;
> > > -		goto out_unlock;
> > > +		goto err_unlock;
> > >  	}
> >
> > My impression - out_xxx means go to do xxx while err_xxx means
> > go to do something for error xxx, though in many places the two
> > are mixed to both meaning 'do xxx'.
> >
> > either way I don't see a need of changing it.
> 
> Ok. I'm fine with either way.
> 
> > > -int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx
> > *ictx)
> > > +static int vfio_iommufd_device_probe_comapt_noiommu(struct
> > vfio_device
> > > *vdev,
> > > +						    struct iommufd_ctx *ictx)
> >
> > s/comapt/compat/
> >
> > btw it's clearer to move this check into vfio_device_group_open().
> >
> > if noiommu then pass NULL to vfio_device_open(), same as the cdev path.
> 
> Right.
> 
> > > +
> > > +int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx
> > *ictx)
> > > +{
> > >  	u32 device_id;
> > >  	int ret;
> > >
> > >  	lockdep_assert_held(&vdev->dev_set->lock);
> > >
> > >  	if (vfio_device_is_noiommu(vdev)) {
> > > -		if (!capable(CAP_SYS_RAWIO))
> > > -			return -EPERM;
> > > -
> > > -		/*
> > > -		 * Require no compat ioas to be assigned to proceed. The
> > > basic
> > > -		 * statement is that the user cannot have done something
> > > that
> > > -		 * implies they expected translation to exist
> > > -		 */
> > > -		if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id))
> > > -			return -EPERM;
> > > -		return 0;
> > > +		ret = vfio_iommufd_device_probe_comapt_noiommu(vdev,
> > > ictx);
> > > +		if (ret)
> > > +			return ret;
> > >  	}
> > >
> > >  	if (WARN_ON(!vdev->ops->bind_iommufd))
> > >  		return -ENODEV;
> > >
> > > -	ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id);
> > > -	if (ret)
> > > -		return ret;
> > > +	/* The legacy path has no way to return the device id */
> > > +	return vdev->ops->bind_iommufd(vdev, ictx, &device_id);
> > > +}
> > >
> > > -	ret = iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id);
> > > -	if (ret)
> > > -		goto err_unbind;
> > > -	ret = vdev->ops->attach_ioas(vdev, &ioas_id);
> > > -	if (ret)
> > > -		goto err_unbind;
> >
> > after noiommu check and attach_ioas are moved out then this
> > entire function can be removed now. Just call the ops in
> > vfio_device_first_open().
> 
> Yes. and also no vfio_iommufd_unbind().

Seems still necessary to have this wrapper. .bind_iommufd callback would
be NULL if CONFIG_IOMMUFD==n. If we call ops->bind_iommufd directly
in vfio_device_first_open() of vfio_main.c, it may trigger kernel panic
for NULL pointer dereference if there is wrong code that passes valid
iommufd pointer.. Ideally, if CONFIG_IOMMUFD==n, vfio_device_first_open
should not receive valid iommufd pointer hence won't call ops->bind_iommufd
at all. So it deserves a panic. However, if we have a wrapper for it, such code
may just fail with -EOPNOTSUPPT.

> >
> > > +int vfio_iommufd_attach_compat_ioas(struct vfio_device *vdev,
> > > +				    struct iommufd_ctx *ictx)
> > > +{
> > > +	u32 ioas_id;
> > > +	int ret;
> > > +
> > > +	lockdep_assert_held(&vdev->dev_set->lock);
> > >
> > >  	/*
> > > -	 * The legacy path has no way to return the device id or the selected
> > > -	 * pt_id
> > > +	 * If the driver doesn't provide this op then it means the device does
> > > +	 * not do DMA at all. So nothing to do.
> > >  	 */
> > > -	return 0;
> > > +	if (WARN_ON(!vdev->ops->bind_iommufd))
> > > +		return -ENODEV;
> > >
> > > -err_unbind:
> > > -	if (vdev->ops->unbind_iommufd)
> > > -		vdev->ops->unbind_iommufd(vdev);
> > > -	return ret;
> > > +	if (vfio_device_is_noiommu(vdev)) {
> > > +		if
> > > (WARN_ON(vfio_iommufd_device_probe_comapt_noiommu(vdev,
> ictx)))
> > > +			return -EINVAL;
> > > +		return 0;
> > > +	}
> >
> > no need. let's directly call following from vfio_device_group_open().
> > In that case no need to do noiommu check twice in one function.
> 
> Ok. maybe still have vfio_iommufd_attach_compat_ioas() but
> only call it if it's not noiommu mode. vfio_device_group_open()
> can call probe_noiommu() first and has a bool to mark noiommu.
> Jason had a remark that it's better to keep the
> iommufd_vfio_compat_ioas_get_id() in iommufd.c

Same with .bind_iommufd(). If we move the compat ioas attach
code to group.c, it may encounter kernel panic if there is wrong
code that passes valid iommufd pointer.

> >
> > > +
> > > +	ret = iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* The legacy path has no way to return the selected pt_id */
> > > +	return vdev->ops->attach_ioas(vdev, &ioas_id);
> > >  }
> > >

Regards,
Yi Liu