Re: [PATCH v8 2/4] iommufd: Add iommufd_access_replace() API

[Nicolin Chen]

On Wed, Jul 26, 2023 at 11:30:17AM -0300, Jason Gunthorpe wrote:
> On Mon, Jul 24, 2023 at 12:47:05PM -0700, Nicolin Chen wrote:
> > -int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> > +static int iommufd_access_change_pt(struct iommufd_access *access, u32 ioas_id)
> >  {
> > +	struct iommufd_ioas *cur_ioas = access->ioas;
> >  	struct iommufd_ioas *new_ioas;
> > -	int rc = 0;
> > +	int rc;
> >  
> > -	mutex_lock(&access->ioas_lock);
> > -	if (WARN_ON(access->ioas || access->ioas_unpin)) {
> > -		mutex_unlock(&access->ioas_lock);
> > -		return -EINVAL;
> > -	}
> > +	lockdep_assert_held(&access->ioas_lock);
> >  
> >  	new_ioas = iommufd_get_ioas(access->ictx, ioas_id);
> > -	if (IS_ERR(new_ioas)) {
> > -		mutex_unlock(&access->ioas_lock);
> > +	if (IS_ERR(new_ioas))
> >  		return PTR_ERR(new_ioas);
> > -	}
> > +
> > +	if (cur_ioas)
> > +		__iommufd_access_detach(access);
> 
> The drop of the mutex while this function runs is racey with the rest
> of this, we can mitigate it by blocking concurrent change while
> detaching which is if access->ioas_unpin is set

Oh. You mean that unmap part dropping the mutex right? I see.

> >  	rc = iopt_add_access(&new_ioas->iopt, access);
> >  	if (rc) {
> > -		mutex_unlock(&access->ioas_lock);
> >  		iommufd_put_object(&new_ioas->obj);
> > +		if (cur_ioas)
> > +			WARN_ON(iommufd_access_change_pt(access,
> > +							 cur_ioas->obj.id));
> 
> We've already dropped our ref to cur_ioas, so this is also racy with
> destroy.

Would it be better by calling iommufd_access_detach() that holds
the same mutex in the iommufd_access_destroy_object()? We could
also unwrap the detach and delay the refcount_dec, as you did in
your attaching patch.

> This is what I came up with:
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 57c0e81f5073b2..e55d6e902edb98 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -758,64 +758,101 @@ void iommufd_access_destroy(struct iommufd_access *access)
>  }
>  EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
>  
> -void iommufd_access_detach(struct iommufd_access *access)
> +static int iommufd_access_change_ioas(struct iommufd_access *access,
> +				      struct iommufd_ioas *new_ioas)
>  {
>  	struct iommufd_ioas *cur_ioas = access->ioas;
> +	int rc;
> +
> +	lockdep_assert_held(&access->ioas_lock);
> +
> +	/* We are racing with a concurrent detach, bail */
> +	if (access->ioas_unpin)
> +		return -EBUSY;

I think this should check access->ioas too? I mean:

+	/* We are racing with a concurrent detach, bail */
+	if (!access->ioas && access->ioas_unpin)
+		return -EBUSY;

Otherwise, a normal detach() would fail, since an access has both
a valid ioas and a valid ioas_unpin.

> +
> +	if (IS_ERR(new_ioas))
> +		return PTR_ERR(new_ioas);
> +
> +	if (cur_ioas == new_ioas)
> +		return 0;
>  
> -	mutex_lock(&access->ioas_lock);
> -	if (WARN_ON(!access->ioas))
> -		goto out;
>  	/*
>  	 * 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) {
> +	if (cur_ioas && access->ops->unmap) {
>  		mutex_unlock(&access->ioas_lock);
>  		access->ops->unmap(access->data, 0, ULONG_MAX);
>  		mutex_lock(&access->ioas_lock);
>  	}
> +
> +	if (new_ioas) {
> +		rc = iopt_add_access(&new_ioas->iopt, access);
> +		if (rc) {
> +			iommufd_put_object(&new_ioas->obj);
> +			access->ioas = cur_ioas;
> +			return rc;
> +		}
> +		iommufd_ref_to_users(&new_ioas->obj);
> +	}
> +
> +	access->ioas = new_ioas;
> +	access->ioas_unpin = new_ioas;
>  	iopt_remove_access(&cur_ioas->iopt, access);

There was a bug in my earlier version, having the same flow by
calling iopt_add_access() prior to iopt_remove_access(). But,
doing that would override the access->iopt_access_list_id and
it would then get unset by the following iopt_remove_access().

Please refer to :
https://lore.kernel.org/linux-iommu/ZJYYWz2wy%2F86FapK@Asurada-Nvidia/

If we want a cleaner detach-then-attach flow, we would need an
atomic function in the io_pagetable.c file handling the id, yet
I couldn't figure a good naming for the atomic function since
it's about acccess shifting between two iopts other than simply
"iopt_repalce_access".

So, I came up with this version calling an iopt_remove_access()
prior to iopt_add_access(), which requires an add-back the old
ioas upon an failure at iopt_add_access(new_ioas).

I will try making some change accordingly on top of this patch.

Thanks
Nicolin