Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] Unconditional clearing of the output `*fence` in `amdgpu_ttm_clear_buffer` causes a guaranteed NULL pointer dereference in `amdgpu_bo_create`. --
commit f32c06ed28183e8d1c9d0dd64a83a68252d25f63 Author: Wentao Liang <[email protected]> drm/amdgpu: fix refcount leak in amdgpu_ttm_clear_buffer() This commit attempts to fix a fence reference leak in the error path of amdgpu_ttm_clear_buffer() by putting the fence and setting it to NULL. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 3d2e00efc7415..d65f1df3574fc 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c [ ... ] > @@ -2585,6 +2585,8 @@ int amdgpu_ttm_clear_buffer(struct amdgpu_bo *bo, > } > err: > mutex_unlock(&entity->lock); > + dma_fence_put(*fence); > + *fence = NULL; [Severity: Critical] Does this unconditionally clear the output fence even on the success path? The loop naturally falls through to the err: label when it completes successfully with r = 0. Since the cleanup isn't guarded by an error check, the output fence is always destroyed and set to NULL. If the clear operation is successful, won't this cause a NULL pointer dereference when the caller uses the fence? For example, in the buffer creation path: amdgpu_bo_create() { r = amdgpu_ttm_clear_buffer(bo, bo->tbo.base.resv, &fence); if (unlikely(r)) goto fail_unreserve; dma_resv_add_fence(bo->tbo.base.resv, fence, DMA_RESV_USAGE_KERNEL); } The caller will see r = 0 but have a NULL fence. Passing that NULL fence into dma_resv_add_fence() will cause an immediate NULL pointer dereference when it checks WARN_ON(dma_fence_is_container(fence)). Should this cleanup be guarded with if (r) to ensure we only drop the fence reference on actual failures? > > return r; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
