Re: [RFC PATCH v1 2/5] drm/gpusvm: move dma_addr allocation before the notifier lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Device mappings are permanently leaked on the error path because `svm_pages->dpagemap` is assigned too late.
--

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

drm/gpusvm: move dma_addr allocation before the notifier lock

The dma_addr allocation was in a lazy allocation flow, it needs unlock
and goto map_pages. The allocation only needs npages, so just do it
before taking the lock. Drop the map_pages label and the relock flow, so
the sequence becomes fault, allocate, then lock, validate, map and
unlock. No functional change intended.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 507ef6f0a60e7..3496cb5688365 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
[ ... ]
> @@ -1575,18 +1583,6 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  		goto retry;
>  	}
>  
> -	if (!svm_pages->dma_addr) {
> -		/* Unlock and restart mapping to allocate memory. */
> -		drm_gpusvm_notifier_unlock(gpusvm);
> -		svm_pages->dma_addr =
> -			kvmalloc_objs(*svm_pages->dma_addr, npages);
> -		if (!svm_pages->dma_addr) {
> -			err = -ENOMEM;
> -			goto err_free;
> -		}
> -		goto map_pages;
> -	}
> -
>  	zdd = NULL;
>  	pagemap = NULL;
>  	num_dma_mapped = 0;

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

If dpagemap->ops->device_map() succeeds for some pages in
drm_gpusvm_get_pages(), but a subsequent mapping operation (like
dma_map_page() or dma_iova_link()) fails, the code jumps to err_unmap:

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

Inside __drm_gpusvm_unmap_pages(), it reads:

	struct drm_pagemap *dpagemap = svm_pages->dpagemap;

And later checks it before unmapping:

	else if (dpagemap && dpagemap->ops->device_unmap)
		dpagemap->ops->device_unmap(dpagemap, dev, addr);

However, back in drm_gpusvm_get_pages(), svm_pages->dpagemap is only
assigned at the very end of the success path:

	if (pagemap) {
		flags.has_devmem_pages = true;
		drm_pagemap_get(dpagemap);
		drm_pagemap_put(svm_pages->dpagemap);
		svm_pages->dpagemap = dpagemap;
	}

Does this mean that during the error path, svm_pages->dpagemap is NULL,
causing __drm_gpusvm_unmap_pages() to silently skip calling
dpagemap->ops->device_unmap() and permanently leak the mappings?

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