On Mon, Nov 28, 2022 at 04:48:58PM +0100, Eric Auger wrote:
> > +/**
> > + * iommufd_access_notify_unmap - Notify users of an iopt to stop using it
> > + * @iopt: iopt to work on
> > + * @iova: Starting iova in the iopt
> > + * @length: Number of bytes
> > + *
> > + * After this function returns there should be no users attached to the pages
> > + * linked to this iopt that intersect with iova,length. Anyone that has attached
> > + * a user through iopt_access_pages() needs to detatch it through
> detach
> > + * iommufd_access_unpin_pages() before this function returns.
> > + *
> > + * The unmap callback may not call or wait for a iommufd_access_destroy() to
> > + * complete. Once iommufd_access_destroy() returns no ops are running and no
> > + * future ops will be called.
> I don't understand the above sentence. Is that related to the
>
> + if (!iommufd_lock_obj(&access->obj))
> + continue;
>
> where is the unmap() called in that case?
It is basically saying a driver cannot write this:
unmap():
mutex_lock(lock)
iommufd_access_unpin_pages(access)
mutex_unlock(lock)
driver_close
mutex_lock(lock)
iommufd_access_destroy(access)
mutex_unlock(lock)
Or any other equivalent thing. How about
* iommufd_access_destroy() will wait for any outstanding unmap callback to
* complete. Once iommufd_access_destroy() no unmap ops are running or will
* run in the future. Due to this a driver must not create locking that prevents
* unmap to complete while iommufd_access_destroy() is running.
And I should really add a lockdep map here, which I will add as a
followup patch:
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index de1babd56af156..d2b8e33ffaa0d7 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -5,6 +5,7 @@
#include <linux/slab.h>
#include <linux/iommu.h>
#include <linux/irqdomain.h>
+#include <linux/lockdep.h>
#include "io_pagetable.h"
#include "iommufd_private.h"
@@ -501,6 +502,15 @@ void iommufd_access_destroy(struct iommufd_access *access)
{
bool was_destroyed;
+ /*
+ * Alert lockdep that this cannot become entangled with an unmap
+ * callback, or we will have deadlock.
+ */
+#ifdef CONFIG_LOCKDEP
+ lock_acquire_exclusive(&access->ioas->iopt.unmap_map, 0, 0, NULL, _RET_IP_);
+ lock_release(&access->ioas->iopt.unmap_map, _RET_IP_);
+#endif
+
was_destroyed = iommufd_object_destroy_user(access->ictx, &access->obj);
WARN_ON(!was_destroyed);
}
diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
index 3467cea795684c..d858cc7f241fd0 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -460,6 +460,9 @@ static int iopt_unmap_iova_range(struct io_pagetable *iopt, unsigned long start,
unsigned long unmapped_bytes = 0;
int rc = -ENOENT;
+#ifdef CONFIG_LOCKDEP
+ lock_acquire(&iopt->unmap_map, 0, 0, NULL, _RET_IP_);
+#endif
/*
* The domains_rwsem must be held in read mode any time any area->pages
* is NULL. This prevents domain attach/detatch from running
@@ -521,6 +524,10 @@ static int iopt_unmap_iova_range(struct io_pagetable *iopt, unsigned long start,
up_read(&iopt->domains_rwsem);
if (unmapped)
*unmapped = unmapped_bytes;
+
+#ifdef CONFIG_LOCKDEP
+ lock_release(&iopt->unmap_map, _RET_IP_);
+#endif
return rc;
}
@@ -643,6 +650,14 @@ void iopt_init_table(struct io_pagetable *iopt)
* restriction.
*/
iopt->iova_alignment = 1;
+
+#ifdef CONFIG_LOCKDEP
+ {
+ static struct lock_class_key key;
+
+ lockdep_init_map(&iopt->unmap_map, "access_unmap", &key, 0);
+ }
+#endif
}
void iopt_destroy_table(struct io_pagetable *iopt)
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 222e86591f8ac9..8fb8e53ee0d3d3 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -45,6 +45,10 @@ struct io_pagetable {
struct rb_root_cached reserved_itree;
u8 disable_large_pages;
unsigned long iova_alignment;
+
+#ifdef CONFIG_LOCKDEP
+ struct lockdep_map unmap_map;
+#endif
};
void iopt_init_table(struct io_pagetable *iopt);
> > +/**
> > + * iommufd_access_pin_pages() - Return a list of pages under the iova
> > + * @access: IOAS access to act on
> > + * @iova: Starting IOVA
> > + * @length: Number of bytes to access
> > + * @out_pages: Output page list
> > + * @flags: IOPMMUFD_ACCESS_RW_* flags
> > + *
> > + * Reads @length bytes starting at iova and returns the struct page * pointers.
> > + * These can be kmap'd by the caller for CPU access.
> > + *
> > + * The caller must perform iopt_unaccess_pages() when done to balance this.
> this function does not exist
iommufd_access_unpin_pages()
Thanks,
Jason
