Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] Uninitialized `dma_resv` for internal Nouveau buffer objects causes NULL pointer dereferences and memory leaks. - [Critical] Massive memory leak of dynamically allocated `dma_resv` objects for all i915 GEM objects. - [High] Memory leak of `dma_resv` object in TTM ghost objects due to missing `dma_resv_put()` in `ttm_transfered_destroy()`. - [Medium] KUnit test runtime crash and compilation failure due to comparing `struct dma_resv *` with `struct dma_resv **`. --
commit ea9a73424b175ca45f706102b9727e26c5e65f0b Author: Christian König <[email protected]> drm/gem: Use dynamic allocation for GEM object dma_resv This patch removes the embedded dma_resv from struct drm_gem_object and replaces it with a dynamically allocated reservation object. It updates GEM object initialization to allocate the object and cleans up references appropriately. It also updates TTM and driver-specific code to handle the dynamically allocated individual_resv. > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c > b/drivers/gpu/drm/i915/gem/i915_gem_object.c > index 384c74794fdc1..5e48e169f2de0 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -144,7 +144,6 @@ void __i915_gem_object_fini(struct drm_i915_gem_object > *obj) > { > mutex_destroy(&obj->mm.get_page.lock); > mutex_destroy(&obj->mm.get_dma_page.lock); > - dma_resv_put(&obj->base._resv); > } [Severity: Critical] Does this code completely leak the dynamically allocated dma_resv object for all i915 GEM objects? Since the custom destruction logic in __i915_gem_object_fini() no longer calls dma_resv_put() for the base GEM resv object, and drm_gem_private_object_fini() is bypassed by i915, it seems the resv allocated in drm_gem_private_object_init() is permanently lost upon destruction. [ ... ] > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c > b/drivers/gpu/drm/nouveau/nouveau_bo.c > index 67c9d32f4f27b..65e1f0c2f984d 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c [ ... ] > @@ -385,7 +383,6 @@ nouveau_bo_new(struct nouveau_cli *cli, u64 size, int > align, > return PTR_ERR(nvbo); > > nvbo->bo.base.size = size; > - dma_resv_init(&nvbo->bo.base._resv); > drm_vma_node_reset(&nvbo->bo.base.vma_node); [Severity: Critical] Does this leave the dma_resv pointer uninitialized (NULL) for internal Nouveau buffer objects? Because nouveau_bo_alloc() uses kzalloc() and bypasses standard GEM initialization, bo->base.resv remains NULL without this initialization. Later, when ttm_bo_init_reserved() evaluates WARN_ON(!dma_resv_trylock(bo->base.resv)), won't this result in an immediate NULL pointer dereference because bo->base.resv is NULL? [ ... ] > diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c > b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c > index bb430763e7a52..6fc381a9d2da6 100644 > --- a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c > +++ b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c > @@ -637,7 +637,7 @@ static void > ttm_bo_validate_no_placement_not_signaled(struct kunit *test) > KUNIT_EXPECT_EQ(test, ctx.bytes_moved, 0); > > if (bo->type != ttm_bo_type_sg) > - KUNIT_ASSERT_PTR_EQ(test, bo->base.resv, &bo->base._resv); > + KUNIT_ASSERT_PTR_EQ(test, bo->base.resv, &bo->individual_resv); [Severity: Medium] Can this compile cleanly or pass at runtime? The variable bo->individual_resv is now a pointer to struct dma_resv. By using the address-of operator here, this assertion attempts to compare a struct dma_resv pointer with a double pointer, which will unconditionally fail the test and break the KUnit suite. [ ... ] > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c > b/drivers/gpu/drm/ttm/ttm_bo_util.c > index af5732f9e489e..7457a16b744f5 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c > @@ -207,7 +207,7 @@ static void ttm_transfered_destroy(struct > ttm_buffer_object *bo) > struct ttm_transfer_obj *fbo; > > fbo = container_of(bo, struct ttm_transfer_obj, base); > - dma_resv_put(&fbo->base.base._resv); > + dma_resv_put(fbo->base.individual_resv); > ttm_bo_put(fbo->bo); > kfree(fbo); > } [Severity: High] Does this code permanently leak the dma_resv object dynamically allocated for a ghost object? In ttm_buffer_object_transfer(), the ghost object acquires an extra reference to fbo->base.base.resv: fbo->base.individual_resv = dma_resv_alloc(); ... fbo->base.base.resv = dma_resv_get(fbo->base.individual_resv); However, when the ghost object is destroyed, ttm_bo_release() sets bo->individual_resv to NULL before calling the destroyer callback. As a result, when ttm_transfered_destroy() calls dma_resv_put(fbo->base.individual_resv), it is operating on a NULL pointer. This leaves the reference on the base GEM dma_resv unreleased, causing a memory leak during buffer evictions. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
