Re: [PATCH v12 14/24] iommufd/device: Add iommufd_access_detach() API

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Jun 02, 2023 at 05:16:43AM -0700, Yi Liu wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
> 
> Previously, the detach routine is only done by the destroy(). And it was
> called by vfio_iommufd_emulated_unbind() when the device runs close(), so
> all the mappings in iopt were cleaned in that setup, when the call trace
> reaches this detach() routine.
> 
> Now, there's a need of a detach uAPI, meaning that it does not only need
> a new iommufd_access_detach() API, but also requires access->ops->unmap()
> call as a cleanup. So add one.
> 
> However, leaving that unprotected can introduce some potential of a race
> condition during the pin_/unpin_pages() call, where access->ioas->iopt is
> getting referenced. So, add an ioas_lock to protect the context of iopt
> referencings.
> 
> Also, to allow the iommufd_access_unpin_pages() callback to happen via
> this unmap() call, add an ioas_unpin pointer, so the unpin routine won't
> be affected by the "access->ioas = NULL" trick.
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Tested-by: Terrence Xu <terrence.xu@intel.com>
> Tested-by: Yanting Jiang <yanting.jiang@intel.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommufd/device.c          | 76 +++++++++++++++++++++++--
>  drivers/iommu/iommufd/iommufd_private.h |  2 +
>  include/linux/iommufd.h                 |  1 +
>  3 files changed, 74 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 96d4281bfa7c..6b4ff635c15e 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -486,6 +486,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
>  	iommufd_ctx_get(ictx);
>  	iommufd_object_finalize(ictx, &access->obj);
>  	*id = access->obj.id;
> +	mutex_init(&access->ioas_lock);
>  	return access;
>  }
>  EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD);
> @@ -505,26 +506,66 @@ void iommufd_access_destroy(struct iommufd_access *access)
>  }
>  EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
>  
> +static void __iommufd_access_detach(struct iommufd_access *access)
> +{
> +	struct iommufd_ioas *cur_ioas = access->ioas;
> +
> +	lockdep_assert_held(&access->ioas_lock);
> +	/*
> +	 * Set ioas to NULL to block any further iommufd_access_pin_pages().
> +	 * iommufd_access_unpin_pages() can continue using access->ioas_unpin.
> +	 */
> +	access->ioas = NULL;
> +
> +	if (access->ops->unmap) {
> +		mutex_unlock(&access->ioas_lock);
> +		access->ops->unmap(access->data, 0, ULONG_MAX);
> +		mutex_lock(&access->ioas_lock);
> +	}
> +	iopt_remove_access(&cur_ioas->iopt, access);
> +	refcount_dec(&cur_ioas->obj.users);
> +}
> +
> +void iommufd_access_detach(struct iommufd_access *access)
> +{
> +	mutex_lock(&access->ioas_lock);
> +	if (WARN_ON(!access->ioas))
> +		goto out;
> +	__iommufd_access_detach(access);
> +out:
> +	access->ioas_unpin = NULL;
> +	mutex_unlock(&access->ioas_lock);
> +}
> +EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD);

There is not really any benefit to make this two functions

> int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> {
[..]
> 	if (access->ioas) {

if (access->ioas || access->ioas_unpin) {

But I wonder if it should be a WARN_ON? Does VFIO protect against
the userspace racing detach and attach, or do we expect to do it here?

> @@ -579,8 +620,8 @@ void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
>  void iommufd_access_unpin_pages(struct iommufd_access *access,
>  				unsigned long iova, unsigned long length)
>  {
> -	struct io_pagetable *iopt = &access->ioas->iopt;
>  	struct iopt_area_contig_iter iter;
> +	struct io_pagetable *iopt;
>  	unsigned long last_iova;
>  	struct iopt_area *area;
>  
> @@ -588,6 +629,13 @@ void iommufd_access_unpin_pages(struct iommufd_access *access,
>  	    WARN_ON(check_add_overflow(iova, length - 1, &last_iova)))
>  		return;
>  
> +	mutex_lock(&access->ioas_lock);
> +	if (!access->ioas_unpin) {

This should be WARN_ON(), the driver has done something wrong if we
call this after the access has been detached.

Jason