Re: [PATCH rc 3/5] iommu: Handle unmap error when iommu_debug is enabled

[Samiullah Khawaja <skhawaja@google.com>]

On Tue, May 12, 2026 at 01:46:15PM -0300, Jason Gunthorpe wrote:
>Sashiko noticed a latent bug where the map error flow called iommu_unmap()
>which calls iommu_debug_unmap_begin()/iommu_debug_unmap_end() however
>since this is an error path the map flow never actually established the
>original iommu_debug_map() it will malfunction.
>
>Lift the unmap error handling into iommu_map_nosync() and reorder it so
>the trace_map()/iommu_debug_map() records the partial mapping and then
>immediately unmaps it. This avoid creating the unbalanced tracking and
>provides saner tracing instead of a unmap unmatched to any map.
>
>Fixes: ccc21213f013 ("iommu: Add calls for IOMMU_DEBUG_PAGEALLOC")
>Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>---
> drivers/iommu/iommu.c | 49 +++++++++++++++++--------------------------
> 1 file changed, 19 insertions(+), 30 deletions(-)
>
>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>index e334588a2476b4..e5fa9875900228 100644
>--- a/drivers/iommu/iommu.c
>+++ b/drivers/iommu/iommu.c
>@@ -2575,12 +2575,11 @@ static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
>
> static int __iommu_map_domain_pgtbl(struct iommu_domain *domain,
> 				    unsigned long iova, phys_addr_t paddr,
>-				    size_t size, int prot, gfp_t gfp)
>+				    size_t size, int prot, gfp_t gfp,
>+				    size_t *mapped)
> {
> 	const struct iommu_domain_ops *ops = domain->ops;
>-	unsigned long orig_iova = iova;
> 	unsigned int min_pagesz;
>-	size_t orig_size = size;
> 	int ret = 0;
>
> 	if (WARN_ON(!ops->map_pages))
>@@ -2603,31 +2602,25 @@ static int __iommu_map_domain_pgtbl(struct iommu_domain *domain,
> 	pr_debug("map: iova 0x%lx pa %pa size 0x%zx\n", iova, &paddr, size);
>
> 	while (size) {
>-		size_t pgsize, count, mapped = 0;
>+		size_t pgsize, count, op_mapped = 0;
>
> 		pgsize = iommu_pgsize(domain, iova, paddr, size, &count);
>
> 		pr_debug("mapping: iova 0x%lx pa %pa pgsize 0x%zx count %zu\n",
> 			 iova, &paddr, pgsize, count);
> 		ret = ops->map_pages(domain, iova, paddr, pgsize, count, prot,
>-				     gfp, &mapped);
>+				     gfp, &op_mapped);
> 		/*
> 		 * Some pages may have been mapped, even if an error occurred,
> 		 * so we should account for those so they can be unmapped.
> 		 */
>-		size -= mapped;
>-
>+		*mapped += op_mapped;
> 		if (ret)
>-			break;
>+			return ret;
>
>-		iova += mapped;
>-		paddr += mapped;
>-	}
>-
>-	/* unroll mapping in case something went wrong */
>-	if (ret) {
>-		iommu_unmap(domain, orig_iova, orig_size - size);
>-		return ret;
>+		size -= op_mapped;
>+		iova += op_mapped;
>+		paddr += op_mapped;
> 	}
> 	return 0;
> }
>@@ -2645,6 +2638,7 @@ int iommu_map_nosync(struct iommu_domain *domain, unsigned long iova,
> 		phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> {
> 	struct pt_iommu *pt = iommupt_from_domain(domain);
>+	size_t mapped = 0;
> 	int ret;
>
> 	might_sleep_if(gfpflags_allow_blocking(gfp));
>@@ -2656,24 +2650,19 @@ int iommu_map_nosync(struct iommu_domain *domain, unsigned long iova,
> 				 __GFP_HIGHMEM))))
> 		return -EINVAL;
>
>-	if (pt) {
>-		size_t mapped = 0;
>-
>+	if (pt)
> 		ret = pt->ops->map_range(pt, iova, paddr, size, prot, gfp,
> 					 &mapped);
>-		if (ret) {
>-			iommu_unmap(domain, iova, mapped);
>-			return ret;
>-		}
>-	} else {
>+	else
> 		ret = __iommu_map_domain_pgtbl(domain, iova, paddr, size, prot,
>-					       gfp);
>-		if (ret)
>-			return ret;
>-	}
>+					       gfp, &mapped);
>
>-	trace_map(iova, paddr, size);
>-	iommu_debug_map(domain, paddr, size);
>+	trace_map(iova, paddr, mapped);
>+	iommu_debug_map(domain, paddr, mapped);
>+	if (ret) {
>+		iommu_unmap(domain, iova, mapped);
>+		return ret;
>+	}
> 	return 0;
> }
>
>-- 
>2.43.0
>

Reviewed-by: Samiullah Khawaja <skhawaja@google.com>