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

[Mostafa Saleh <smostafa@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.

There is usually littel coverage in such paths, I have been thinking
of creating some test-suites on the IOMMU and io-pgtable/SMMUv3 level
to cover some of the failure cases and some of the tricky page table
operations, the problem is that there are many things to cover (starting
from the crashes/logical issues till TLB invalidation correctness of
some operations).

I will try to post something when I get some time.

Reviewed-by: Mostafa Saleh <smostafa@google.com>

Thanks,
Mostafa

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