Skip to contentSkip to conversation

drm/msm: DMABUF_DEBUG fixes

PATCH Superseded Rob Clark 5 participants 14 messages Oct 6 – Oct 7

Subsystems (4)

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Rob Clark

author maintainer
#0 lore raw

cover drm/msm: DMABUF_DEBUG fixes

With DMABUF_DEBUG=y, the page information is stripped from the sgt that we get for an externally allocated buffer that is dma-buf imported (as opposed to an exported GEM buffer that is re-imported). This is a bit unfortunate, since we need that information for mapping the buffer in both paths:

1. Userspace GPU pgtables (TTBR0), where we use io_pgtable directly. Fortunately this we can fix ourselves easily.

2. Iommu managed pgtables (scanout, kernel GPU buffers (TTBR1)). In normal usage that iommu subsystem envisions the dma-mapping and iommu-mapping are combined. We can't really work around this but we can stop trying to pretend to be a normal iommu user and just go directly to the iommu_domain_ops.

This all depends on being dma_map_direct() for the dma_map_sgtable() to work. This should be the case on modern things that use arm-smmu. It's less clear when it comes to older pre-arm-smmu hw. It's possible that this series causes those things to catch fire. If so, look to commit 0e764a01015d ("iommu/arm-smmu: Allow client devices to select direct mapping") for inspiration.

Rob Clark (5):
drm/msm: Cleanup if pages_to_sg() fails
drm/msm/gem: dma_map/unmap_sgtable()
drm/msm: Extract out map/unmap helpers
drm/msm: Convert iommu map/unmap to helpers
drm/msm: Convert map helper to use dma-address

 drivers/gpu/drm/msm/msm_gem.c   |  57 ++++++----
 drivers/gpu/drm/msm/msm_iommu.c | 182 ++++++++++++++++++++------------
 2 files changed, 153 insertions(+), 86 deletions(-)
Signature · 1 line
2.55.0
author maintainer
#1 lore raw

PATCH 1/5 drm/msm: Cleanup if pages_to_sg() fails

We never really handled this. But in practice mapping to smmu would have failed without an sgt, which would have been pretty obvious.

---
 drivers/gpu/drm/msm/msm_gem.c | 41 ++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 18 deletions(-)
drivers/gpu/drm/msm/msm_gem.c +23 -18
index 130ae27ee00b..181b9ad32672 100644
@@ -190,6 +190,7 @@ static void update_lru(struct drm_gem_object *obj)
 static struct page **get_pages(struct drm_gem_object *obj)
 {
 	struct msm_gem_object *msm_obj = to_msm_bo(obj);
+	int ret = 0;
 
 	msm_gem_assert_locked(obj);
 
@@ -209,19 +210,17 @@ static struct page **get_pages(struct drm_gem_object *obj)
 			return p;
 		}
 
-		update_device_mem(dev->dev_private, obj->size);
-
 		msm_obj->pages = p;
 
 		msm_obj->sgt = drm_prime_pages_to_sg(obj->dev, p, npages);
 		if (IS_ERR(msm_obj->sgt)) {
-			void *ptr = ERR_CAST(msm_obj->sgt);
-
 			DRM_DEV_ERROR(dev->dev, "failed to allocate sgt\n");
-			msm_obj->sgt = NULL;
-			return ptr;
+			ret = PTR_ERR(msm_obj->sgt);
+			goto error;
 		}
 
+		update_device_mem(dev->dev_private, obj->size);
+
 		/* For non-cached buffers, ensure the new pages are clean
 		 * because display controller, GPU, etc. are not coherent:
 		 */
@@ -232,6 +231,14 @@ static struct page **get_pages(struct drm_gem_object *obj)
 	}
 
 	return msm_obj->pages;
+
+error:
+	msm_obj->sgt = NULL;
+
+	drm_gem_put_pages(obj, msm_obj->pages, false, false);
+	msm_obj->pages = NULL;
+
+	return ERR_PTR(ret);
 }
 
 static void put_pages(struct drm_gem_object *obj)
@@ -246,18 +253,16 @@ static void put_pages(struct drm_gem_object *obj)
 		drm_gpuvm_bo_gem_evict(obj, true);
 
 	if (msm_obj->pages) {
-		if (msm_obj->sgt) {
-			/* For non-cached buffers, ensure the new
-			 * pages are clean because display controller,
-			 * GPU, etc. are not coherent:
-			 */
-			if (msm_obj->flags & MSM_BO_WC)
-				sync_for_cpu(msm_obj);
-
-			sg_free_table(msm_obj->sgt);
-			kfree(msm_obj->sgt);
-			msm_obj->sgt = NULL;
-		}
+		/* For non-cached buffers, ensure the new
+		 * pages are clean because display controller,
+		 * GPU, etc. are not coherent:
+		 */
+		if (msm_obj->flags & MSM_BO_WC)
+			sync_for_cpu(msm_obj);
+
+		sg_free_table(msm_obj->sgt);
+		kfree(msm_obj->sgt);
+		msm_obj->sgt = NULL;
 
 		update_device_mem(obj->dev->dev_private, -obj->size);
 
Signature · 1 line
2.55.0
author maintainer
#2 lore raw

PATCH 2/5 drm/msm/gem: dma_map/unmap_sgtable()

In reply to Rob Clark

This should functionally be a no-op for us, since we are managing our SMMU directly (dma_map_ops is NULL), but it populates the dma_len/ dma_address's so that we can use them for setting up pgtable mappings.

---
 drivers/gpu/drm/msm/msm_gem.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/gpu/drm/msm/msm_gem.c +16 -0
index 181b9ad32672..72e3cd8bb1e7 100644
@@ -219,6 +219,15 @@ static struct page **get_pages(struct drm_gem_object *obj)
 			goto error;
 		}
 
+		ret = dma_map_sgtable(drm_dev_dma_dev(obj->dev),
+				      msm_obj->sgt,
+				      DMA_BIDIRECTIONAL,
+				      DMA_ATTR_SKIP_CPU_SYNC);
+		if (ret) {
+			DRM_DEV_ERROR(dev->dev, "could not map sgtable: %d\n", ret);
+			goto error;
+		}
+
 		update_device_mem(dev->dev_private, obj->size);
 
 		/* For non-cached buffers, ensure the new pages are clean
@@ -233,6 +242,8 @@ static struct page **get_pages(struct drm_gem_object *obj)
 	return msm_obj->pages;
 
 error:
+	if (!IS_ERR(msm_obj->sgt))
+		drm_prime_gem_destroy(obj, msm_obj->sgt);
 	msm_obj->sgt = NULL;
 
 	drm_gem_put_pages(obj, msm_obj->pages, false, false);
@@ -260,6 +271,11 @@ static void put_pages(struct drm_gem_object *obj)
 		if (msm_obj->flags & MSM_BO_WC)
 			sync_for_cpu(msm_obj);
 
+		dma_unmap_sgtable(drm_dev_dma_dev(obj->dev),
+				  msm_obj->sgt,
+				  DMA_BIDIRECTIONAL,
+				  DMA_ATTR_SKIP_CPU_SYNC);
+
 		sg_free_table(msm_obj->sgt);
 		kfree(msm_obj->sgt);
 		msm_obj->sgt = NULL;
Signature · 1 line
2.55.0
author maintainer
#3 lore raw

PATCH 3/5 drm/msm: Extract out map/unmap helpers

In reply to Rob Clark

We're going to re-use these in the next commit. No functional changes, just some ugly callbacks.

---
 drivers/gpu/drm/msm/msm_iommu.c | 150 ++++++++++++++++++++------------
 1 file changed, 93 insertions(+), 57 deletions(-)
drivers/gpu/drm/msm/msm_iommu.c +93 -57
index da6782fca6bd..a658493f8007 100644
@@ -52,9 +52,8 @@ static struct msm_iommu_pagetable *to_pagetable(struct msm_mmu *mmu)
 }
 
 /* based on iommu_pgsize() in iommu.c: */
-static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,
-			   unsigned long iova, phys_addr_t paddr,
-			   size_t size, size_t *count)
+static size_t calc_pgsize(unsigned long pgsize_bitmap, unsigned long iova,
+			  phys_addr_t paddr, size_t size, size_t *count)
 {
 	unsigned int pgsize_idx, pgsize_idx_next;
 	unsigned long pgsizes;
@@ -62,7 +61,7 @@ static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,
 	unsigned long addr_merge = paddr | iova;
 
 	/* Page sizes supported by the hardware and small enough for @size */
-	pgsizes = pagetable->pgsize_bitmap & GENMASK(__fls(size), 0);
+	pgsizes = pgsize_bitmap & GENMASK(__fls(size), 0);
 
 	/* Constrain the page sizes further based on the maximum alignment */
 	if (likely(addr_merge))
@@ -78,7 +77,7 @@ static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,
 		return pgsize;
 
 	/* Find the next biggest support page size, if it exists */
-	pgsizes = pagetable->pgsize_bitmap & ~GENMASK(pgsize_idx, 0);
+	pgsizes = pgsize_bitmap & ~GENMASK(pgsize_idx, 0);
 	if (!pgsizes)
 		goto out_set_count;
 
@@ -107,24 +106,27 @@ static size_t calc_pgsize(struct msm_iommu_pagetable *pagetable,
 	return pgsize;
 }
 
-static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova,
-		size_t size)
+typedef size_t (*unmap_fn)(void *arg, u64 iova, size_t pgsize, size_t count);
+typedef int (*map_fn)(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize,
+		      size_t pgcount, int prot, size_t *mapped);
+
+static inline int
+__do_unmap(unsigned long pgsize_bitmap, u64 iova, size_t size,
+	   void *arg, unmap_fn unmap)
 {
-	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
-	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
 	int ret = 0;
 
 	while (size) {
 		size_t pgsize, count;
 		ssize_t unmapped;
 
-		pgsize = calc_pgsize(pagetable, iova, iova, size, &count);
+		pgsize = calc_pgsize(pgsize_bitmap, iova, iova, size, &count);
 
-		unmapped = ops->unmap_pages(ops, iova, pgsize, count, NULL);
+		unmapped = unmap(arg, iova, pgsize, count);
 		if (unmapped <= 0) {
 			ret = -EINVAL;
 			/*
-			 * Continue attempting to unamp the remained of the
+			 * Continue attempting to unmap the remained of the
 			 * range, so we don't end up with some dangling
 			 * mapped pages
 			 */
@@ -135,55 +137,18 @@ static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova,
 		size -= unmapped;
 	}
 
-	iommu_flush_iotlb_all(to_msm_iommu(pagetable->parent)->domain);
-
 	return ret;
 }
 
-static int msm_iommu_pagetable_map_prr(struct msm_mmu *mmu, u64 iova, size_t len, int prot)
-{
-	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
-	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
-	struct msm_iommu *iommu = to_msm_iommu(pagetable->parent);
-	phys_addr_t phys = page_to_phys(iommu->prr_page);
-	u64 addr = iova;
-
-	while (len) {
-		size_t mapped = 0;
-		size_t size = PAGE_SIZE;
-		int ret;
-
-		ret = ops->map_pages(ops, addr, phys, size, 1, prot, GFP_KERNEL, &mapped);
-
-		/* map_pages could fail after mapping some of the pages,
-		 * so update the counters before error handling.
-		 */
-		addr += mapped;
-		len  -= mapped;
-
-		if (ret) {
-			msm_iommu_pagetable_unmap(mmu, iova, addr - iova);
-			return -EINVAL;
-		}
-	}
-
-	return 0;
-}
-
-static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
-				   struct sg_table *sgt, size_t off, size_t len,
-				   int prot)
+static inline int
+__do_map(unsigned long pgsize_bitmap, u64 iova, struct sg_table *sgt, size_t off,
+	 size_t len, int prot, void *arg, map_fn map, unmap_fn unmap)
 {
-	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
-	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
 	struct scatterlist *sg;
 	u64 addr = iova;
 	unsigned int i;
 
-	if (!sgt)
-		return msm_iommu_pagetable_map_prr(mmu, iova, len, prot);
-
-	for_each_sgtable_sg(sgt, sg, i) {
+	for_each_sgtable_sg (sgt, sg, i) {
 		size_t size = sg->length;
 		phys_addr_t phys = sg_phys(sg);
 
@@ -204,10 +169,9 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
 			size_t pgsize, count, mapped = 0;
 			int ret;
 
-			pgsize = calc_pgsize(pagetable, addr, phys, size, &count);
+			pgsize = calc_pgsize(pgsize_bitmap, addr, phys, size, &count);
 
-			ret = ops->map_pages(ops, addr, phys, pgsize, count,
-					     prot, GFP_KERNEL, &mapped);
+			ret = map(arg, phys, addr, pgsize, count, prot, &mapped);
 
 			/* map_pages could fail after mapping some of the pages,
 			 * so update the counters before error handling.
@@ -218,7 +182,7 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
 			len  -= mapped;
 
 			if (ret) {
-				msm_iommu_pagetable_unmap(mmu, iova, addr - iova);
+				__do_unmap(pgsize_bitmap, iova, addr - iova, arg, unmap);
 				return -EINVAL;
 			}
 		}
@@ -227,6 +191,78 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
 	return 0;
 }
 
+static size_t
+__unmap_pgtable(void *arg, u64 iova, size_t pgsize, size_t pgcount)
+{
+	struct io_pgtable_ops *ops = arg;
+	return ops->unmap_pages(ops, iova, pgsize, pgcount, NULL);
+}
+
+static int
+__map_pgtable(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize,
+	      size_t pgcount, int prot, size_t *mapped)
+{
+	struct io_pgtable_ops *ops = arg;
+	return ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot, GFP_KERNEL, mapped);
+}
+
+static int msm_iommu_pagetable_unmap(struct msm_mmu *mmu, u64 iova, size_t size)
+{
+	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
+	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
+	int ret = 0;
+
+	ret = __do_unmap(pagetable->pgsize_bitmap, iova, size, ops, __unmap_pgtable);
+
+	iommu_flush_iotlb_all(to_msm_iommu(pagetable->parent)->domain);
+
+	return ret;
+}
+
+static int msm_iommu_pagetable_map_prr(struct msm_mmu *mmu, u64 iova, size_t len, int prot)
+{
+	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
+	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
+	struct msm_iommu *iommu = to_msm_iommu(pagetable->parent);
+	phys_addr_t phys = page_to_phys(iommu->prr_page);
+	u64 addr = iova;
+
+	while (len) {
+		size_t mapped = 0;
+		size_t size = PAGE_SIZE;
+		int ret;
+
+		ret = ops->map_pages(ops, addr, phys, size, 1, prot, GFP_KERNEL, &mapped);
+
+		/* map_pages could fail after mapping some of the pages,
+		 * so update the counters before error handling.
+		 */
+		addr += mapped;
+		len  -= mapped;
+
+		if (ret) {
+			msm_iommu_pagetable_unmap(mmu, iova, addr - iova);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
+				   struct sg_table *sgt, size_t off, size_t len,
+				   int prot)
+{
+	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
+	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
+
+	if (!sgt)
+		return msm_iommu_pagetable_map_prr(mmu, iova, len, prot);
+
+	return __do_map(pagetable->pgsize_bitmap, iova, sgt, off, len, prot,
+			ops, __map_pgtable, __unmap_pgtable);
+}
+
 static void msm_iommu_pagetable_destroy(struct msm_mmu *mmu)
 {
 	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
Signature · 1 line
2.55.0
#4 lore raw

Re: [RFT 3/5] drm/msm: Extract out map/unmap helpers

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm: missing IOTLB flush on pagetable map error path --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: missing IOTLB flush on pagetable map error path
--

commit 7d5e2545b852417f316f5873d935ec2b95998bbf
Author: Rob Clark <[email protected]>

drm/msm: Extract out map/unmap helpers

This commit extracts __do_map() and __do_unmap() helpers from msm_iommu_pagetable_map() and msm_iommu_pagetable_unmap() respectively. It aims to reuse these helpers without any functional changes.

Quoted 4 lines
drivers/gpu/drm/msm/msm_iommu.c +0 -0
index da6782fca6bd2..a658493f80072 100644

[ ... ]

Quoted 14 lines
drivers/gpu/drm/msm/msm_iommu.c +2 -3
@@ -204,10 +169,9 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
 			size_t pgsize, count, mapped = 0;
 			int ret;
 
-			pgsize = calc_pgsize(pagetable, addr, phys, size, &count);
+			pgsize = calc_pgsize(pgsize_bitmap, addr, phys, size, &count);
 
-			ret = ops->map_pages(ops, addr, phys, pgsize, count,
-					     prot, GFP_KERNEL, &mapped);
+			ret = map(arg, phys, addr, pgsize, count, prot, &mapped);
 
 			/* map_pages could fail after mapping some of the pages,
 			 * so update the counters before error handling.

drivers/gpu/drm/msm/msm_iommu.c static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova, · line 187

Quoted 7 lines
drivers/gpu/drm/msm/msm_iommu.c +1 -1
@@ -218,7 +182,7 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
 			len  -= mapped;
 
 			if (ret) {
-				msm_iommu_pagetable_unmap(mmu, iova, addr - iova);
+				__do_unmap(pgsize_bitmap, iova, addr - iova, arg, unmap);
 				return -EINVAL;
 			}

[Severity: High]
Does this error path miss an IOTLB flush?

Before this commit, a failure in the mapping loop would call msm_iommu_pagetable_unmap(), which flushes the IOTLB after unmapping.

Now, the refactored __do_map() calls the new __do_unmap() helper directly. Because __do_unmap() skips the IOTLB flush, the error rollback leaves stale virtual-to-physical address translations for partially mapped pages in the TLB, which can lead to rogue DMA and memory corruption (use-after-free).

[ ... ]

Quoted 6 lines
drivers/gpu/drm/msm/msm_iommu.c +2 -0
@@ -227,6 +191,78 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
 	return 0;
 }
 
+static size_t
+__unmap_pgtable(void *arg, u64 iova, size_t pgsize, size_t pgcount)

[ ... ]

drivers/gpu/drm/msm/msm_iommu.c static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova, · line 264

Quoted 13 lines
drivers/gpu/drm/msm/msm_iommu.c +13 -0
+static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
+				   struct sg_table *sgt, size_t off, size_t len,
+				   int prot)
+{
+	struct msm_iommu_pagetable *pagetable = to_pagetable(mmu);
+	struct io_pgtable_ops *ops = pagetable->pgtbl_ops;
+
+	if (!sgt)
+		return msm_iommu_pagetable_map_prr(mmu, iova, len, prot);
+
+	return __do_map(pagetable->pgsize_bitmap, iova, sgt, off, len, prot,
+			ops, __map_pgtable, __unmap_pgtable);
+}

[Severity: High]
Can this propagate the error from __do_map() without flushing the IOTLB?

When __do_map() hits an error and returns, msm_iommu_pagetable_map() passes the error up the stack without initiating a TLB flush, cementing the missing flush bug from the __do_map() failure path.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#5 lore raw

Re: [RFT 3/5] drm/msm: Extract out map/unmap helpers

Quoted 1 line

Does this error path miss an IOTLB flush?

Yes. msm_iommu_pagetable_unmap() called iommu_flush_iotlb_all() after rollback. __do_map() now calls __do_unmap() directly and loses that flush.

An arm64 QEMU/SMMUv3 test:

- mapped one 4 KiB page,
- hit -EEXIST on the second page, and
- verified that rollback removed the first PTE.

No stale translation was observed because the device had not accessed the mapping. This tested the low-level IOMMU sequence, not the MSM driver or Qualcomm hardware.

Restoring iommu_flush_iotlb_all() on this error path preserves the old behavior.

Karl

#6 lore raw

Re: [RFT 3/5] drm/msm: Extract out map/unmap helpers

Quoted 13 lines · Karl Mehltretter
Quoted 2 lines

Does this error path miss an IOTLB flush?

Yes. msm_iommu_pagetable_unmap() called iommu_flush_iotlb_all() after rollback. __do_map() now calls __do_unmap() directly and loses that flush.

An arm64 QEMU/SMMUv3 test:

- mapped one 4 KiB page,
- hit -EEXIST on the second page, and
- verified that rollback removed the first PTE.

Quoted 6 lines

No stale translation was observed because the device had not accessed the mapping. This tested the low-level IOMMU sequence, not the MSM driver or Qualcomm hardware.

Restoring iommu_flush_iotlb_all() on this error path preserves the old behavior.

I have a fixup, but haven't sent a v2 yet.. current series is still enough to verify whether there is more work needed on older pre-arm-smmu hw or not

BR,
-R

author maintainer
#7 lore raw

PATCH 4/5 drm/msm: Convert iommu map/unmap to helpers

In reply to Rob Clark

Re-use the previously introduced map/unmap helpers in the iommu path (TTBR1 for GPU, ie. kernel managed buffers, and display/etc).

The callbacks work in terms of iommu_domain/ops instead of io_pgtable. It would be kinda nice to just deal with io_pgtable, but the tlb ops that arm-smmu uses for it's own io_pgtable require doing runpm, which we wouldn't otherwise be able to do.

---
 drivers/gpu/drm/msm/msm_iommu.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)
drivers/gpu/drm/msm/msm_iommu.c +21 -11
index a658493f8007..50038d00c3a8 100644
@@ -720,12 +720,27 @@ static void msm_iommu_detach(struct msm_mmu *mmu)
 	iommu_detach_device(iommu->domain, mmu->dev);
 }
 
+static size_t
+__unmap_iommu(void *arg, u64 iova, size_t pgsize, size_t pgcount)
+{
+	struct iommu_domain *domain = arg;
+	return domain->ops->unmap_pages(domain, iova, pgsize, pgcount, NULL);
+}
+
+static int
+__map_iommu(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize,
+	    size_t pgcount, int prot, size_t *mapped)
+{
+	struct iommu_domain *domain = arg;
+	return domain->ops->map_pages(domain, iova, paddr, pgsize, pgcount,
+				      prot, GFP_KERNEL, mapped);
+}
+
 static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
 			 struct sg_table *sgt, size_t off, size_t len,
 			 int prot)
 {
-	struct msm_iommu *iommu = to_msm_iommu(mmu);
-	ssize_t ret;
+	struct iommu_domain *domain = to_msm_iommu(mmu)->domain;
 
 	WARN_ON(off != 0);
 
@@ -733,23 +748,18 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
 	if (iova & BIT_ULL(48))
 		iova |= GENMASK_ULL(63, 49);
 
-	ret = iommu_map_sgtable(iommu->domain, iova, sgt, prot);
-	if (ret < 0)
-		return ret;
-
-	return (ret == len) ? 0 : -EINVAL;
+	return __do_map(domain->pgsize_bitmap, iova, sgt, off, len, prot,
+			domain, __map_iommu, __unmap_iommu);
 }
 
 static int msm_iommu_unmap(struct msm_mmu *mmu, uint64_t iova, size_t len)
 {
-	struct msm_iommu *iommu = to_msm_iommu(mmu);
+	struct iommu_domain *domain = to_msm_iommu(mmu)->domain;
 
 	if (iova & BIT_ULL(48))
 		iova |= GENMASK_ULL(63, 49);
 
-	iommu_unmap(iommu->domain, iova, len);
-
-	return 0;
+	return __do_unmap(domain->pgsize_bitmap, iova, len, domain, __unmap_iommu);
 }
 
 static void msm_iommu_destroy(struct msm_mmu *mmu)
Signature · 1 line
2.55.0
#8 lore raw

Re: [RFT 4/5] drm/msm: Convert iommu map/unmap to helpers

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] msm: missing TLB flush in unmap sequence causes GPU UAF - [Low] msm: build failure due to…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] msm: missing TLB flush in unmap sequence causes GPU UAF
- [Low] msm: build failure due to undefined __do_map and __do_unmap
--

commit a6f7885817ad9924dcb4d6924c8061a9096367bc
Author: Rob Clark <[email protected]>
Subject: drm/msm: Convert iommu map/unmap to helpers

This patch converts the msm iommu map and unmap paths to use new helpers. It replaces calls to iommu_map_sgtable() and iommu_unmap() with __do_map() and __do_unmap(), utilizing __map_iommu() and __unmap_iommu() callbacks.

Quoted 4 lines
drivers/gpu/drm/msm/msm_iommu.c +0 -0
index da6782fca6bd2..03acc851c5e6d 100644

[ ... ]

drivers/gpu/drm/msm/msm_iommu.c static void msm_iommu_detach(struct msm_mmu *mmu) · line 692

Quoted 10 lines
drivers/gpu/drm/msm/msm_iommu.c +6 -0
@@ -684,12 +684,27 @@ static void msm_iommu_detach(struct msm_mmu *mmu)
 	iommu_detach_device(iommu->domain, mmu->dev);
 }
 
+static size_t
+__unmap_iommu(void *arg, u64 iova, size_t pgsize, size_t pgcount)
+{
+	struct iommu_domain *domain = arg;
+	return domain->ops->unmap_pages(domain, iova, pgsize, pgcount, NULL);
+}

[Severity: Critical]
By passing NULL for iotlb_gather, does this bypass the IOMMU driver's TLB gathering and flushing mechanism?

When iommu_unmap() was previously used, it provided an iotlb_gather structure and followed up with a synchronous TLB flush. Since this code is invoked from msm_iommu_unmap(), leaving the TLB unflushed might retain stale translations in hardware.

Can this lead to a use-after-free regression if the physical pages are later reallocated and accessed by the GPU through the stale TLB entries?

Quoted 3 lines
drivers/gpu/drm/msm/msm_iommu.c +3 -0
+
+static int
+__map_iommu(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize,

[ ... ]

drivers/gpu/drm/msm/msm_iommu.c static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, · line 717

Quoted 12 lines
drivers/gpu/drm/msm/msm_iommu.c +2 -5
@@ -697,23 +712,18 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
 	if (iova & BIT_ULL(48))
 		iova |= GENMASK_ULL(63, 49);
 
-	ret = iommu_map_sgtable(iommu->domain, iova, sgt, prot);
-	if (ret < 0)
-		return ret;
-
-	return (ret == len) ? 0 : -EINVAL;
+	return __do_map(domain->pgsize_bitmap, iova, sgt, off, len, prot,
+			domain, __map_iommu, __unmap_iommu);
 }

[Severity: Low]
Is __do_map() defined anywhere in the tree?

It appears this replaces iommu_map_sgtable() with a call to __do_map(), but there is no declaration or definition of __do_map() available. Does this cause an implicit declaration compile error when building with DRM_MSM enabled?

Quoted 4 lines
drivers/gpu/drm/msm/msm_iommu.c +0 -1
 static int msm_iommu_unmap(struct msm_mmu *mmu, uint64_t iova, size_t len)
 {
-	struct msm_iommu *iommu = to_msm_iommu(mmu);

drivers/gpu/drm/msm/msm_iommu.c static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, · line 763

Quoted 10 lines
drivers/gpu/drm/msm/msm_iommu.c +2 -3
+	struct iommu_domain *domain = to_msm_iommu(mmu)->domain;
 
 	if (iova & BIT_ULL(48))
 		iova |= GENMASK_ULL(63, 49);
 
-	iommu_unmap(iommu->domain, iova, len);
-
-	return 0;
+	return __do_unmap(domain->pgsize_bitmap, iova, len, domain, __unmap_iommu);
 }

[Severity: Low]
Similar to the mapping path, is __do_unmap() actually defined?

It looks like the change from iommu_unmap() to __do_unmap() will fail to compile due to a missing definition.

Additionally, if __do_unmap() simply invokes __unmap_iommu() and returns, is the required TLB flush completely missing from this unmap sequence?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#9 lore raw

Re: [RFT 4/5] drm/msm: Convert iommu map/unmap to helpers

Quoted 2 lines

By passing NULL for iotlb_gather, does this bypass the IOMMU driver's TLB gathering and flushing mechanism?

Yes. On arm64 QEMU/SMMUv3, unmap_pages(..., NULL) returned 4096 after device DMA populated the IOTLB, but the next DMA still reached the old page. It faulted after an explicit IOTLB flush.

Using iommu_unmap() invalidated the translation:

	return iommu_unmap(domain, iova, pgsize * pgcount);
Quoted 6 lines

It appears this replaces iommu_map_sgtable() with a call to __do_map(), but there is no declaration or definition of __do_map() available.

[...]

Similar to the mapping path, is __do_unmap() actually defined?

Patch 3 defines both helpers. msm_iommu.o builds with all five patches.

Karl

#10 lore raw

Re: [RFT 4/5] drm/msm: Convert iommu map/unmap to helpers

Quoted 12 lines · Karl Mehltretter
Quoted 3 lines

By passing NULL for iotlb_gather, does this bypass the IOMMU driver's TLB gathering and flushing mechanism?

Yes. On arm64 QEMU/SMMUv3, unmap_pages(..., NULL) returned 4096 after device DMA populated the IOTLB, but the next DMA still reached the old page. It faulted after an explicit IOTLB flush.

Using iommu_unmap() invalidated the translation:

        return iommu_unmap(domain, iova, pgsize * pgcount);
Quoted 8 lines
Quoted 6 lines

It appears this replaces iommu_map_sgtable() with a call to __do_map(), but there is no declaration or definition of __do_map() available.

[...]

Similar to the mapping path, is __do_unmap() actually defined?

Patch 3 defines both helpers. msm_iommu.o builds with all five patches.

Sashiko is kinda funny sometimes :-)

BR,
-R

author maintainer
#11 lore raw

PATCH 5/5 drm/msm: Convert map helper to use dma-address

In reply to Rob Clark

Now that we have everything unified in one path, switch over to using dma addresses for setting up the mapping. This avoids issues with CONFIG_DMABUF_DEBUG stripping the pages from the sgt.

---
 drivers/gpu/drm/msm/msm_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpu/drm/msm/msm_iommu.c +3 -3
index 50038d00c3a8..27a74514e942 100644
@@ -148,9 +148,9 @@ __do_map(unsigned long pgsize_bitmap, u64 iova, struct sg_table *sgt, size_t off
 	u64 addr = iova;
 	unsigned int i;
 
-	for_each_sgtable_sg (sgt, sg, i) {
-		size_t size = sg->length;
-		phys_addr_t phys = sg_phys(sg);
+	for_each_sgtable_dma_sg (sgt, sg, i) {
+		size_t size = sg_dma_len(sg);
+		phys_addr_t phys = sg_dma_address(sg);
 
 		if (!len)
 			break;
Signature · 1 line
2.55.0
Tested-by on the v1 series
#12 lore raw
In reply to Rob Clark

Hi Rob,

Quoted 3 lines · Rob Clark

With DMABUF_DEBUG=y, the page information is stripped from the sgt that we get for an externally allocated buffer that is dma-buf imported (as opposed to an exported GEM buffer that is re-imported). [...]

Tested on the machine that originally reported the breakage - this exercises exactly the externally-allocated-buffer case from your cover letter:

SFA14-11), v7.3-rc5 + this series, CONFIG_DMABUF_DEBUG=y

Hardware video decode in chromium (V4L2 decoder capture buffers from videobuf2-dma-contig imported into msm and rendered by the GPU) displays correctly, with zero arm-smmu faults and zero io-pgtable WARNs. For comparison, on plain v7.3-rc5 with DMABUF_DEBUG=y the same workload logs UCHE translation faults and a __arm_lpae_unmap() WARN storm (~470 traces per minute of playback).

This is also much cleaner than the translation-based approach in the follow-up series I withdrew - mapping from the DMA addresses is where I should have ended up in the first place.

One small suggestion for __do_map(): panthor treats "map_pages() mapped nothing" as an error (panthor_mmu.c):

        /* If nothing was mapped, consider it an ENOMEM. */
        if (!ret && !mapped)
                ret = -ENOMEM;

With the DMA fields now populated on both paths this should not trigger in practice, but it would turn any future regression back into a loud failure instead of a silent empty mapping - which is the failure mode this whole series fixes.

Happy to run more (heap-exported dmabuf import test, kmssink scanout of V4L2 frames) on any revision.

BR,
Jianfeng

#13 lore raw
Quoted 26 lines · Jianfeng Liu

Hi Rob,

Quoted 3 lines · Rob Clark

With DMABUF_DEBUG=y, the page information is stripped from the sgt that we get for an externally allocated buffer that is dma-buf imported (as opposed to an exported GEM buffer that is re-imported). [...]

Tested on the machine that originally reported the breakage - this exercises exactly the externally-allocated-buffer case from your cover letter:

SFA14-11), v7.3-rc5 + this series, CONFIG_DMABUF_DEBUG=y

Hardware video decode in chromium (V4L2 decoder capture buffers from videobuf2-dma-contig imported into msm and rendered by the GPU) displays correctly, with zero arm-smmu faults and zero io-pgtable WARNs. For comparison, on plain v7.3-rc5 with DMABUF_DEBUG=y the same workload logs UCHE translation faults and a __arm_lpae_unmap() WARN storm (~470 traces per minute of playback).

This is also much cleaner than the translation-based approach in the follow-up series I withdrew - mapping from the DMA addresses is where I should have ended up in the first place.

Quoted 6 lines

One small suggestion for __do_map(): panthor treats "map_pages() mapped nothing" as an error (panthor_mmu.c):

        /* If nothing was mapped, consider it an ENOMEM. */
        if (!ret && !mapped)
                ret = -ENOMEM;

Yeah, that is probably a good idea.

Thanks for testing

BR,
-R

Quoted 11 lines

With the DMA fields now populated on both paths this should not trigger in practice, but it would turn any future regression back into a loud failure instead of a silent empty mapping - which is the failure mode this whole series fixes.

Happy to run more (heap-exported dmabuf import test, kmssink scanout of V4L2 frames) on any revision.

BR,
Jianfeng