Re: [PATCH v4 1/3] drm/gpusvm: free the whole IOVA reservation on unmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `dma_iova_destroy()` API violates its contract by failing to free the entire IOVA reservation, causing leaks across multiple subsystems.
--

commit 2502141c3a5e40996ab0b0ebb511ee9346f04740
Author: Honglei Huang <honghuan@amd.com>
Subject: drm/gpusvm: free the whole IOVA reservation on unmap

This commit fixes an IOVA space leak in drm_gpusvm by replacing a call
to dma_iova_destroy() with dma_iova_unlink() and dma_iova_free(). The
issue occurred because dma_iova_destroy() only freed the linked portion
of the reservation.

Cc: stable@vger.kernel.org

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 958cb605aedd5..3145d55cd860b 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -1146,10 +1146,19 @@ static void __drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm,
>  		};
>  		bool use_iova = dma_use_iova(&svm_pages->state);
>  
> -		if (use_iova)
> -			dma_iova_destroy(dev, &svm_pages->state,
> -					 svm_pages->state_offset,
> -					 svm_pages->dma_addr[0].dir, 0);

[Severity: High]
This is a pre-existing issue, but does dma_iova_destroy() violate its contract
across the kernel by failing to free the entire IOVA reservation?

While this patch correctly works around the leak in drm_gpusvm by using
dma_iova_unlink() and dma_iova_free(), the underlying dma_iova_destroy()
implementation still appears to use mapped_len to determine how much to free:

drivers/iommu/dma-iommu.c:dma_iova_destroy() {
    ...
    __iommu_dma_iova_unlink(dev, state, 0, mapped_len, dir, attrs, true);
    ...
}

If mapped_len is smaller than state->__size, the rest of the IOVA space is
leaked, eventually exhausting the IOVA space and causing DMA mapping failures.
This appears to affect other subsystems as well. For example, if pages are
skipped during migration:

drivers/gpu/drm/drm_pagemap.c:drm_pagemap_migrate_unmap_pages() {
    ...
    dma_iova_destroy(dev, &state->dma_state, state->offset, dir, 0);
    ...
}

It also seems reachable in error paths where a mapping fails halfway:

drivers/dma-buf/dma-buf-mapping.c:dma_buf_phys_vec_to_sgt() {
    ...
    dma_iova_destroy(attach->dev, dma->state, mapped_len, dir, DMA_ATTR_MMIO);
    ...
}

Would it be better to fix dma_iova_destroy() centrally so it always frees the
entire IOVA space regardless of the mapped_len parameter, rather than
open-coding the fix here?

> +		/*
> +		 * IOVA is reserved for the whole range but only the linked
> +		 * system pages (state_offset bytes) need unlinking; free the
> +		 * entire reservation to avoid leaking the device-page part.
> +		 * On the error path state_offset is 0, so just free it.
> +		 */
> +		if (use_iova) {
> +			if (svm_pages->state_offset)
> +				dma_iova_unlink(dev, &svm_pages->state, 0,
> +						svm_pages->state_offset,
> +						svm_pages->dma_addr[0].dir, 0);
> +			dma_iova_free(dev, &svm_pages->state);
> +		}
>  
>  		for (i = 0, j = 0; i < npages; j++) {
>  			struct drm_pagemap_addr *addr = &svm_pages->dma_addr[j];

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701062800.409248-1-honghuan@amd.com?part=1