[PATCH rc 2/2] iommu: Do not return 0 from map_pages if it doesn't do anything

[Will Deacon]

On Thu, Aug 22, 2024 at 11:45:55AM -0300, Jason Gunthorpe wrote:
> These three implementations of map_pages() all succeed if a mapping is
> requested with no read or write. Since they return back to __iommu_map()
> leaving the mapped output as 0 it triggers an infinite loop. Therefore
> nothing is using no-access protection bits.
> 
> Further, VFIO and iommufd rely on iommu_iova_to_phys() to get back PFNs
> stored by map, if iommu_map() succeeds but iommu_iova_to_phys() fails that
> will create serious bugs.
> 
> Thus remove this never used "nothing to do" concept and just fail map
> immediately.
> 
> Fixes: e5fc9753b1a8 ("iommu/io-pgtable: Add ARMv7 short descriptor support")
> Fixes: e1d3c0fd701d ("iommu: add ARM LPAE page table allocator")
> Fixes: 745ef1092bcf ("iommu/io-pgtable: Move Apple DART support to its own file")
> Signed-off-by: Jason Gunthorpe <jgg at nvidia.com>
> ---
>  drivers/iommu/io-pgtable-arm-v7s.c | 3 +--
>  drivers/iommu/io-pgtable-arm.c     | 3 +--
>  drivers/iommu/io-pgtable-dart.c    | 3 +--
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
> index 75f244a3e12df6..06ffc683b28fee 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -552,9 +552,8 @@ static int arm_v7s_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
>  		    paddr >= (1ULL << data->iop.cfg.oas)))
>  		return -ERANGE;
>  
> -	/* If no access, then nothing to do */
>  	if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
> -		return 0;
> +		return -EINVAL;
>  
>  	while (pgcount--) {
>  		ret = __arm_v7s_map(data, iova, paddr, pgsize, prot, 1, data->pgd,
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index f5d9fd1f45bf49..ff4149ae1751d4 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -515,9 +515,8 @@ static int arm_lpae_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
>  	if (WARN_ON(iaext || paddr >> cfg->oas))
>  		return -ERANGE;
>  
> -	/* If no access, then nothing to do */
>  	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
> -		return 0;
> +		return -EINVAL;

I think just removing this hunk altogether would get us the right semantics
for stage-2 mappings, but it's esoteric and not used so -EINVAL is probably
better:

Acked-by: Will Deacon <will at kernel.org>

Will