Re: [PATCH] drm/radeon: switch from get_user_pages to pin_user_pages

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing FOLL_LONGTERM flag in pin_user_pages for long-term DMA.

Pre-existing issues:
- [Critical] The error path in `radeon_ttm_tt_pin_userptr` leaks scatterlist entries and leaves a dangling pointer that triggers a Use-After-Free.
- [Critical] The return value of `radeon_ttm_tt_pin_userptr` is ignored, causing the driver to program the GPU GART with uninitialized memory.
--

commit 5aab225074cf5de663f6a8e7e139c7bfe602c69f
Author: Seongjun Hong <hsj0512@snu.ac.kr>

drm/radeon: switch from get_user_pages to pin_user_pages

This commit replaces get_user_pages with pin_user_pages to guarantee
that these DMA pages are pinned correctly.

> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index e7ab8162ac694..22fc35a0e8d81 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -349,7 +349,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_device *bdev, struct ttm_tt *ttm
>  		uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
>  		struct page **pages = ttm->pages + pinned;
>  
> -		r = get_user_pages(userptr, num_pages, write ? FOLL_WRITE : 0,
> +		r = pin_user_pages(userptr, num_pages, write ? FOLL_WRITE : 0,
>  				   pages);

[Severity: High]
Does this pinning operation require the FOLL_LONGTERM flag?

Documentation (Documentation/core-api/pin_user_pages.rst) dictates that
long-term DMA pins must use FOLL_LONGTERM. Without it, pinning memory
indefinitely in ZONE_MOVABLE or CMA regions might break memory compaction
and hotplug, potentially leading to CMA exhaustion.

>  		if (r < 0)
>  			goto release_pages;
[ ... ]
> @@ -377,7 +377,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_device *bdev, struct ttm_tt *ttm
>  	kfree(ttm->sg);

[Severity: Critical]
This is a pre-existing issue, but does this error path leak the scatterlist
and leave a dangling pointer?

If dma_map_sgtable() fails, we jump here and call kfree(ttm->sg). This
frees the container struct without calling sg_free_table(ttm->sg), leaking
the inner scatterlist array.

Additionally, ttm->sg is not set to NULL. When the object is later torn down,
radeon_ttm_tt_unpin_userptr() sees a non-NULL ttm->sg:

drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_tt_unpin_userptr() {
    ...
    /* double check that we don't free the table twice */
    if (!ttm->sg || !ttm->sg->sgl)
        return;

    /* free the sg table and pages again */
    dma_unmap_sgtable(rdev->dev, ttm->sg, direction, 0);
    ...
}

Could this lead to a use-after-free when it dereferences the freed pointer
and attempts to unmap it?

>  
>  release_pages:
> -	release_pages(ttm->pages, pinned);
> +	unpin_user_pages(ttm->pages, pinned);
>  	return r;
>  }

[Severity: Critical]
This is also a pre-existing issue, but is it safe to ignore the return value
of radeon_ttm_tt_pin_userptr() here?

In radeon_ttm_backend_bind():

drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_backend_bind() {
    ...
    if (gtt->userptr) {
        radeon_ttm_tt_pin_userptr(bdev, ttm);
        flags &= ~RADEON_GART_PAGE_WRITE;
    }

    r = radeon_gart_bind(rdev, gtt->offset, ttm->num_pages,
                         ttm->pages, gtt->ttm.dma_address, flags);
    ...
}

If radeon_ttm_tt_pin_userptr() fails (for example, if userspace provides an
invalid userptr), the failure is ignored. The ttm->dma_address array remains
uninitialized (zeroed) and is blindly passed to radeon_gart_bind().

Could this result in programming the hardware GART with zeroed DMA addresses,
effectively mapping physical address 0x0 for GPU read/write access?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827090100.428302-1-hsj0512@snu.ac.kr?part=1