Re: [PATCH] fix: drm/ttm/tests: use KUNIT_ASSERT_EQ for critical error checks to prevent kref underflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Using KUNIT_ASSERT_EQ aborts the test function on failure and bypasses required manual resource teardown.
- [Medium] Erroneous fix replacing KUNIT_EXPECT_EQ with KUNIT_ASSERT_EQ after ttm_bo_validate() causes a leaked BO reference and potential Use-After-Free.
--

commit af379b6fc27176bd6e80498788a4a528ca8f7559
Author: WenTao Liang <vulab@iscas.ac.cn>
fix: drm/ttm/tests: use KUNIT_ASSERT_EQ for critical error checks to prevent kref underflow

Replace KUNIT_EXPECT_EQ with KUNIT_ASSERT_EQ in ttm_bo_validate test cases
where subsequent ttm_bo_fini would cause a kref underflow if the preceding
ttm_bo_init_reserved (or ttm_bo_validate) failed. When those functions fail
they already release their internal references, leaving the refcount at 0.
Continuing to ttm_bo_fini without aborting performs an extra kref_put.

Cc: stable@vger.kernel.org
Fixes: 8bd1ff5ddc7b ("drm/ttm/tests: Test simple BO creation and validation")

> Replace KUNIT_EXPECT_EQ with KUNIT_ASSERT_EQ in ttm_bo_validate test cases
> where subsequent ttm_bo_fini would cause a kref underflow if the preceding
> ttm_bo_init_reserved (or ttm_bo_validate) failed. When those functions fail
> they already release their internal references, leaving the refcount at 0.

Is this description accurate regarding ttm_bo_validate()?

Looking at ttm_bo_validate(), it does not drop the BO reference on its error
paths. Only ttm_bo_init_reserved() does this, so a failure in
ttm_bo_validate() would leave the reference intact.

> 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 2db221f6fc3a1..7c4179f6349c5 100644
> --- a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
> +++ b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
[ ... ]
> @@ -249,7 +249,7 @@ static void ttm_bo_validate_basic(struct kunit *test)
>  	err = ttm_bo_init_reserved(priv->ttm_dev, bo, params->bo_type,
>  				   fst_placement, PAGE_SIZE, &ctx_init, NULL,
>  				   NULL, &dummy_ttm_bo_destroy);
> -	KUNIT_EXPECT_EQ(test, err, 0);
> +	KUNIT_ASSERT_EQ(test, err, 0);

[Severity: Medium]
Will this leak the mock managers on failure?

Because KUNIT_ASSERT_EQ() aborts the test function immediately upon failure,
if ttm_bo_init_reserved() fails here, execution skips the
ttm_mock_manager_fini() cleanup at the end of ttm_bo_validate_basic().

This could leave dangling mock managers in the global state.

This pattern is also present in ttm_bo_validate_same_placement(),
ttm_bo_validate_all_pinned_evict(), and ttm_bo_validate_allowed_only_evict(),
where manual cleanups and mock manager teardowns would also be skipped.

[ ... ]
> @@ -840,7 +840,7 @@ static void ttm_bo_validate_happy_evict(struct kunit *test)
>  	err = ttm_bo_validate(bo_val, placement, &ctx_val);
>  	ttm_bo_unreserve(bo_val);
>  
> -	KUNIT_EXPECT_EQ(test, err, 0);
> +	KUNIT_ASSERT_EQ(test, err, 0);

[Severity: Medium]
Does this skip necessary cleanup and leak the buffer object?

As noted above, ttm_bo_validate() does not drop the BO reference on failure.
If it fails here, aborting the test skips the mandatory ttm_bo_fini() loops
at the end of ttm_bo_validate_happy_evict().

While the test array is freed by KUnit on test exit, the leaked elements
might still be referenced by the DRM device's VMA manager. This could
potentially cause a use-after-free when the device is destroyed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626145409.49370-1-vulab@iscas.ac.cn?part=1