[PATCH 0/2] drm/gpusvm: fix IOVA unmap path in __drm_gpusvm_unmap_pages()

[PATCH 0/2] drm/gpusvm: fix IOVA unmap path in __drm_gpusvm_unmap_pages()

[Honglei Huang]

From: Honglei Huang <honghuan@amd.com>

Two small fixes for the IOVA unmap path in __drm_gpusvm_unmap_pages(),
both spotted by AI review:

  - Avoid reading dma_addr[0].dir when no pages were mapped (the
    get_pages() error path can reach unmap with npages == 0 and an
    uninitialised dma_addr array). Use a zeroing allocator and guard the
    direction argument with npages.
  - Do not route system pages to the device_unmap() callback on the IOVA
    path. Branch off addr->proto so system pages only get an explicit
    dma_unmap_page() in the non IOVA case, and only real device pages
    reach device_unmap().

Honglei Huang (2):
  drm/gpusvm: avoid reading uninitialized dma_addr[0].dir on IOVA unmap
  drm/gpusvm: do not route system pages to device_unmap() on IOVA unmap

 drivers/gpu/drm/drm_gpusvm.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/2] drm/gpusvm: avoid reading uninitialized dma_addr[0].dir on IOVA unmap

[Honglei Huang]

From: Honglei Huang <honghuan@amd.com>

__drm_gpusvm_unmap_pages() unconditionally reads
svm_pages->dma_addr[0].dir to pass the DMA direction to dma_iova_destroy()
whenever dma_use_iova() is true. On the error path of drm_gpusvm_get_pages()
the first dma_iova_link() may fail at i == 0, jumping to err_unmap with
num_dma_mapped == 0. As dma_addr came from a non zeroing allocator, dma_addr[0]
is then uninitialised, so the direction argument reads garbage. This trips
KMSAN and feeds a bogus dma_data_direction to the DMA layer.

Allocate the dma_addr array with the kvzalloc_objs() so every entry
has a correct value, and guard the direction argument
with npages so dma_iova_destroy() never dereferences dma_addr[0] when no
pages were mapped.

Reported-by: Sashiko AI review
Signed-off-by: Honglei Huang <honghuan@amd.com>
---
 drivers/gpu/drm/drm_gpusvm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index b2a848a7b2d..a9adf128eec 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -1224,10 +1224,17 @@ static void __drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm,
 		};
 		bool use_iova = dma_use_iova(&svm_pages->state);
 
+		/*
+		 * On the get_pages() error path npages can be 0 with
+		 * dma_addr[0] still unpopulated, while the IOVA reservation
+		 * already needs tearing down. Pass DMA_BIDIRECTIONAL in that
+		 * case instead of reading dma_addr[0].dir.
+		 */
 		if (use_iova)
 			dma_iova_destroy(dev, &svm_pages->state,
 					 svm_pages->state_offset,
-					 svm_pages->dma_addr[0].dir, 0);
+					 npages ? svm_pages->dma_addr[0].dir :
+						  DMA_BIDIRECTIONAL, 0);
 
 		for (i = 0, j = 0; i < npages; j++) {
 			struct drm_pagemap_addr *addr = &svm_pages->dma_addr[j];
@@ -1548,7 +1555,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
 		/* Unlock and restart mapping to allocate memory. */
 		drm_gpusvm_notifier_unlock(gpusvm);
 		svm_pages->dma_addr =
-			kvmalloc_objs(*svm_pages->dma_addr, npages);
+			kvzalloc_objs(*svm_pages->dma_addr, npages);
 		if (!svm_pages->dma_addr) {
 			err = -ENOMEM;
 			goto err_free;
-- 
2.34.1

Re: [PATCH 1/2] drm/gpusvm: avoid reading uninitialized dma_addr[0].dir on IOVA unmap

[sashiko-bot]

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

Pre-existing issues:
- [High] Device memory mappings created by `device_map` are leaked on the error path in `drm_gpusvm_get_pages()` because `svm_pages->dpagemap` is NULL.
--

commit ba7f1a24074336f55f3749b1bf75d6961df95930
Author: Honglei Huang <honghuan@amd.com>

drm/gpusvm: avoid reading uninitialized dma_addr[0].dir on IOVA unmap

This commit fixes an issue where an uninitialized DMA direction could be
passed to dma_iova_destroy() on the get_pages() error path. It guards
the direction argument with a check on npages and zeroes the dma_addr array.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 958cb605aedd5..2d74e2aae0a77 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -1146,10 +1146,17 @@ static void __drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm,
>  		};
>  		bool use_iova = dma_use_iova(&svm_pages->state);
>  
> +		/*
> +		 * On the get_pages() error path npages can be 0 with
> +		 * dma_addr[0] still unpopulated, while the IOVA reservation
> +		 * already needs tearing down. Pass DMA_BIDIRECTIONAL in that
> +		 * case instead of reading dma_addr[0].dir.
> +		 */
>  		if (use_iova)
>  			dma_iova_destroy(dev, &svm_pages->state,
>  					 svm_pages->state_offset,
> -					 svm_pages->dma_addr[0].dir, 0);
> +					 npages ? svm_pages->dma_addr[0].dir :
> +						  DMA_BIDIRECTIONAL, 0);
>  
>  		for (i = 0, j = 0; i < npages; j++) {
>  			struct drm_pagemap_addr *addr = &svm_pages->dma_addr[j];

[Severity: High]
This is a pre-existing issue, but does this error path leak device mappings?

When drm_gpusvm_get_pages() maps device pages successfully using
dpagemap->ops->device_map(), but then fails on a subsequent page (for
example, returning -EOPNOTSUPP because ctx->allow_mixed is false), it jumps
to the err_unmap label:

drm_gpusvm_get_pages() {
        ...
err_unmap:
        svm_pages->flags.has_dma_mapping = true;
        __drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);
}

However, svm_pages->dpagemap hasn't been assigned the local dpagemap yet.
When __drm_gpusvm_unmap_pages() runs, it reads the uninitialized
svm_pages->dpagemap as NULL.

This causes the cleanup loop in __drm_gpusvm_unmap_pages() to skip
device_unmap():

__drm_gpusvm_unmap_pages() {
        struct drm_pagemap *dpagemap = svm_pages->dpagemap;
        ...
        for (i = 0, j = 0; i < npages; j++) {
            ...
            } else if (dpagemap && dpagemap->ops->device_unmap) {
                dpagemap->ops->device_unmap(dpagemap, dev, addr);
            }
        }
}

Since dpagemap is NULL, device_unmap() is never called for the already
mapped pages. Can this permanently leak the device mappings?

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

[PATCH 2/2] drm/gpusvm: do not route system pages to device_unmap() on IOVA unmap

[Honglei Huang]

From: Honglei Huang <honghuan@amd.com>

In a mixed range: ctx->allow_mixed dpagemap is not NULL while some entries
are system pages. The unmap loop used:

        dma_unmap_page(...);
    else if (dpagemap && dpagemap->ops->device_unmap)
        dpagemap->ops->device_unmap(...);

When use_iova is true the first condition is false for system pages, so they
fall through to device_unmap() and a system DMA address is handed to the
device specific unmap callback, risking invalid accesses or state corruption.

Key the branch off addr->proto instead: system pages only need an explicit
dma_unmap_page() in the non IOVA case, IOVA system pages are already torn
down by the single dma_iova_destroy(), and only genuine device pages
reach device_unmap().

Reported-by: Sashiko AI review
Signed-off-by: Honglei Huang <honghuan@amd.com>
---
 drivers/gpu/drm/drm_gpusvm.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index a9adf128eec..96c14618591 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -1239,12 +1239,17 @@ static void __drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm,
 		for (i = 0, j = 0; i < npages; j++) {
 			struct drm_pagemap_addr *addr = &svm_pages->dma_addr[j];
 
-			if (!use_iova && addr->proto == DRM_INTERCONNECT_SYSTEM)
-				dma_unmap_page(dev,
-					       addr->addr,
-					       PAGE_SIZE << addr->order,
-					       addr->dir);
-			else if (dpagemap && dpagemap->ops->device_unmap)
+			if (addr->proto == DRM_INTERCONNECT_SYSTEM) {
+				/*
+				 * IOVA pages are cleaned by the ma_iova_destroy() above.
+				 * only the non IOVA mappings need unmap here.
+				 */
+				if (!use_iova)
+					dma_unmap_page(dev,
+						       addr->addr,
+						       PAGE_SIZE << addr->order,
+						       addr->dir);
+			} else if (dpagemap && dpagemap->ops->device_unmap)
 				dpagemap->ops->device_unmap(dpagemap,
 							    dev, addr);
 			i += 1 << addr->order;
-- 
2.34.1

Re: [PATCH 2/2] drm/gpusvm: do not route system pages to device_unmap() on IOVA unmap

[sashiko-bot]

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

Pre-existing issues:
- [High] IOVA address space is permanently leaked when unmapping mixed ranges because `dma_iova_try_alloc` allocates for the entire range but `dma_iova_destroy` only frees the linked portion.
--

commit 00ffc2a555c26b1b1afa96a3e7aabeee992b2163
Author: Honglei Huang <honghuan@amd.com>

drm/gpusvm: do not route system pages to device_unmap() on IOVA unmap

This commit fixes an issue in mixed ranges where system pages were incorrectly
routed to device_unmap() during IOVA unmap by checking the page protocol.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 2d74e2aae0a77..80b9a3f4cb919 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -1161,12 +1161,17 @@ static void __drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm,
>  		for (i = 0, j = 0; i < npages; j++) {
>  			struct drm_pagemap_addr *addr = &svm_pages->dma_addr[j];
>  
> -			if (!use_iova && addr->proto == DRM_INTERCONNECT_SYSTEM)
> -				dma_unmap_page(dev,
> -					       addr->addr,
> -					       PAGE_SIZE << addr->order,
> -					       addr->dir);
> -			else if (dpagemap && dpagemap->ops->device_unmap)
> +			if (addr->proto == DRM_INTERCONNECT_SYSTEM) {
> +				/*
> +				 * IOVA pages are cleaned by the ma_iova_destroy() above.

[Severity: High]
This isn't a bug introduced by this patch, but looking at how IOVA pages are
cleaned up here, does this leak IOVA address space for mixed ranges?

In drm_gpusvm_get_pages(), dma_iova_try_alloc() allocates IOVA space for the
entire range size:

    dma_iova_try_alloc(gpusvm->drm->dev, state, 0, npages * PAGE_SIZE);

However, when this range is unmapped in __drm_gpusvm_unmap_pages(),
dma_iova_destroy() is called with svm_pages->state_offset as the length:

    dma_iova_destroy(dev, &svm_pages->state,
                     svm_pages->state_offset,
                     npages ? svm_pages->dma_addr[0].dir :
                              DMA_BIDIRECTIONAL, 0);

If a user-space application allocates a range and migrates part of it to
device memory (creating a mixed range), state_offset will only be the sum
of the linked system page sizes.

Since dma_iova_destroy() only frees exactly the requested length, it seems
the unlinked portion of the original IOVA allocation is permanently leaked,
which could eventually exhaust the IOVA space.

> +				 */
> +				if (!use_iova)
> +					dma_unmap_page(dev,
> +						       addr->addr,
> +						       PAGE_SIZE << addr->order,
> +						       addr->dir);
> +			} else if (dpagemap && dpagemap->ops->device_unmap)
>  				dpagemap->ops->device_unmap(dpagemap,
>  							    dev, addr);
>  			i += 1 << addr->order;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627033325.3795298-1-honglei1.huang@amd.com?part=2