Re: [PATCH 05/10] drm/gem: Use dynamic allocation for GEM object dma_resv

[sashiko-bot@kernel.org]

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 <ckoenig.leichtzumerken@gmail.com>

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/20260827124910.2245-1-christian.koenig@amd.com?part=5