[PATCH V2 0/5] fixes for virtual address update

[PATCH V2 0/5] fixes for virtual address update

[Steve Sistare]

Fix bugs in the interfaces that allow the underlying memory object of an
iova range to be mapped in a new address space.  They allow userland to
indefinitely block vfio mediated device kernel threads, and do not
propagate the locked_vm count to a new mm.

The fixes impose restrictions that eliminate waiting conditions, so
revert the dead code:
  commit 898b9eaeb3fe ("vfio/type1: block on invalid vaddr")
  commit 487ace134053 ("vfio/type1: implement notify callback")
  commit ec5e32940cc9 ("vfio: iommu driver notify callback")

Changes from V1 (thanks Alex):
  * do not allow group attach while vaddrs are invalid
  * add patches to delete dead code
  * add WARN_ON for never-should-happen conditions
  * check for changed mm in unmap.
  * check for vfio_lock_acct failure in remap

Steve Sistare (5):
  vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR
  vfio/type1: prevent locked_vm underflow
  vfio/type1: revert "block on invalid vaddr"
  vfio/type1: revert "implement notify callback"
  vfio: revert "iommu driver notify callback"

 drivers/vfio/container.c        |   5 --
 drivers/vfio/vfio.h             |   7 --
 drivers/vfio/vfio_iommu_type1.c | 163 +++++++++++++---------------------------
 include/uapi/linux/vfio.h       |  15 ++--
 4 files changed, 63 insertions(+), 127 deletions(-)

-- 
1.8.3.1

[PATCH V2 1/5] vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR

[Steve Sistare]

Disable the VFIO_UPDATE_VADDR capability if mediated devices are present.
Their kernel threads could be blocked indefinitely by a misbehaving
userland while trying to pin/unpin pages while vaddrs are being updated.

Do not allow groups to be added to the container while vaddr's are invalid,
so we never need to block user threads from pinning, and can delete the
vaddr-waiting code in a subsequent patch.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 drivers/vfio/vfio_iommu_type1.c | 31 ++++++++++++++++++++++++++++++-
 include/uapi/linux/vfio.h       | 15 +++++++++------
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 23c24fe..80bdb4d 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -859,6 +859,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
 	if (!iommu->v2)
 		return -EACCES;
 
+	WARN_ON(iommu->vaddr_invalid_count);
+
 	mutex_lock(&iommu->lock);
 
 	/*
@@ -976,6 +978,8 @@ static void vfio_iommu_type1_unpin_pages(void *iommu_data,
 
 	mutex_lock(&iommu->lock);
 
+	WARN_ON(iommu->vaddr_invalid_count);
+
 	do_accounting = list_empty(&iommu->domain_list);
 	for (i = 0; i < npage; i++) {
 		dma_addr_t iova = user_iova + PAGE_SIZE * i;
@@ -1343,6 +1347,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
 
 	mutex_lock(&iommu->lock);
 
+	/* Cannot update vaddr if mdev is present. */
+	if (invalidate_vaddr && !list_empty(&iommu->emulated_iommu_groups))
+		goto unlock;
+
 	pgshift = __ffs(iommu->pgsize_bitmap);
 	pgsize = (size_t)1 << pgshift;
 
@@ -2189,6 +2197,10 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 
 	mutex_lock(&iommu->lock);
 
+	/* Attach could require pinning, so disallow while vaddr is invalid. */
+	if (iommu->vaddr_invalid_count)
+		goto out_unlock;
+
 	/* Check for duplicates */
 	if (vfio_iommu_find_iommu_group(iommu, iommu_group))
 		goto out_unlock;
@@ -2660,6 +2672,16 @@ static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
 	return ret;
 }
 
+static int vfio_iommu_has_emulated(struct vfio_iommu *iommu)
+{
+	int ret;
+
+	mutex_lock(&iommu->lock);
+	ret = !list_empty(&iommu->emulated_iommu_groups);
+	mutex_unlock(&iommu->lock);
+	return ret;
+}
+
 static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
 					    unsigned long arg)
 {
@@ -2668,8 +2690,13 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
 	case VFIO_TYPE1v2_IOMMU:
 	case VFIO_TYPE1_NESTING_IOMMU:
 	case VFIO_UNMAP_ALL:
-	case VFIO_UPDATE_VADDR:
 		return 1;
+	case VFIO_UPDATE_VADDR:
+		/*
+		 * Disable this feature if mdevs are present.  They cannot
+		 * safely pin/unpin while vaddrs are being updated.
+		 */
+		return iommu && !vfio_iommu_has_emulated(iommu);
 	case VFIO_DMA_CC_IOMMU:
 		if (!iommu)
 			return 0;
@@ -3080,6 +3107,8 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
 	size_t offset;
 	int ret;
 
+	WARN_ON(iommu->vaddr_invalid_count);
+
 	*copied = 0;
 
 	ret = vfio_find_dma_valid(iommu, user_iova, 1, &dma);
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index d7d8e09..4e8d344 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -49,7 +49,11 @@
 /* Supports VFIO_DMA_UNMAP_FLAG_ALL */
 #define VFIO_UNMAP_ALL			9
 
-/* Supports the vaddr flag for DMA map and unmap */
+/*
+ * Supports the vaddr flag for DMA map and unmap.  Not supported for mediated
+ * devices, so this capability is subject to change as groups are added or
+ * removed.
+ */
 #define VFIO_UPDATE_VADDR		10
 
 /*
@@ -1215,8 +1219,7 @@ struct vfio_iommu_type1_info_dma_avail {
  * Map process virtual addresses to IO virtual addresses using the
  * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
  *
- * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova, and
- * unblock translation of host virtual addresses in the iova range.  The vaddr
+ * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova. The vaddr
  * must have previously been invalidated with VFIO_DMA_UNMAP_FLAG_VADDR.  To
  * maintain memory consistency within the user application, the updated vaddr
  * must address the same memory object as originally mapped.  Failure to do so
@@ -1267,9 +1270,9 @@ struct vfio_bitmap {
  * must be 0.  This cannot be combined with the get-dirty-bitmap flag.
  *
  * If flags & VFIO_DMA_UNMAP_FLAG_VADDR, do not unmap, but invalidate host
- * virtual addresses in the iova range.  Tasks that attempt to translate an
- * iova's vaddr will block.  DMA to already-mapped pages continues.  This
- * cannot be combined with the get-dirty-bitmap flag.
+ * virtual addresses in the iova range.  DMA to already-mapped pages continues.
+ * Groups may not be added to the container while any addresses are invalid.
+ * This cannot be combined with the get-dirty-bitmap flag.
  */
 struct vfio_iommu_type1_dma_unmap {
 	__u32	argsz;
-- 
1.8.3.1

[PATCH V2 2/5] vfio/type1: prevent locked_vm underflow

[Steve Sistare]

When a vfio container is preserved across exec using the VFIO_UPDATE_VADDR
interfaces, locked_vm of the new mm becomes 0.  If the user later unmaps a
dma mapping, locked_vm underflows to a large unsigned value, and a
subsequent dma map request fails with ENOMEM in __account_locked_vm.

To avoid underflow, do not decrement locked_vm during unmap if the
dma's mm has changed.  To restore the correct locked_vm count, when
VFIO_DMA_MAP_FLAG_VADDR is used and the dma's mm has changed, add
the mapping's pinned page count to the new mm->locked_vm, subject
to the rlimit.  Now that mediated devices are excluded when using
VFIO_UPDATE_VADDR, the amount of pinned memory equals the size of
the mapping.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 drivers/vfio/vfio_iommu_type1.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 80bdb4d..35a1a52 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -100,6 +100,7 @@ struct vfio_dma {
 	struct task_struct	*task;
 	struct rb_root		pfn_list;	/* Ex-user pinned pfn list */
 	unsigned long		*bitmap;
+	struct mm_struct	*mm;
 };
 
 struct vfio_batch {
@@ -1165,7 +1166,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
 					    &iotlb_gather);
 	}
 
-	if (do_accounting) {
+	if (do_accounting && current->mm == dma->mm) {
 		vfio_lock_acct(dma, -unlocked, true);
 		return 0;
 	}
@@ -1178,6 +1179,7 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
 	vfio_unmap_unpin(iommu, dma, true);
 	vfio_unlink_dma(iommu, dma);
 	put_task_struct(dma->task);
+	mmdrop(dma->mm);
 	vfio_dma_bitmap_free(dma);
 	if (dma->vaddr_invalid) {
 		iommu->vaddr_invalid_count--;
@@ -1623,9 +1625,20 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 			   dma->size != size) {
 			ret = -EINVAL;
 		} else {
-			dma->vaddr = vaddr;
-			dma->vaddr_invalid = false;
-			iommu->vaddr_invalid_count--;
+			if (current->mm != dma->mm) {
+				ret = vfio_lock_acct(dma, size >> PAGE_SHIFT,
+						     0);
+				if (!ret) {
+					mmdrop(dma->mm);
+					dma->mm = current->mm;
+					mmgrab(dma->mm);
+				}
+			}
+			if (!ret) {
+				dma->vaddr = vaddr;
+				dma->vaddr_invalid = false;
+				iommu->vaddr_invalid_count--;
+			}
 			wake_up_all(&iommu->vaddr_wait);
 		}
 		goto out_unlock;
@@ -1683,6 +1696,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 	get_task_struct(current->group_leader);
 	dma->task = current->group_leader;
 	dma->lock_cap = capable(CAP_IPC_LOCK);
+	dma->mm = dma->task->mm;
+	mmgrab(dma->mm);
 
 	dma->pfn_list = RB_ROOT;
 
-- 
1.8.3.1

[PATCH V2 3/5] vfio/type1: revert "block on invalid vaddr"

[Steve Sistare]

Revert this dead code:
  commit 898b9eaeb3fe ("vfio/type1: block on invalid vaddr")

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 drivers/vfio/vfio_iommu_type1.c | 94 +++--------------------------------------
 1 file changed, 5 insertions(+), 89 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 35a1a52..d4f961d 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -72,7 +72,6 @@ struct vfio_iommu {
 	unsigned int		vaddr_invalid_count;
 	uint64_t		pgsize_bitmap;
 	uint64_t		num_non_pinned_groups;
-	wait_queue_head_t	vaddr_wait;
 	bool			v2;
 	bool			nesting;
 	bool			dirty_page_tracking;
@@ -153,8 +152,6 @@ struct vfio_regions {
 #define DIRTY_BITMAP_PAGES_MAX	 ((u64)INT_MAX)
 #define DIRTY_BITMAP_SIZE_MAX	 DIRTY_BITMAP_BYTES(DIRTY_BITMAP_PAGES_MAX)
 
-#define WAITED 1
-
 static int put_pfn(unsigned long pfn, int prot);
 
 static struct vfio_iommu_group*
@@ -596,61 +593,6 @@ static int vaddr_get_pfns(struct mm_struct *mm, unsigned long vaddr,
 	return ret;
 }
 
-static int vfio_wait(struct vfio_iommu *iommu)
-{
-	DEFINE_WAIT(wait);
-
-	prepare_to_wait(&iommu->vaddr_wait, &wait, TASK_KILLABLE);
-	mutex_unlock(&iommu->lock);
-	schedule();
-	mutex_lock(&iommu->lock);
-	finish_wait(&iommu->vaddr_wait, &wait);
-	if (kthread_should_stop() || !iommu->container_open ||
-	    fatal_signal_pending(current)) {
-		return -EFAULT;
-	}
-	return WAITED;
-}
-
-/*
- * Find dma struct and wait for its vaddr to be valid.  iommu lock is dropped
- * if the task waits, but is re-locked on return.  Return result in *dma_p.
- * Return 0 on success with no waiting, WAITED on success if waited, and -errno
- * on error.
- */
-static int vfio_find_dma_valid(struct vfio_iommu *iommu, dma_addr_t start,
-			       size_t size, struct vfio_dma **dma_p)
-{
-	int ret = 0;
-
-	do {
-		*dma_p = vfio_find_dma(iommu, start, size);
-		if (!*dma_p)
-			return -EINVAL;
-		else if (!(*dma_p)->vaddr_invalid)
-			return ret;
-		else
-			ret = vfio_wait(iommu);
-	} while (ret == WAITED);
-
-	return ret;
-}
-
-/*
- * Wait for all vaddr in the dma_list to become valid.  iommu lock is dropped
- * if the task waits, but is re-locked on return.  Return 0 on success with no
- * waiting, WAITED on success if waited, and -errno on error.
- */
-static int vfio_wait_all_valid(struct vfio_iommu *iommu)
-{
-	int ret = 0;
-
-	while (iommu->vaddr_invalid_count && ret >= 0)
-		ret = vfio_wait(iommu);
-
-	return ret;
-}
-
 /*
  * Attempt to pin pages.  We really don't want to track all the pfns and
  * the iommu can only map chunks of consecutive pfns anyway, so get the
@@ -851,7 +793,6 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
 	unsigned long remote_vaddr;
 	struct vfio_dma *dma;
 	bool do_accounting;
-	dma_addr_t iova;
 
 	if (!iommu || !pages)
 		return -EINVAL;
@@ -864,22 +805,6 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
 
 	mutex_lock(&iommu->lock);
 
-	/*
-	 * Wait for all necessary vaddr's to be valid so they can be used in
-	 * the main loop without dropping the lock, to avoid racing vs unmap.
-	 */
-again:
-	if (iommu->vaddr_invalid_count) {
-		for (i = 0; i < npage; i++) {
-			iova = user_iova + PAGE_SIZE * i;
-			ret = vfio_find_dma_valid(iommu, iova, PAGE_SIZE, &dma);
-			if (ret < 0)
-				goto pin_done;
-			if (ret == WAITED)
-				goto again;
-		}
-	}
-
 	/* Fail if no dma_umap notifier is registered */
 	if (list_empty(&iommu->device_list)) {
 		ret = -EINVAL;
@@ -895,6 +820,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
 
 	for (i = 0; i < npage; i++) {
 		unsigned long phys_pfn;
+		dma_addr_t iova;
 		struct vfio_pfn *vpfn;
 
 		iova = user_iova + PAGE_SIZE * i;
@@ -1181,10 +1107,8 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
 	put_task_struct(dma->task);
 	mmdrop(dma->mm);
 	vfio_dma_bitmap_free(dma);
-	if (dma->vaddr_invalid) {
+	if (dma->vaddr_invalid)
 		iommu->vaddr_invalid_count--;
-		wake_up_all(&iommu->vaddr_wait);
-	}
 	kfree(dma);
 	iommu->dma_avail++;
 }
@@ -1639,7 +1563,6 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 				dma->vaddr_invalid = false;
 				iommu->vaddr_invalid_count--;
 			}
-			wake_up_all(&iommu->vaddr_wait);
 		}
 		goto out_unlock;
 	} else if (dma) {
@@ -1730,10 +1653,6 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
 	unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
 	int ret;
 
-	ret = vfio_wait_all_valid(iommu);
-	if (ret < 0)
-		return ret;
-
 	/* Arbitrarily pick the first domain in the list for lookups */
 	if (!list_empty(&iommu->domain_list))
 		d = list_first_entry(&iommu->domain_list,
@@ -2623,7 +2542,6 @@ static void *vfio_iommu_type1_open(unsigned long arg)
 	mutex_init(&iommu->lock);
 	mutex_init(&iommu->device_list_lock);
 	INIT_LIST_HEAD(&iommu->device_list);
-	init_waitqueue_head(&iommu->vaddr_wait);
 	iommu->pgsize_bitmap = PAGE_MASK;
 	INIT_LIST_HEAD(&iommu->emulated_iommu_groups);
 
@@ -3120,15 +3038,14 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
 	struct vfio_dma *dma;
 	bool kthread = current->mm == NULL;
 	size_t offset;
-	int ret;
 
 	WARN_ON(iommu->vaddr_invalid_count);
 
 	*copied = 0;
 
-	ret = vfio_find_dma_valid(iommu, user_iova, 1, &dma);
-	if (ret < 0)
-		return ret;
+	dma = vfio_find_dma(iommu, user_iova, 1);
+	if (!dma)
+		return -EINVAL;
 
 	if ((write && !(dma->prot & IOMMU_WRITE)) ||
 			!(dma->prot & IOMMU_READ))
@@ -3230,7 +3147,6 @@ static void vfio_iommu_type1_notify(void *iommu_data,
 	mutex_lock(&iommu->lock);
 	iommu->container_open = false;
 	mutex_unlock(&iommu->lock);
-	wake_up_all(&iommu->vaddr_wait);
 }
 
 static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = {
-- 
1.8.3.1

[PATCH V2 4/5] vfio/type1: revert "implement notify callback"

[Steve Sistare]

This is dead code.  Revert it.
  commit 487ace134053 ("vfio/type1: implement notify callback")

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 drivers/vfio/vfio_iommu_type1.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index d4f961d..2fc0e34 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -75,7 +75,6 @@ struct vfio_iommu {
 	bool			v2;
 	bool			nesting;
 	bool			dirty_page_tracking;
-	bool			container_open;
 	struct list_head	emulated_iommu_groups;
 };
 
@@ -2538,7 +2537,6 @@ static void *vfio_iommu_type1_open(unsigned long arg)
 	INIT_LIST_HEAD(&iommu->iova_list);
 	iommu->dma_list = RB_ROOT;
 	iommu->dma_avail = dma_entry_limit;
-	iommu->container_open = true;
 	mutex_init(&iommu->lock);
 	mutex_init(&iommu->device_list_lock);
 	INIT_LIST_HEAD(&iommu->device_list);
@@ -3137,18 +3135,6 @@ static int vfio_iommu_type1_dma_rw(void *iommu_data, dma_addr_t user_iova,
 	return domain;
 }
 
-static void vfio_iommu_type1_notify(void *iommu_data,
-				    enum vfio_iommu_notify_type event)
-{
-	struct vfio_iommu *iommu = iommu_data;
-
-	if (event != VFIO_IOMMU_CONTAINER_CLOSE)
-		return;
-	mutex_lock(&iommu->lock);
-	iommu->container_open = false;
-	mutex_unlock(&iommu->lock);
-}
-
 static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = {
 	.name			= "vfio-iommu-type1",
 	.owner			= THIS_MODULE,
@@ -3163,7 +3149,6 @@ static void vfio_iommu_type1_notify(void *iommu_data,
 	.unregister_device	= vfio_iommu_type1_unregister_device,
 	.dma_rw			= vfio_iommu_type1_dma_rw,
 	.group_iommu_domain	= vfio_iommu_type1_group_iommu_domain,
-	.notify			= vfio_iommu_type1_notify,
 };
 
 static int __init vfio_iommu_type1_init(void)
-- 
1.8.3.1

[PATCH V2 5/5] vfio: revert "iommu driver notify callback"

[Steve Sistare]

Revert this dead code:
  commit ec5e32940cc9 ("vfio: iommu driver notify callback")

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 drivers/vfio/container.c | 5 -----
 drivers/vfio/vfio.h      | 7 -------
 2 files changed, 12 deletions(-)

diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
index d74164a..5bfd10d 100644
--- a/drivers/vfio/container.c
+++ b/drivers/vfio/container.c
@@ -382,11 +382,6 @@ static int vfio_fops_open(struct inode *inode, struct file *filep)
 static int vfio_fops_release(struct inode *inode, struct file *filep)
 {
 	struct vfio_container *container = filep->private_data;
-	struct vfio_iommu_driver *driver = container->iommu_driver;
-
-	if (driver && driver->ops->notify)
-		driver->ops->notify(container->iommu_data,
-				    VFIO_IOMMU_CONTAINER_CLOSE);
 
 	filep->private_data = NULL;
 
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index bcad54b..8a439c6 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -62,11 +62,6 @@ struct vfio_group {
 	struct blocking_notifier_head	notifier;
 };
 
-/* events for the backend driver notify callback */
-enum vfio_iommu_notify_type {
-	VFIO_IOMMU_CONTAINER_CLOSE = 0,
-};
-
 /**
  * struct vfio_iommu_driver_ops - VFIO IOMMU driver callbacks
  */
@@ -97,8 +92,6 @@ struct vfio_iommu_driver_ops {
 				  void *data, size_t count, bool write);
 	struct iommu_domain *(*group_iommu_domain)(void *iommu_data,
 						   struct iommu_group *group);
-	void		(*notify)(void *iommu_data,
-				  enum vfio_iommu_notify_type event);
 };
 
 struct vfio_iommu_driver {
-- 
1.8.3.1

Re: [PATCH V2 1/5] vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR

[Alex Williamson]

On Tue, 13 Dec 2022 11:40:55 -0800
Steve Sistare <steven.sistare@oracle.com> wrote:

> Disable the VFIO_UPDATE_VADDR capability if mediated devices are present.
> Their kernel threads could be blocked indefinitely by a misbehaving
> userland while trying to pin/unpin pages while vaddrs are being updated.
> 
> Do not allow groups to be added to the container while vaddr's are invalid,
> so we never need to block user threads from pinning, and can delete the
> vaddr-waiting code in a subsequent patch.
> 


Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")


> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 31 ++++++++++++++++++++++++++++++-
>  include/uapi/linux/vfio.h       | 15 +++++++++------
>  2 files changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 23c24fe..80bdb4d 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -859,6 +859,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>  	if (!iommu->v2)
>  		return -EACCES;
>  
> +	WARN_ON(iommu->vaddr_invalid_count);
> +

I'd expect this to abort and return -errno rather than simply trigger a
warning.

>  	mutex_lock(&iommu->lock);
>  
>  	/*
> @@ -976,6 +978,8 @@ static void vfio_iommu_type1_unpin_pages(void *iommu_data,
>  
>  	mutex_lock(&iommu->lock);
>  
> +	WARN_ON(iommu->vaddr_invalid_count);
> +

This should never happen or else I'd suggest this also make an early
exit.

>  	do_accounting = list_empty(&iommu->domain_list);
>  	for (i = 0; i < npage; i++) {
>  		dma_addr_t iova = user_iova + PAGE_SIZE * i;
> @@ -1343,6 +1347,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>  
>  	mutex_lock(&iommu->lock);
>  
> +	/* Cannot update vaddr if mdev is present. */
> +	if (invalidate_vaddr && !list_empty(&iommu->emulated_iommu_groups))
> +		goto unlock;

A different errno here to reflect that the container state is the issue
might be appropriate here.

> +
>  	pgshift = __ffs(iommu->pgsize_bitmap);
>  	pgsize = (size_t)1 << pgshift;
>  
> @@ -2189,6 +2197,10 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  
>  	mutex_lock(&iommu->lock);
>  
> +	/* Attach could require pinning, so disallow while vaddr is invalid. */
> +	if (iommu->vaddr_invalid_count)
> +		goto out_unlock;
> +
>  	/* Check for duplicates */
>  	if (vfio_iommu_find_iommu_group(iommu, iommu_group))
>  		goto out_unlock;
> @@ -2660,6 +2672,16 @@ static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
>  	return ret;
>  }
>  
> +static int vfio_iommu_has_emulated(struct vfio_iommu *iommu)
> +{
> +	int ret;
> +
> +	mutex_lock(&iommu->lock);
> +	ret = !list_empty(&iommu->emulated_iommu_groups);
> +	mutex_unlock(&iommu->lock);
> +	return ret;
> +}
> +
>  static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>  					    unsigned long arg)
>  {
> @@ -2668,8 +2690,13 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>  	case VFIO_TYPE1v2_IOMMU:
>  	case VFIO_TYPE1_NESTING_IOMMU:
>  	case VFIO_UNMAP_ALL:
> -	case VFIO_UPDATE_VADDR:
>  		return 1;
> +	case VFIO_UPDATE_VADDR:
> +		/*
> +		 * Disable this feature if mdevs are present.  They cannot
> +		 * safely pin/unpin while vaddrs are being updated.
> +		 */
> +		return iommu && !vfio_iommu_has_emulated(iommu);
>  	case VFIO_DMA_CC_IOMMU:
>  		if (!iommu)
>  			return 0;
> @@ -3080,6 +3107,8 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
>  	size_t offset;
>  	int ret;
>  
> +	WARN_ON(iommu->vaddr_invalid_count);
> +

Same as pinning, this should trigger -errno.  Thanks,

Alex

>  	*copied = 0;
>  
>  	ret = vfio_find_dma_valid(iommu, user_iova, 1, &dma);
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index d7d8e09..4e8d344 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -49,7 +49,11 @@
>  /* Supports VFIO_DMA_UNMAP_FLAG_ALL */
>  #define VFIO_UNMAP_ALL			9
>  
> -/* Supports the vaddr flag for DMA map and unmap */
> +/*
> + * Supports the vaddr flag for DMA map and unmap.  Not supported for mediated
> + * devices, so this capability is subject to change as groups are added or
> + * removed.
> + */
>  #define VFIO_UPDATE_VADDR		10
>  
>  /*
> @@ -1215,8 +1219,7 @@ struct vfio_iommu_type1_info_dma_avail {
>   * Map process virtual addresses to IO virtual addresses using the
>   * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
>   *
> - * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova, and
> - * unblock translation of host virtual addresses in the iova range.  The vaddr
> + * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova. The vaddr
>   * must have previously been invalidated with VFIO_DMA_UNMAP_FLAG_VADDR.  To
>   * maintain memory consistency within the user application, the updated vaddr
>   * must address the same memory object as originally mapped.  Failure to do so
> @@ -1267,9 +1270,9 @@ struct vfio_bitmap {
>   * must be 0.  This cannot be combined with the get-dirty-bitmap flag.
>   *
>   * If flags & VFIO_DMA_UNMAP_FLAG_VADDR, do not unmap, but invalidate host
> - * virtual addresses in the iova range.  Tasks that attempt to translate an
> - * iova's vaddr will block.  DMA to already-mapped pages continues.  This
> - * cannot be combined with the get-dirty-bitmap flag.
> + * virtual addresses in the iova range.  DMA to already-mapped pages continues.
> + * Groups may not be added to the container while any addresses are invalid.
> + * This cannot be combined with the get-dirty-bitmap flag.
>   */
>  struct vfio_iommu_type1_dma_unmap {
>  	__u32	argsz;

Re: [PATCH V2 2/5] vfio/type1: prevent locked_vm underflow

[Alex Williamson]

On Tue, 13 Dec 2022 11:40:56 -0800
Steve Sistare <steven.sistare@oracle.com> wrote:

> When a vfio container is preserved across exec using the VFIO_UPDATE_VADDR
> interfaces, locked_vm of the new mm becomes 0.  If the user later unmaps a
> dma mapping, locked_vm underflows to a large unsigned value, and a
> subsequent dma map request fails with ENOMEM in __account_locked_vm.
> 
> To avoid underflow, do not decrement locked_vm during unmap if the
> dma's mm has changed.  To restore the correct locked_vm count, when
> VFIO_DMA_MAP_FLAG_VADDR is used and the dma's mm has changed, add
> the mapping's pinned page count to the new mm->locked_vm, subject
> to the rlimit.  Now that mediated devices are excluded when using
> VFIO_UPDATE_VADDR, the amount of pinned memory equals the size of
> the mapping.
> 


Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")


> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 80bdb4d..35a1a52 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -100,6 +100,7 @@ struct vfio_dma {
>  	struct task_struct	*task;
>  	struct rb_root		pfn_list;	/* Ex-user pinned pfn list */
>  	unsigned long		*bitmap;
> +	struct mm_struct	*mm;
>  };
>  
>  struct vfio_batch {
> @@ -1165,7 +1166,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
>  					    &iotlb_gather);
>  	}
>  
> -	if (do_accounting) {
> +	if (do_accounting && current->mm == dma->mm) {


This seems incompatible with ffed0518d871 ("vfio: remove useless
judgement") where we no longer assume that the unmap mm is the same as
the mapping mm.

Does this need to get_task_mm(dma->task) and compare that mm to dma->mm
to determine whether an exec w/o vaddr remapping has occurred?  That's
the only use case I can figure out where grabbing the mm for dma->mm
actually makes any sense at all.

>  		vfio_lock_acct(dma, -unlocked, true);
>  		return 0;
>  	}
> @@ -1178,6 +1179,7 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
>  	vfio_unmap_unpin(iommu, dma, true);
>  	vfio_unlink_dma(iommu, dma);
>  	put_task_struct(dma->task);
> +	mmdrop(dma->mm);
>  	vfio_dma_bitmap_free(dma);
>  	if (dma->vaddr_invalid) {
>  		iommu->vaddr_invalid_count--;
> @@ -1623,9 +1625,20 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>  			   dma->size != size) {
>  			ret = -EINVAL;
>  		} else {
> -			dma->vaddr = vaddr;
> -			dma->vaddr_invalid = false;
> -			iommu->vaddr_invalid_count--;
> +			if (current->mm != dma->mm) {
> +				ret = vfio_lock_acct(dma, size >> PAGE_SHIFT,
> +						     0);
> +				if (!ret) {
> +					mmdrop(dma->mm);
> +					dma->mm = current->mm;
> +					mmgrab(dma->mm);
> +				}
> +			}
> +			if (!ret) {
> +				dma->vaddr = vaddr;
> +				dma->vaddr_invalid = false;
> +				iommu->vaddr_invalid_count--;
> +			}

Poor flow, shouldn't this be:

			if (current->mm != dma->mm) {
				ret = vfio_lock_acct(dma,
						     size >> PAGE_SHIFT, 0);
				if (ret)
					goto out_unlock;

				mmdrop(dma->mm);
				dma->mm = current->mm;
				mmgrab(dma->mm);
			}
			dma->vaddr = vaddr;
			dma->vaddr_invalid = false;
			iommu->vaddr_invalid_count--;


Thanks,
Alex

>  			wake_up_all(&iommu->vaddr_wait);
>  		}
>  		goto out_unlock;
> @@ -1683,6 +1696,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>  	get_task_struct(current->group_leader);
>  	dma->task = current->group_leader;
>  	dma->lock_cap = capable(CAP_IPC_LOCK);
> +	dma->mm = dma->task->mm;
> +	mmgrab(dma->mm);
>  
>  	dma->pfn_list = RB_ROOT;
>  

Re: [PATCH V2 1/5] vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR

[Steven Sistare]

On 12/13/2022 3:22 PM, Alex Williamson wrote:
> On Tue, 13 Dec 2022 11:40:55 -0800
> Steve Sistare <steven.sistare@oracle.com> wrote:
> 
>> Disable the VFIO_UPDATE_VADDR capability if mediated devices are present.
>> Their kernel threads could be blocked indefinitely by a misbehaving
>> userland while trying to pin/unpin pages while vaddrs are being updated.
>>
>> Do not allow groups to be added to the container while vaddr's are invalid,
>> so we never need to block user threads from pinning, and can delete the
>> vaddr-waiting code in a subsequent patch.
>>
> 
> 
> Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")

will do in both patches, slipped through the cracks.

>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 31 ++++++++++++++++++++++++++++++-
>>  include/uapi/linux/vfio.h       | 15 +++++++++------
>>  2 files changed, 39 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 23c24fe..80bdb4d 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -859,6 +859,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>  	if (!iommu->v2)
>>  		return -EACCES;
>>  
>> +	WARN_ON(iommu->vaddr_invalid_count);
>> +
> 
> I'd expect this to abort and return -errno rather than simply trigger a
> warning.

I added the three WARN_ON's at your request, but they should never fire because
we exclude mdevs.  I prefer not to bloat the code with additional checking that
never fires, and I would prefer to just delete WARN_ON, but its your call.

>>  	mutex_lock(&iommu->lock);
>>  
>>  	/*
>> @@ -976,6 +978,8 @@ static void vfio_iommu_type1_unpin_pages(void *iommu_data,
>>  
>>  	mutex_lock(&iommu->lock);
>>  
>> +	WARN_ON(iommu->vaddr_invalid_count);
>> +
> 
> This should never happen or else I'd suggest this also make an early
> exit.

I would like to delete the WARN_ON's entirely.

>>  	do_accounting = list_empty(&iommu->domain_list);
>>  	for (i = 0; i < npage; i++) {
>>  		dma_addr_t iova = user_iova + PAGE_SIZE * i;
>> @@ -1343,6 +1347,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>>  
>>  	mutex_lock(&iommu->lock);
>>  
>> +	/* Cannot update vaddr if mdev is present. */
>> +	if (invalidate_vaddr && !list_empty(&iommu->emulated_iommu_groups))
>> +		goto unlock;
> 
> A different errno here to reflect that the container state is the issue
> might be appropriate here.

Will do.

>> +
>>  	pgshift = __ffs(iommu->pgsize_bitmap);
>>  	pgsize = (size_t)1 << pgshift;
>>  
>> @@ -2189,6 +2197,10 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>  
>>  	mutex_lock(&iommu->lock);
>>  
>> +	/* Attach could require pinning, so disallow while vaddr is invalid. */
>> +	if (iommu->vaddr_invalid_count)
>> +		goto out_unlock;
>> +
>>  	/* Check for duplicates */
>>  	if (vfio_iommu_find_iommu_group(iommu, iommu_group))
>>  		goto out_unlock;
>> @@ -2660,6 +2672,16 @@ static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
>>  	return ret;
>>  }
>>  
>> +static int vfio_iommu_has_emulated(struct vfio_iommu *iommu)
>> +{
>> +	int ret;
>> +
>> +	mutex_lock(&iommu->lock);
>> +	ret = !list_empty(&iommu->emulated_iommu_groups);
>> +	mutex_unlock(&iommu->lock);
>> +	return ret;
>> +}
>> +
>>  static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>>  					    unsigned long arg)
>>  {
>> @@ -2668,8 +2690,13 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>>  	case VFIO_TYPE1v2_IOMMU:
>>  	case VFIO_TYPE1_NESTING_IOMMU:
>>  	case VFIO_UNMAP_ALL:
>> -	case VFIO_UPDATE_VADDR:
>>  		return 1;
>> +	case VFIO_UPDATE_VADDR:
>> +		/*
>> +		 * Disable this feature if mdevs are present.  They cannot
>> +		 * safely pin/unpin while vaddrs are being updated.
>> +		 */
>> +		return iommu && !vfio_iommu_has_emulated(iommu);
>>  	case VFIO_DMA_CC_IOMMU:
>>  		if (!iommu)
>>  			return 0;
>> @@ -3080,6 +3107,8 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
>>  	size_t offset;
>>  	int ret;
>>  
>> +	WARN_ON(iommu->vaddr_invalid_count);
>> +
> 
> Same as pinning, this should trigger -errno.  Thanks,

Another one that should never happen.  

- Steve

>>  	*copied = 0;
>>  
>>  	ret = vfio_find_dma_valid(iommu, user_iova, 1, &dma);
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index d7d8e09..4e8d344 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -49,7 +49,11 @@
>>  /* Supports VFIO_DMA_UNMAP_FLAG_ALL */
>>  #define VFIO_UNMAP_ALL			9
>>  
>> -/* Supports the vaddr flag for DMA map and unmap */
>> +/*
>> + * Supports the vaddr flag for DMA map and unmap.  Not supported for mediated
>> + * devices, so this capability is subject to change as groups are added or
>> + * removed.
>> + */
>>  #define VFIO_UPDATE_VADDR		10
>>  
>>  /*
>> @@ -1215,8 +1219,7 @@ struct vfio_iommu_type1_info_dma_avail {
>>   * Map process virtual addresses to IO virtual addresses using the
>>   * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
>>   *
>> - * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova, and
>> - * unblock translation of host virtual addresses in the iova range.  The vaddr
>> + * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova. The vaddr
>>   * must have previously been invalidated with VFIO_DMA_UNMAP_FLAG_VADDR.  To
>>   * maintain memory consistency within the user application, the updated vaddr
>>   * must address the same memory object as originally mapped.  Failure to do so
>> @@ -1267,9 +1270,9 @@ struct vfio_bitmap {
>>   * must be 0.  This cannot be combined with the get-dirty-bitmap flag.
>>   *
>>   * If flags & VFIO_DMA_UNMAP_FLAG_VADDR, do not unmap, but invalidate host
>> - * virtual addresses in the iova range.  Tasks that attempt to translate an
>> - * iova's vaddr will block.  DMA to already-mapped pages continues.  This
>> - * cannot be combined with the get-dirty-bitmap flag.
>> + * virtual addresses in the iova range.  DMA to already-mapped pages continues.
>> + * Groups may not be added to the container while any addresses are invalid.
>> + * This cannot be combined with the get-dirty-bitmap flag.
>>   */
>>  struct vfio_iommu_type1_dma_unmap {
>>  	__u32	argsz;
> 

Re: [PATCH V2 1/5] vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR

[Alex Williamson]

On Tue, 13 Dec 2022 15:37:45 -0500
Steven Sistare <steven.sistare@oracle.com> wrote:

> On 12/13/2022 3:22 PM, Alex Williamson wrote:
> > On Tue, 13 Dec 2022 11:40:55 -0800
> > Steve Sistare <steven.sistare@oracle.com> wrote:
> >   
> >> Disable the VFIO_UPDATE_VADDR capability if mediated devices are present.
> >> Their kernel threads could be blocked indefinitely by a misbehaving
> >> userland while trying to pin/unpin pages while vaddrs are being updated.
> >>
> >> Do not allow groups to be added to the container while vaddr's are invalid,
> >> so we never need to block user threads from pinning, and can delete the
> >> vaddr-waiting code in a subsequent patch.
> >>  
> > 
> > 
> > Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")  
> 
> will do in both patches, slipped through the cracks.
> 
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >> ---
> >>  drivers/vfio/vfio_iommu_type1.c | 31 ++++++++++++++++++++++++++++++-
> >>  include/uapi/linux/vfio.h       | 15 +++++++++------
> >>  2 files changed, 39 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 23c24fe..80bdb4d 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -859,6 +859,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> >>  	if (!iommu->v2)
> >>  		return -EACCES;
> >>  
> >> +	WARN_ON(iommu->vaddr_invalid_count);
> >> +  
> > 
> > I'd expect this to abort and return -errno rather than simply trigger a
> > warning.  
> 
> I added the three WARN_ON's at your request, but they should never fire because
> we exclude mdevs.  I prefer not to bloat the code with additional checking that
> never fires, and I would prefer to just delete WARN_ON, but its your call.


Other than convention, what prevents non-mdev code from using this
interface?  I agree that making vaddr unmapping and emulated IOMMU
devices mutually exclusive *should* be enough, but I have reason to
suspect there could be out-of-tree non-mdev drivers using these
interfaces.  Thanks,

Alex

 
> >>  	mutex_lock(&iommu->lock);
> >>  
> >>  	/*
> >> @@ -976,6 +978,8 @@ static void vfio_iommu_type1_unpin_pages(void *iommu_data,
> >>  
> >>  	mutex_lock(&iommu->lock);
> >>  
> >> +	WARN_ON(iommu->vaddr_invalid_count);
> >> +  
> > 
> > This should never happen or else I'd suggest this also make an early
> > exit.  
> 
> I would like to delete the WARN_ON's entirely.
> 
> >>  	do_accounting = list_empty(&iommu->domain_list);
> >>  	for (i = 0; i < npage; i++) {
> >>  		dma_addr_t iova = user_iova + PAGE_SIZE * i;
> >> @@ -1343,6 +1347,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >>  
> >>  	mutex_lock(&iommu->lock);
> >>  
> >> +	/* Cannot update vaddr if mdev is present. */
> >> +	if (invalidate_vaddr && !list_empty(&iommu->emulated_iommu_groups))
> >> +		goto unlock;  
> > 
> > A different errno here to reflect that the container state is the issue
> > might be appropriate here.  
> 
> Will do.
> 
> >> +
> >>  	pgshift = __ffs(iommu->pgsize_bitmap);
> >>  	pgsize = (size_t)1 << pgshift;
> >>  
> >> @@ -2189,6 +2197,10 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> >>  
> >>  	mutex_lock(&iommu->lock);
> >>  
> >> +	/* Attach could require pinning, so disallow while vaddr is invalid. */
> >> +	if (iommu->vaddr_invalid_count)
> >> +		goto out_unlock;
> >> +
> >>  	/* Check for duplicates */
> >>  	if (vfio_iommu_find_iommu_group(iommu, iommu_group))
> >>  		goto out_unlock;
> >> @@ -2660,6 +2672,16 @@ static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
> >>  	return ret;
> >>  }
> >>  
> >> +static int vfio_iommu_has_emulated(struct vfio_iommu *iommu)
> >> +{
> >> +	int ret;
> >> +
> >> +	mutex_lock(&iommu->lock);
> >> +	ret = !list_empty(&iommu->emulated_iommu_groups);
> >> +	mutex_unlock(&iommu->lock);
> >> +	return ret;
> >> +}
> >> +
> >>  static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
> >>  					    unsigned long arg)
> >>  {
> >> @@ -2668,8 +2690,13 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
> >>  	case VFIO_TYPE1v2_IOMMU:
> >>  	case VFIO_TYPE1_NESTING_IOMMU:
> >>  	case VFIO_UNMAP_ALL:
> >> -	case VFIO_UPDATE_VADDR:
> >>  		return 1;
> >> +	case VFIO_UPDATE_VADDR:
> >> +		/*
> >> +		 * Disable this feature if mdevs are present.  They cannot
> >> +		 * safely pin/unpin while vaddrs are being updated.
> >> +		 */
> >> +		return iommu && !vfio_iommu_has_emulated(iommu);
> >>  	case VFIO_DMA_CC_IOMMU:
> >>  		if (!iommu)
> >>  			return 0;
> >> @@ -3080,6 +3107,8 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
> >>  	size_t offset;
> >>  	int ret;
> >>  
> >> +	WARN_ON(iommu->vaddr_invalid_count);
> >> +  
> > 
> > Same as pinning, this should trigger -errno.  Thanks,  
> 
> Another one that should never happen.  
> 
> - Steve
> 
> >>  	*copied = 0;
> >>  
> >>  	ret = vfio_find_dma_valid(iommu, user_iova, 1, &dma);
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index d7d8e09..4e8d344 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -49,7 +49,11 @@
> >>  /* Supports VFIO_DMA_UNMAP_FLAG_ALL */
> >>  #define VFIO_UNMAP_ALL			9
> >>  
> >> -/* Supports the vaddr flag for DMA map and unmap */
> >> +/*
> >> + * Supports the vaddr flag for DMA map and unmap.  Not supported for mediated
> >> + * devices, so this capability is subject to change as groups are added or
> >> + * removed.
> >> + */
> >>  #define VFIO_UPDATE_VADDR		10
> >>  
> >>  /*
> >> @@ -1215,8 +1219,7 @@ struct vfio_iommu_type1_info_dma_avail {
> >>   * Map process virtual addresses to IO virtual addresses using the
> >>   * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
> >>   *
> >> - * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova, and
> >> - * unblock translation of host virtual addresses in the iova range.  The vaddr
> >> + * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova. The vaddr
> >>   * must have previously been invalidated with VFIO_DMA_UNMAP_FLAG_VADDR.  To
> >>   * maintain memory consistency within the user application, the updated vaddr
> >>   * must address the same memory object as originally mapped.  Failure to do so
> >> @@ -1267,9 +1270,9 @@ struct vfio_bitmap {
> >>   * must be 0.  This cannot be combined with the get-dirty-bitmap flag.
> >>   *
> >>   * If flags & VFIO_DMA_UNMAP_FLAG_VADDR, do not unmap, but invalidate host
> >> - * virtual addresses in the iova range.  Tasks that attempt to translate an
> >> - * iova's vaddr will block.  DMA to already-mapped pages continues.  This
> >> - * cannot be combined with the get-dirty-bitmap flag.
> >> + * virtual addresses in the iova range.  DMA to already-mapped pages continues.
> >> + * Groups may not be added to the container while any addresses are invalid.
> >> + * This cannot be combined with the get-dirty-bitmap flag.
> >>   */
> >>  struct vfio_iommu_type1_dma_unmap {
> >>  	__u32	argsz;  
> >   
> 

Re: [PATCH V2 2/5] vfio/type1: prevent locked_vm underflow

[Steven Sistare]

On 12/13/2022 3:23 PM, Alex Williamson wrote:
> On Tue, 13 Dec 2022 11:40:56 -0800
> Steve Sistare <steven.sistare@oracle.com> wrote:
> 
>> When a vfio container is preserved across exec using the VFIO_UPDATE_VADDR
>> interfaces, locked_vm of the new mm becomes 0.  If the user later unmaps a
>> dma mapping, locked_vm underflows to a large unsigned value, and a
>> subsequent dma map request fails with ENOMEM in __account_locked_vm.
>>
>> To avoid underflow, do not decrement locked_vm during unmap if the
>> dma's mm has changed.  To restore the correct locked_vm count, when
>> VFIO_DMA_MAP_FLAG_VADDR is used and the dma's mm has changed, add
>> the mapping's pinned page count to the new mm->locked_vm, subject
>> to the rlimit.  Now that mediated devices are excluded when using
>> VFIO_UPDATE_VADDR, the amount of pinned memory equals the size of
>> the mapping.
> 
> Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")
> 
> 
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 23 +++++++++++++++++++----
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 80bdb4d..35a1a52 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -100,6 +100,7 @@ struct vfio_dma {
>>  	struct task_struct	*task;
>>  	struct rb_root		pfn_list;	/* Ex-user pinned pfn list */
>>  	unsigned long		*bitmap;
>> +	struct mm_struct	*mm;
>>  };
>>  
>>  struct vfio_batch {
>> @@ -1165,7 +1166,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
>>  					    &iotlb_gather);
>>  	}
>>  
>> -	if (do_accounting) {
>> +	if (do_accounting && current->mm == dma->mm) {
> 
> 
> This seems incompatible with ffed0518d871 ("vfio: remove useless
> judgement") where we no longer assume that the unmap mm is the same as
> the mapping mm.

They are compatible.  My fix allows another task to unmap, but only decreases
locked_vm if the current mm matches the original mm that locked it.  And the
"original" mm is updated by MAP_FLAG_VADDR.

> Does this need to get_task_mm(dma->task) and compare that mm to dma->mm
> to determine whether an exec w/o vaddr remapping has occurred?  That's
> the only use case I can figure out where grabbing the mm for dma->mm
> actually makes any sense at all.

The mm grab does detect an exec.  Before exec, at map time, we get task and grab
its mm.  During exec, task gets a new mm.  The old mm becomes defunct, but we
still hold it and can examine its pointer address.

The new code does not require that current == dma->task.

>>  		vfio_lock_acct(dma, -unlocked, true);
>>  		return 0;
>>  	}
>> @@ -1178,6 +1179,7 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
>>  	vfio_unmap_unpin(iommu, dma, true);
>>  	vfio_unlink_dma(iommu, dma);
>>  	put_task_struct(dma->task);
>> +	mmdrop(dma->mm);
>>  	vfio_dma_bitmap_free(dma);
>>  	if (dma->vaddr_invalid) {
>>  		iommu->vaddr_invalid_count--;
>> @@ -1623,9 +1625,20 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>>  			   dma->size != size) {
>>  			ret = -EINVAL;
>>  		} else {
>> -			dma->vaddr = vaddr;
>> -			dma->vaddr_invalid = false;
>> -			iommu->vaddr_invalid_count--;
>> +			if (current->mm != dma->mm) {
>> +				ret = vfio_lock_acct(dma, size >> PAGE_SHIFT,
>> +						     0);
>> +				if (!ret) {
>> +					mmdrop(dma->mm);
>> +					dma->mm = current->mm;
>> +					mmgrab(dma->mm);
>> +				}
>> +			}
>> +			if (!ret) {
>> +				dma->vaddr = vaddr;
>> +				dma->vaddr_invalid = false;
>> +				iommu->vaddr_invalid_count--;
>> +			}
> 
> Poor flow, shouldn't this be:
> 
> 			if (current->mm != dma->mm) {
> 				ret = vfio_lock_acct(dma,
> 						     size >> PAGE_SHIFT, 0);
> 				if (ret)
> 					goto out_unlock;
> 
> 				mmdrop(dma->mm);
> 				dma->mm = current->mm;
> 				mmgrab(dma->mm);
> 			}
> 			dma->vaddr = vaddr;
> 			dma->vaddr_invalid = false;
> 			iommu->vaddr_invalid_count--;

Better, will do, thanks.

- Steve

>>  			wake_up_all(&iommu->vaddr_wait);
>>  		}
>>  		goto out_unlock;
>> @@ -1683,6 +1696,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>>  	get_task_struct(current->group_leader);
>>  	dma->task = current->group_leader;
>>  	dma->lock_cap = capable(CAP_IPC_LOCK);
>> +	dma->mm = dma->task->mm;
>> +	mmgrab(dma->mm);
>>  
>>  	dma->pfn_list = RB_ROOT;
>>  
> 

Re: [PATCH V2 1/5] vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR

[Steven Sistare]

On 12/13/2022 3:59 PM, Alex Williamson wrote:
> On Tue, 13 Dec 2022 15:37:45 -0500
> Steven Sistare <steven.sistare@oracle.com> wrote:
> 
>> On 12/13/2022 3:22 PM, Alex Williamson wrote:
>>> On Tue, 13 Dec 2022 11:40:55 -0800
>>> Steve Sistare <steven.sistare@oracle.com> wrote:
>>>   
>>>> Disable the VFIO_UPDATE_VADDR capability if mediated devices are present.
>>>> Their kernel threads could be blocked indefinitely by a misbehaving
>>>> userland while trying to pin/unpin pages while vaddrs are being updated.
>>>>
>>>> Do not allow groups to be added to the container while vaddr's are invalid,
>>>> so we never need to block user threads from pinning, and can delete the
>>>> vaddr-waiting code in a subsequent patch.
>>>>  
>>>
>>>
>>> Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")  
>>
>> will do in both patches, slipped through the cracks.
>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>>  drivers/vfio/vfio_iommu_type1.c | 31 ++++++++++++++++++++++++++++++-
>>>>  include/uapi/linux/vfio.h       | 15 +++++++++------
>>>>  2 files changed, 39 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>>>> index 23c24fe..80bdb4d 100644
>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>> @@ -859,6 +859,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>>>  	if (!iommu->v2)
>>>>  		return -EACCES;
>>>>  
>>>> +	WARN_ON(iommu->vaddr_invalid_count);
>>>> +  
>>>
>>> I'd expect this to abort and return -errno rather than simply trigger a
>>> warning.  
>>
>> I added the three WARN_ON's at your request, but they should never fire because
>> we exclude mdevs.  I prefer not to bloat the code with additional checking that
>> never fires, and I would prefer to just delete WARN_ON, but its your call.
> 
> Other than convention, what prevents non-mdev code from using this
> interface?  I agree that making vaddr unmapping and emulated IOMMU
> devices mutually exclusive *should* be enough, but I have reason to
> suspect there could be out-of-tree non-mdev drivers using these
> interfaces.  Thanks,

OK, none of the exclusion checks will prevent such calls, even for mdevs.  I will 
delete the WARN_ON's and return an error code.

- Steve
  
>>>>  	mutex_lock(&iommu->lock);
>>>>  
>>>>  	/*
>>>> @@ -976,6 +978,8 @@ static void vfio_iommu_type1_unpin_pages(void *iommu_data,
>>>>  
>>>>  	mutex_lock(&iommu->lock);
>>>>  
>>>> +	WARN_ON(iommu->vaddr_invalid_count);
>>>> +  
>>>
>>> This should never happen or else I'd suggest this also make an early
>>> exit.  
>>
>> I would like to delete the WARN_ON's entirely.
>>
>>>>  	do_accounting = list_empty(&iommu->domain_list);
>>>>  	for (i = 0; i < npage; i++) {
>>>>  		dma_addr_t iova = user_iova + PAGE_SIZE * i;
>>>> @@ -1343,6 +1347,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>>>>  
>>>>  	mutex_lock(&iommu->lock);
>>>>  
>>>> +	/* Cannot update vaddr if mdev is present. */
>>>> +	if (invalidate_vaddr && !list_empty(&iommu->emulated_iommu_groups))
>>>> +		goto unlock;  
>>>
>>> A different errno here to reflect that the container state is the issue
>>> might be appropriate here.  
>>
>> Will do.
>>
>>>> +
>>>>  	pgshift = __ffs(iommu->pgsize_bitmap);
>>>>  	pgsize = (size_t)1 << pgshift;
>>>>  
>>>> @@ -2189,6 +2197,10 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>>>  
>>>>  	mutex_lock(&iommu->lock);
>>>>  
>>>> +	/* Attach could require pinning, so disallow while vaddr is invalid. */
>>>> +	if (iommu->vaddr_invalid_count)
>>>> +		goto out_unlock;
>>>> +
>>>>  	/* Check for duplicates */
>>>>  	if (vfio_iommu_find_iommu_group(iommu, iommu_group))
>>>>  		goto out_unlock;
>>>> @@ -2660,6 +2672,16 @@ static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +static int vfio_iommu_has_emulated(struct vfio_iommu *iommu)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	mutex_lock(&iommu->lock);
>>>> +	ret = !list_empty(&iommu->emulated_iommu_groups);
>>>> +	mutex_unlock(&iommu->lock);
>>>> +	return ret;
>>>> +}
>>>> +
>>>>  static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>>>>  					    unsigned long arg)
>>>>  {
>>>> @@ -2668,8 +2690,13 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>>>>  	case VFIO_TYPE1v2_IOMMU:
>>>>  	case VFIO_TYPE1_NESTING_IOMMU:
>>>>  	case VFIO_UNMAP_ALL:
>>>> -	case VFIO_UPDATE_VADDR:
>>>>  		return 1;
>>>> +	case VFIO_UPDATE_VADDR:
>>>> +		/*
>>>> +		 * Disable this feature if mdevs are present.  They cannot
>>>> +		 * safely pin/unpin while vaddrs are being updated.
>>>> +		 */
>>>> +		return iommu && !vfio_iommu_has_emulated(iommu);
>>>>  	case VFIO_DMA_CC_IOMMU:
>>>>  		if (!iommu)
>>>>  			return 0;
>>>> @@ -3080,6 +3107,8 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
>>>>  	size_t offset;
>>>>  	int ret;
>>>>  
>>>> +	WARN_ON(iommu->vaddr_invalid_count);
>>>> +  
>>>
>>> Same as pinning, this should trigger -errno.  Thanks,  
>>
>> Another one that should never happen.  
>>
>> - Steve
>>
>>>>  	*copied = 0;
>>>>  
>>>>  	ret = vfio_find_dma_valid(iommu, user_iova, 1, &dma);
>>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>>> index d7d8e09..4e8d344 100644
>>>> --- a/include/uapi/linux/vfio.h
>>>> +++ b/include/uapi/linux/vfio.h
>>>> @@ -49,7 +49,11 @@
>>>>  /* Supports VFIO_DMA_UNMAP_FLAG_ALL */
>>>>  #define VFIO_UNMAP_ALL			9
>>>>  
>>>> -/* Supports the vaddr flag for DMA map and unmap */
>>>> +/*
>>>> + * Supports the vaddr flag for DMA map and unmap.  Not supported for mediated
>>>> + * devices, so this capability is subject to change as groups are added or
>>>> + * removed.
>>>> + */
>>>>  #define VFIO_UPDATE_VADDR		10
>>>>  
>>>>  /*
>>>> @@ -1215,8 +1219,7 @@ struct vfio_iommu_type1_info_dma_avail {
>>>>   * Map process virtual addresses to IO virtual addresses using the
>>>>   * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
>>>>   *
>>>> - * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova, and
>>>> - * unblock translation of host virtual addresses in the iova range.  The vaddr
>>>> + * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova. The vaddr
>>>>   * must have previously been invalidated with VFIO_DMA_UNMAP_FLAG_VADDR.  To
>>>>   * maintain memory consistency within the user application, the updated vaddr
>>>>   * must address the same memory object as originally mapped.  Failure to do so
>>>> @@ -1267,9 +1270,9 @@ struct vfio_bitmap {
>>>>   * must be 0.  This cannot be combined with the get-dirty-bitmap flag.
>>>>   *
>>>>   * If flags & VFIO_DMA_UNMAP_FLAG_VADDR, do not unmap, but invalidate host
>>>> - * virtual addresses in the iova range.  Tasks that attempt to translate an
>>>> - * iova's vaddr will block.  DMA to already-mapped pages continues.  This
>>>> - * cannot be combined with the get-dirty-bitmap flag.
>>>> + * virtual addresses in the iova range.  DMA to already-mapped pages continues.
>>>> + * Groups may not be added to the container while any addresses are invalid.
>>>> + * This cannot be combined with the get-dirty-bitmap flag.
>>>>   */
>>>>  struct vfio_iommu_type1_dma_unmap {
>>>>  	__u32	argsz;  
>>>   
>>
> 

Re: [PATCH V2 1/5] vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR

[Alex Williamson]

On Tue, 13 Dec 2022 16:16:31 -0500
Steven Sistare <steven.sistare@oracle.com> wrote:

> On 12/13/2022 3:59 PM, Alex Williamson wrote:
> > On Tue, 13 Dec 2022 15:37:45 -0500
> > Steven Sistare <steven.sistare@oracle.com> wrote:
> >   
> >> On 12/13/2022 3:22 PM, Alex Williamson wrote:  
> >>> On Tue, 13 Dec 2022 11:40:55 -0800
> >>> Steve Sistare <steven.sistare@oracle.com> wrote:
> >>>     
> >>>> Disable the VFIO_UPDATE_VADDR capability if mediated devices are present.
> >>>> Their kernel threads could be blocked indefinitely by a misbehaving
> >>>> userland while trying to pin/unpin pages while vaddrs are being updated.
> >>>>
> >>>> Do not allow groups to be added to the container while vaddr's are invalid,
> >>>> so we never need to block user threads from pinning, and can delete the
> >>>> vaddr-waiting code in a subsequent patch.
> >>>>    
> >>>
> >>>
> >>> Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")    
> >>
> >> will do in both patches, slipped through the cracks.
> >>  
> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >>>> ---
> >>>>  drivers/vfio/vfio_iommu_type1.c | 31 ++++++++++++++++++++++++++++++-
> >>>>  include/uapi/linux/vfio.h       | 15 +++++++++------
> >>>>  2 files changed, 39 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >>>> index 23c24fe..80bdb4d 100644
> >>>> --- a/drivers/vfio/vfio_iommu_type1.c
> >>>> +++ b/drivers/vfio/vfio_iommu_type1.c
> >>>> @@ -859,6 +859,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> >>>>  	if (!iommu->v2)
> >>>>  		return -EACCES;
> >>>>  
> >>>> +	WARN_ON(iommu->vaddr_invalid_count);
> >>>> +    
> >>>
> >>> I'd expect this to abort and return -errno rather than simply trigger a
> >>> warning.    
> >>
> >> I added the three WARN_ON's at your request, but they should never fire because
> >> we exclude mdevs.  I prefer not to bloat the code with additional checking that
> >> never fires, and I would prefer to just delete WARN_ON, but its your call.  
> > 
> > Other than convention, what prevents non-mdev code from using this
> > interface?  I agree that making vaddr unmapping and emulated IOMMU
> > devices mutually exclusive *should* be enough, but I have reason to
> > suspect there could be out-of-tree non-mdev drivers using these
> > interfaces.  Thanks,  
> 
> OK, none of the exclusion checks will prevent such calls, even for mdevs.  I will 
> delete the WARN_ON's and return an error code.

The WARN_ONs are still appropriate.  We've specified via comment in
vfio_pin_pages() that only devices created with
vfio_register_emulated_iommu_dev() can use it, so if the mutual
exclusion we think should be present between vaddr and kernel threads
is broken, there's a driver bug that justifies a WARN_ON.  Thanks,

Alex
 
> >>>>  	mutex_lock(&iommu->lock);
> >>>>  
> >>>>  	/*
> >>>> @@ -976,6 +978,8 @@ static void vfio_iommu_type1_unpin_pages(void *iommu_data,
> >>>>  
> >>>>  	mutex_lock(&iommu->lock);
> >>>>  
> >>>> +	WARN_ON(iommu->vaddr_invalid_count);
> >>>> +    
> >>>
> >>> This should never happen or else I'd suggest this also make an early
> >>> exit.    
> >>
> >> I would like to delete the WARN_ON's entirely.
> >>  
> >>>>  	do_accounting = list_empty(&iommu->domain_list);
> >>>>  	for (i = 0; i < npage; i++) {
> >>>>  		dma_addr_t iova = user_iova + PAGE_SIZE * i;
> >>>> @@ -1343,6 +1347,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >>>>  
> >>>>  	mutex_lock(&iommu->lock);
> >>>>  
> >>>> +	/* Cannot update vaddr if mdev is present. */
> >>>> +	if (invalidate_vaddr && !list_empty(&iommu->emulated_iommu_groups))
> >>>> +		goto unlock;    
> >>>
> >>> A different errno here to reflect that the container state is the issue
> >>> might be appropriate here.    
> >>
> >> Will do.
> >>  
> >>>> +
> >>>>  	pgshift = __ffs(iommu->pgsize_bitmap);
> >>>>  	pgsize = (size_t)1 << pgshift;
> >>>>  
> >>>> @@ -2189,6 +2197,10 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> >>>>  
> >>>>  	mutex_lock(&iommu->lock);
> >>>>  
> >>>> +	/* Attach could require pinning, so disallow while vaddr is invalid. */
> >>>> +	if (iommu->vaddr_invalid_count)
> >>>> +		goto out_unlock;
> >>>> +
> >>>>  	/* Check for duplicates */
> >>>>  	if (vfio_iommu_find_iommu_group(iommu, iommu_group))
> >>>>  		goto out_unlock;
> >>>> @@ -2660,6 +2672,16 @@ static int vfio_domains_have_enforce_cache_coherency(struct vfio_iommu *iommu)
> >>>>  	return ret;
> >>>>  }
> >>>>  
> >>>> +static int vfio_iommu_has_emulated(struct vfio_iommu *iommu)
> >>>> +{
> >>>> +	int ret;
> >>>> +
> >>>> +	mutex_lock(&iommu->lock);
> >>>> +	ret = !list_empty(&iommu->emulated_iommu_groups);
> >>>> +	mutex_unlock(&iommu->lock);
> >>>> +	return ret;
> >>>> +}
> >>>> +
> >>>>  static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
> >>>>  					    unsigned long arg)
> >>>>  {
> >>>> @@ -2668,8 +2690,13 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
> >>>>  	case VFIO_TYPE1v2_IOMMU:
> >>>>  	case VFIO_TYPE1_NESTING_IOMMU:
> >>>>  	case VFIO_UNMAP_ALL:
> >>>> -	case VFIO_UPDATE_VADDR:
> >>>>  		return 1;
> >>>> +	case VFIO_UPDATE_VADDR:
> >>>> +		/*
> >>>> +		 * Disable this feature if mdevs are present.  They cannot
> >>>> +		 * safely pin/unpin while vaddrs are being updated.
> >>>> +		 */
> >>>> +		return iommu && !vfio_iommu_has_emulated(iommu);
> >>>>  	case VFIO_DMA_CC_IOMMU:
> >>>>  		if (!iommu)
> >>>>  			return 0;
> >>>> @@ -3080,6 +3107,8 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
> >>>>  	size_t offset;
> >>>>  	int ret;
> >>>>  
> >>>> +	WARN_ON(iommu->vaddr_invalid_count);
> >>>> +    
> >>>
> >>> Same as pinning, this should trigger -errno.  Thanks,    
> >>
> >> Another one that should never happen.  
> >>
> >> - Steve
> >>  
> >>>>  	*copied = 0;
> >>>>  
> >>>>  	ret = vfio_find_dma_valid(iommu, user_iova, 1, &dma);
> >>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >>>> index d7d8e09..4e8d344 100644
> >>>> --- a/include/uapi/linux/vfio.h
> >>>> +++ b/include/uapi/linux/vfio.h
> >>>> @@ -49,7 +49,11 @@
> >>>>  /* Supports VFIO_DMA_UNMAP_FLAG_ALL */
> >>>>  #define VFIO_UNMAP_ALL			9
> >>>>  
> >>>> -/* Supports the vaddr flag for DMA map and unmap */
> >>>> +/*
> >>>> + * Supports the vaddr flag for DMA map and unmap.  Not supported for mediated
> >>>> + * devices, so this capability is subject to change as groups are added or
> >>>> + * removed.
> >>>> + */
> >>>>  #define VFIO_UPDATE_VADDR		10
> >>>>  
> >>>>  /*
> >>>> @@ -1215,8 +1219,7 @@ struct vfio_iommu_type1_info_dma_avail {
> >>>>   * Map process virtual addresses to IO virtual addresses using the
> >>>>   * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
> >>>>   *
> >>>> - * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova, and
> >>>> - * unblock translation of host virtual addresses in the iova range.  The vaddr
> >>>> + * If flags & VFIO_DMA_MAP_FLAG_VADDR, update the base vaddr for iova. The vaddr
> >>>>   * must have previously been invalidated with VFIO_DMA_UNMAP_FLAG_VADDR.  To
> >>>>   * maintain memory consistency within the user application, the updated vaddr
> >>>>   * must address the same memory object as originally mapped.  Failure to do so
> >>>> @@ -1267,9 +1270,9 @@ struct vfio_bitmap {
> >>>>   * must be 0.  This cannot be combined with the get-dirty-bitmap flag.
> >>>>   *
> >>>>   * If flags & VFIO_DMA_UNMAP_FLAG_VADDR, do not unmap, but invalidate host
> >>>> - * virtual addresses in the iova range.  Tasks that attempt to translate an
> >>>> - * iova's vaddr will block.  DMA to already-mapped pages continues.  This
> >>>> - * cannot be combined with the get-dirty-bitmap flag.
> >>>> + * virtual addresses in the iova range.  DMA to already-mapped pages continues.
> >>>> + * Groups may not be added to the container while any addresses are invalid.
> >>>> + * This cannot be combined with the get-dirty-bitmap flag.
> >>>>   */
> >>>>  struct vfio_iommu_type1_dma_unmap {
> >>>>  	__u32	argsz;    
> >>>     
> >>  
> >   
> 

Re: [PATCH V2 2/5] vfio/type1: prevent locked_vm underflow

[Alex Williamson]

On Tue, 13 Dec 2022 16:01:21 -0500
Steven Sistare <steven.sistare@oracle.com> wrote:

> On 12/13/2022 3:23 PM, Alex Williamson wrote:
> > On Tue, 13 Dec 2022 11:40:56 -0800
> > Steve Sistare <steven.sistare@oracle.com> wrote:
> >   
> >> When a vfio container is preserved across exec using the VFIO_UPDATE_VADDR
> >> interfaces, locked_vm of the new mm becomes 0.  If the user later unmaps a
> >> dma mapping, locked_vm underflows to a large unsigned value, and a
> >> subsequent dma map request fails with ENOMEM in __account_locked_vm.
> >>
> >> To avoid underflow, do not decrement locked_vm during unmap if the
> >> dma's mm has changed.  To restore the correct locked_vm count, when
> >> VFIO_DMA_MAP_FLAG_VADDR is used and the dma's mm has changed, add
> >> the mapping's pinned page count to the new mm->locked_vm, subject
> >> to the rlimit.  Now that mediated devices are excluded when using
> >> VFIO_UPDATE_VADDR, the amount of pinned memory equals the size of
> >> the mapping.  
> > 
> > Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")
> > 
> >   
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >> ---
> >>  drivers/vfio/vfio_iommu_type1.c | 23 +++++++++++++++++++----
> >>  1 file changed, 19 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 80bdb4d..35a1a52 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -100,6 +100,7 @@ struct vfio_dma {
> >>  	struct task_struct	*task;
> >>  	struct rb_root		pfn_list;	/* Ex-user pinned pfn list */
> >>  	unsigned long		*bitmap;
> >> +	struct mm_struct	*mm;
> >>  };
> >>  
> >>  struct vfio_batch {
> >> @@ -1165,7 +1166,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
> >>  					    &iotlb_gather);
> >>  	}
> >>  
> >> -	if (do_accounting) {
> >> +	if (do_accounting && current->mm == dma->mm) {  
> > 
> > 
> > This seems incompatible with ffed0518d871 ("vfio: remove useless
> > judgement") where we no longer assume that the unmap mm is the same as
> > the mapping mm.  
> 
> They are compatible.  My fix allows another task to unmap, but only decreases
> locked_vm if the current mm matches the original mm that locked it.  And the
> "original" mm is updated by MAP_FLAG_VADDR.

It seems like there's either a bug fix or behavioral change to
ffed0518d871 then.  What mm were we previously accounting in their
fork/exec scenario that we're not with this change?

> > Does this need to get_task_mm(dma->task) and compare that mm to dma->mm
> > to determine whether an exec w/o vaddr remapping has occurred?  That's
> > the only use case I can figure out where grabbing the mm for dma->mm
> > actually makes any sense at all.  
> 
> The mm grab does detect an exec.  Before exec, at map time, we get task and grab
> its mm.  During exec, task gets a new mm.  The old mm becomes defunct, but we
> still hold it and can examine its pointer address.

This is describing exactly the test I'm asking about, if dma->task->mm
no longer matches dma->mm then an exec has occurred w/o a subsequent
vaddr remap.  So why are we bringing current->mm into the equation?
Thanks,

Alex 

> The new code does not require that current == dma->task.
> 
> >>  		vfio_lock_acct(dma, -unlocked, true);
> >>  		return 0;
> >>  	}
> >> @@ -1178,6 +1179,7 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
> >>  	vfio_unmap_unpin(iommu, dma, true);
> >>  	vfio_unlink_dma(iommu, dma);
> >>  	put_task_struct(dma->task);
> >> +	mmdrop(dma->mm);
> >>  	vfio_dma_bitmap_free(dma);
> >>  	if (dma->vaddr_invalid) {
> >>  		iommu->vaddr_invalid_count--;
> >> @@ -1623,9 +1625,20 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> >>  			   dma->size != size) {
> >>  			ret = -EINVAL;
> >>  		} else {
> >> -			dma->vaddr = vaddr;
> >> -			dma->vaddr_invalid = false;
> >> -			iommu->vaddr_invalid_count--;
> >> +			if (current->mm != dma->mm) {
> >> +				ret = vfio_lock_acct(dma, size >> PAGE_SHIFT,
> >> +						     0);
> >> +				if (!ret) {
> >> +					mmdrop(dma->mm);
> >> +					dma->mm = current->mm;
> >> +					mmgrab(dma->mm);
> >> +				}
> >> +			}
> >> +			if (!ret) {
> >> +				dma->vaddr = vaddr;
> >> +				dma->vaddr_invalid = false;
> >> +				iommu->vaddr_invalid_count--;
> >> +			}  
> > 
> > Poor flow, shouldn't this be:
> > 
> > 			if (current->mm != dma->mm) {
> > 				ret = vfio_lock_acct(dma,
> > 						     size >> PAGE_SHIFT, 0);
> > 				if (ret)
> > 					goto out_unlock;
> > 
> > 				mmdrop(dma->mm);
> > 				dma->mm = current->mm;
> > 				mmgrab(dma->mm);
> > 			}
> > 			dma->vaddr = vaddr;
> > 			dma->vaddr_invalid = false;
> > 			iommu->vaddr_invalid_count--;  
> 
> Better, will do, thanks.
> 
> - Steve
> 
> >>  			wake_up_all(&iommu->vaddr_wait);
> >>  		}
> >>  		goto out_unlock;
> >> @@ -1683,6 +1696,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> >>  	get_task_struct(current->group_leader);
> >>  	dma->task = current->group_leader;
> >>  	dma->lock_cap = capable(CAP_IPC_LOCK);
> >> +	dma->mm = dma->task->mm;
> >> +	mmgrab(dma->mm);
> >>  
> >>  	dma->pfn_list = RB_ROOT;
> >>    
> >   
> 

Re: [PATCH V2 2/5] vfio/type1: prevent locked_vm underflow

[Steven Sistare]

On 12/13/2022 4:31 PM, Alex Williamson wrote:
> On Tue, 13 Dec 2022 16:01:21 -0500
> Steven Sistare <steven.sistare@oracle.com> wrote:
>> On 12/13/2022 3:23 PM, Alex Williamson wrote:
>>> On Tue, 13 Dec 2022 11:40:56 -0800
>>> Steve Sistare <steven.sistare@oracle.com> wrote:
>>>   
>>>> When a vfio container is preserved across exec using the VFIO_UPDATE_VADDR
>>>> interfaces, locked_vm of the new mm becomes 0.  If the user later unmaps a
>>>> dma mapping, locked_vm underflows to a large unsigned value, and a
>>>> subsequent dma map request fails with ENOMEM in __account_locked_vm.
>>>>
>>>> To avoid underflow, do not decrement locked_vm during unmap if the
>>>> dma's mm has changed.  To restore the correct locked_vm count, when
>>>> VFIO_DMA_MAP_FLAG_VADDR is used and the dma's mm has changed, add
>>>> the mapping's pinned page count to the new mm->locked_vm, subject
>>>> to the rlimit.  Now that mediated devices are excluded when using
>>>> VFIO_UPDATE_VADDR, the amount of pinned memory equals the size of
>>>> the mapping.  
>>>
>>> Fixes: c3cbab24db38 ("vfio/type1: implement interfaces to update vaddr")
>>>
>>>   
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>>  drivers/vfio/vfio_iommu_type1.c | 23 +++++++++++++++++++----
>>>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>>>> index 80bdb4d..35a1a52 100644
>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>> @@ -100,6 +100,7 @@ struct vfio_dma {
>>>>  	struct task_struct	*task;
>>>>  	struct rb_root		pfn_list;	/* Ex-user pinned pfn list */
>>>>  	unsigned long		*bitmap;
>>>> +	struct mm_struct	*mm;
>>>>  };
>>>>  
>>>>  struct vfio_batch {
>>>> @@ -1165,7 +1166,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
>>>>  					    &iotlb_gather);
>>>>  	}
>>>>  
>>>> -	if (do_accounting) {
>>>> +	if (do_accounting && current->mm == dma->mm) {  
>>>
>>>
>>> This seems incompatible with ffed0518d871 ("vfio: remove useless
>>> judgement") where we no longer assume that the unmap mm is the same as
>>> the mapping mm.  
>>
>> They are compatible.  My fix allows another task to unmap, but only decreases
>> locked_vm if the current mm matches the original mm that locked it.  And the
>> "original" mm is updated by MAP_FLAG_VADDR.
> 
> It seems like there's either a bug fix or behavioral change to
> ffed0518d871 then.  What mm were we previously accounting in their
> fork/exec scenario that we're not with this change?

locked_vm is broken in their fork/exec scenario.  They must have some other
patch or work around for it.  Or, they have not tested enough to hit underflow.
If after exec, you first dma map something new, then dma unmap something smaller,
locked_vm does not underflow.

>>> Does this need to get_task_mm(dma->task) and compare that mm to dma->mm
>>> to determine whether an exec w/o vaddr remapping has occurred?  That's
>>> the only use case I can figure out where grabbing the mm for dma->mm
>>> actually makes any sense at all.  
>>
>> The mm grab does detect an exec.  Before exec, at map time, we get task and grab
>> its mm.  During exec, task gets a new mm.  The old mm becomes defunct, but we
>> still hold it and can examine its pointer address.
> 
> This is describing exactly the test I'm asking about, if dma->task->mm
> no longer matches dma->mm then an exec has occurred w/o a subsequent
> vaddr remap.  So why are we bringing current->mm into the equation?

Sorry, memories of my "redo" series are leaking in.  You are correct,
vfio_lock_acct modifies dma->task->mm, and dma->task may differ from current,
so I do need to check get_task_mm(dma->task).

- Steve

>> The new code does not require that current == dma->task.
>>
>>>>  		vfio_lock_acct(dma, -unlocked, true);
>>>>  		return 0;
>>>>  	}
>>>> @@ -1178,6 +1179,7 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
>>>>  	vfio_unmap_unpin(iommu, dma, true);
>>>>  	vfio_unlink_dma(iommu, dma);
>>>>  	put_task_struct(dma->task);
>>>> +	mmdrop(dma->mm);
>>>>  	vfio_dma_bitmap_free(dma);
>>>>  	if (dma->vaddr_invalid) {
>>>>  		iommu->vaddr_invalid_count--;
>>>> @@ -1623,9 +1625,20 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>>>>  			   dma->size != size) {
>>>>  			ret = -EINVAL;
>>>>  		} else {
>>>> -			dma->vaddr = vaddr;
>>>> -			dma->vaddr_invalid = false;
>>>> -			iommu->vaddr_invalid_count--;
>>>> +			if (current->mm != dma->mm) {
>>>> +				ret = vfio_lock_acct(dma, size >> PAGE_SHIFT,
>>>> +						     0);
>>>> +				if (!ret) {
>>>> +					mmdrop(dma->mm);
>>>> +					dma->mm = current->mm;
>>>> +					mmgrab(dma->mm);
>>>> +				}
>>>> +			}
>>>> +			if (!ret) {
>>>> +				dma->vaddr = vaddr;
>>>> +				dma->vaddr_invalid = false;
>>>> +				iommu->vaddr_invalid_count--;
>>>> +			}  
>>>
>>> Poor flow, shouldn't this be:
>>>
>>> 			if (current->mm != dma->mm) {
>>> 				ret = vfio_lock_acct(dma,
>>> 						     size >> PAGE_SHIFT, 0);
>>> 				if (ret)
>>> 					goto out_unlock;
>>>
>>> 				mmdrop(dma->mm);
>>> 				dma->mm = current->mm;
>>> 				mmgrab(dma->mm);
>>> 			}
>>> 			dma->vaddr = vaddr;
>>> 			dma->vaddr_invalid = false;
>>> 			iommu->vaddr_invalid_count--;  
>>
>> Better, will do, thanks.
>>
>> - Steve
>>
>>>>  			wake_up_all(&iommu->vaddr_wait);
>>>>  		}
>>>>  		goto out_unlock;
>>>> @@ -1683,6 +1696,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>>>>  	get_task_struct(current->group_leader);
>>>>  	dma->task = current->group_leader;
>>>>  	dma->lock_cap = capable(CAP_IPC_LOCK);
>>>> +	dma->mm = dma->task->mm;
>>>> +	mmgrab(dma->mm);
>>>>  
>>>>  	dma->pfn_list = RB_ROOT;
>>>>    
>>>   
>>
>