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 <[email protected]> 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/[email protected]?part=1
