Re: [RFC PATCH v1 3/5] drm/gpusvm: extract drm_gpusvm_dma_map_pages() helper

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Device memory mappings are permanently leaked on the error path due to deferred assignment of svm_pages->dpagemap
--

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

drm/gpusvm: extract drm_gpusvm_dma_map_pages() helper

This commit moves the per-device DMA mapping loop of drm_gpusvm_get_pages()
into a helper drm_gpusvm_dma_map_pages(). The mapping logic is only moved,
not changed, so there is no functional change.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 3496cb5688365..753361b965cf9 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
[ ... ]
> @@ -1695,17 +1631,114 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	/* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */
>  	WRITE_ONCE(svm_pages->flags.__flags, flags.__flags);
>  
> +	return 0;
> +
> +err_unmap:
> +	svm_pages->flags.has_dma_mapping = true;
> +	__drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);

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

When a device page is successfully mapped in drm_gpusvm_dma_map_pages(),
dpagemap->ops->device_map() is used. However, svm_pages->dpagemap is only
updated at the end of the function upon complete success.

If an error occurs on a subsequent page (such as returning -EOPNOTSUPP
due to encountering a mixed device memory mapping), the code jumps to
err_unmap and calls __drm_gpusvm_unmap_pages(). 

Because svm_pages->dpagemap is still NULL at this point, the unmap helper
will skip the cleanup callback:

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(...)
    ...
}

Should the local dpagemap be temporarily assigned to svm_pages->dpagemap
before calling the unmap helper, or is there another way to ensure these
partially mapped pages are properly cleaned up?

> +	return err;
> +}

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