Re: [PATCH v5 4/5] drm/gpusvm: move struct drm_gpusvm_pages out of struct drm_gpusvm_range

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Passing uninitialized DMA direction to `dma_iova_destroy()` when mapping fails on the first iteration.
- [High] Device memory DMA mappings are permanently leaked if `drm_gpusvm_get_pages()` fails midway through mapping.
--

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

drm/gpusvm: move struct drm_gpusvm_pages out of struct drm_gpusvm_range

Since the physical pages and MM VA range have been abstractly separated,
this commit moves struct drm_gpusvm_pages out of struct drm_gpusvm_range
to bring flexibility to the drm gpu SVM framework. It updates helpers
and documentation to operate directly on driver-embedded pages objects.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 85fbadc9716a4..6b79aecb22dbc 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c

[ ... ]

> @@ -183,7 +232,10 @@
>   *				goto retry;
>   *		}
>   *
> - *		err = drm_gpusvm_range_get_pages(gpusvm, range, &ctx);
> + *		err = drm_gpusvm_get_pages(gpusvm, &drange->pages,
> + *					   gpusvm->mm, &range->notifier->notifier,
> + *					   drm_gpusvm_range_start(range),
> + *					   drm_gpusvm_range_end(range), &ctx);

[Severity: High]
This is a pre-existing issue, but passing an uninitialized DMA direction
to dma_iova_destroy() can occur if mapping fails on the first iteration.

In drm_gpusvm_get_pages(), the dma_addr array is allocated without
zeroing. If dma_iova_link() fails on the very first page (i=0):

drivers/gpu/drm/drm_gpusvm.c:drm_gpusvm_get_pages() {
        ...
        if (dma_use_iova(state)) {
                err = dma_iova_link(...);
                if (err)
                        goto err_unmap;
        }
        ...
}

The code jumps to err_unmap and calls __drm_gpusvm_unmap_pages(), which
dereferences the uninitialized dir parameter:

drivers/gpu/drm/drm_gpusvm.c:__drm_gpusvm_unmap_pages() {
        ...
        if (use_iova)
                dma_iova_destroy(dev, &svm_pages->state,
                                 svm_pages->state_offset,
                                 svm_pages->dma_addr[0].dir, 0);
}

Could this lead to undefined behavior in the DMA API during IOVA teardown?


[Severity: High]
This isn't a bug introduced by this patch, but it appears device memory DMA
mappings are permanently leaked if drm_gpusvm_get_pages() fails midway
through mapping.

In drm_gpusvm_get_pages(), svm_pages->dpagemap is only populated upon
complete success:

drivers/gpu/drm/drm_gpusvm.c:drm_gpusvm_get_pages() {
        ...
        if (pagemap) {
                flags.has_devmem_pages = true;
                drm_pagemap_get(dpagemap);
                drm_pagemap_put(svm_pages->dpagemap);
                svm_pages->dpagemap = dpagemap;
        }
        ...
}

If a failure occurs (e.g. DMA mapping error) after successfully mapping one
or more device pages, the code jumps to err_unmap. Because
svm_pages->dpagemap remains NULL on this error path, the unmap loop skips
calling device_unmap():

drivers/gpu/drm/drm_gpusvm.c:__drm_gpusvm_unmap_pages() {
        struct drm_pagemap *dpagemap = svm_pages->dpagemap;
        ...
        else if (dpagemap && dpagemap->ops->device_unmap)
                dpagemap->ops->device_unmap(dpagemap, dev, addr);
}

Could this exhaust mapping resources over time by permanently leaking
these mappings?

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