Re: [PATCH v3 23/32] vfio: use iova_to_phys_length for efficient unmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ARM SMMU translation fault mishandling leads to unbounded WARN_ON spam and a massive leak of pinned pages.
--

commit 4e6f222bdc9e32de4a4536a57f7ce9b046abb017
Author: Guanghui Feng <guanghuifeng@linux.alibaba.com>

vfio: use iova_to_phys_length for efficient unmap
    
Use iommu_iova_to_phys_length() to get PTE page size, allowing
traversal by actual mapping granularity instead of PAGE_SIZE steps.

> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c8151ba54de33..115d88d7003e8 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1177,25 +1177,42 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
>  
>  	iommu_iotlb_gather_init(&iotlb_gather);
>  	while (pos < dma->size) {
> -		size_t unmapped, len;
> +		size_t unmapped, len, pgsize;
>  		phys_addr_t phys, next;
>  		dma_addr_t iova = dma->iova + pos;
>  
> -		phys = iommu_iova_to_phys(domain->domain, iova);
> -		if (WARN_ON(!phys)) {
> +		/* Single page table walk returns both phys and PTE size */
> +		phys = iommu_iova_to_phys_length(domain->domain, iova,
> +						  &pgsize);
> +		if (WARN_ON(phys == PHYS_ADDR_MAX)) {
>  			pos += PAGE_SIZE;
>  			continue;
>  		}
> +		if (WARN_ON(!pgsize || pgsize < PAGE_SIZE))
> +			pgsize = PAGE_SIZE;

[Severity: High]
How does this handle ARM SMMU Stage-1 domains where the iova_to_phys_length
callback delegates to arm_smmu_iova_to_phys_hard()?

When a translation fault occurs, arm_smmu_iova_to_phys_hard() returns 0 rather
than PHYS_ADDR_MAX, and leaves pgsize as 0. Because 0 != PHYS_ADDR_MAX, this
check doesn't catch the fault.

Could this regression cause the WARN_ON(!pgsize) below to trigger and dump a
stack trace for every unmapped page in the region?

Additionally, when this falls through to unmap the page, unmap_unpin_fast()
correctly returns 0 unmapped bytes. This later hits the unmap loop break:

drivers/vfio/vfio_iommu_type1.c:vfio_unmap_unpin() {
        ...
        unmapped = unmap_unpin_fast(domain, dma, iova, len, phys, ...);
        if (!unmapped) {
                unmapped = unmap_unpin_slow(domain, dma, iova, len,
                                            phys, &unlocked);
                if (WARN_ON(!unmapped))
                        break;
        }
        ...
}

Does breaking out of this loop on the 0-byte unmap permanently skip and leak
any remaining pinned memory pages in the DMA region?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603151804.1963871-1-guanghuifeng@linux.alibaba.com?part=23