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 <[email protected]> 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: [email protected] 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/[email protected]?part=1
