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 <[email protected]> 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: [email protected] > 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/[email protected]?part=1
