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

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Jun 23, 2023 at 11:15:40AM -0300, Jason Gunthorpe wrote:

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

The __iommufd_access_detach() will be used by replace() in the
following series. Yet, let's merge them here then. And I'll add
__iommufd_access_detach() back in the replace series.

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

Ack.

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

VFIO has a vdev->iommufd_attached flag to prevent a double call
of this function. And detach and attach there also have a mutex
protection. So it should be a WARN_ON here, I think.

> > @@ -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.

Ack. Also adding a line of comments for that:
+       /*
+        * The driver must be doing something wrong if it calls this before an
+        * iommufd_access_attach() or after an iommufd_access_detach().
+        */
+       if (WARN_ON(!access->ioas_unpin)) {

Thanks
Nic