Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] The error path in `radeon_ttm_tt_pin_userptr()` incorrectly frees `ttm->sg` using `kfree()` without setting the pointer to NULL and without properly freeing the inner scatterlist array if it was allocated. This leads to a memory leak, dangling pointer, use-after-free, double-put of pages, and double-free during the object teardown sequence. -- commit 4389e2026f1a66251ba9951cbc3ed34d9181fa4f Author: Seongjun Hong <[email protected]> drm/radeon: check radeon_ttm_tt_pin_userptr() return value in bind This commit propagates the error from radeon_ttm_tt_pin_userptr() in radeon_ttm_backend_bind() to avoid writing stale or uninitialized DMA addresses to the GPU's GART page table. > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c > b/drivers/gpu/drm/radeon/radeon_ttm.c > index e7ab8162ac694..1b72cac9ed6b6 100644 > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > @@ -431,7 +431,9 @@ static int radeon_ttm_backend_bind(struct ttm_device > *bdev, > return 0; > > if (gtt->userptr) { > - radeon_ttm_tt_pin_userptr(bdev, ttm); > + r = radeon_ttm_tt_pin_userptr(bdev, ttm); > + if (r) > + return r; [Severity: Critical] This is a pre-existing issue, but by returning the error here, we trigger the TTM object teardown sequence which exposes memory management bugs in the existing error handling paths. If radeon_ttm_tt_pin_userptr() fails at dma_map_sgtable() (for instance), it does this: drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_tt_pin_userptr() { ... r = dma_map_sgtable(rdev->dev, ttm->sg, direction, 0); if (r) goto release_sg; ... release_sg: kfree(ttm->sg); release_pages: release_pages(ttm->pages, pinned); return r; } This frees ttm->sg without setting it to NULL, leaving a dangling pointer. It also leaks the internal scatterlist nodes instead of using sg_free_table(). Because we now return the error from radeon_ttm_backend_bind(), TTM initiates teardown via ttm_tt_unpopulate() -> radeon_ttm_tt_unpopulate() -> radeon_ttm_tt_unbind() -> radeon_ttm_tt_unpin_userptr(). In radeon_ttm_tt_unpin_userptr(), we have a use-after-free read: 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; ... } It also does a double-put on the pages, as they were already released in the initial error path: drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_tt_unpin_userptr() { ... put_page(page); ... } Finally, radeon_ttm_tt_unpopulate() causes a double-free of ttm->sg: drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_tt_unpopulate() { ... if (gtt && gtt->userptr) { kfree(ttm->sg); ... } Can we update the error paths in radeon_ttm_tt_pin_userptr() and related teardown functions to safely handle these failure cases before propagating the error here? > flags &= ~RADEON_GART_PAGE_WRITE; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
