Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Mutex locked in test kthread but unlocked in executor thread via KUnit cleanup action, causing lockdep violations. - [Medium] Wrapping kunit_add_action() in KUNIT_ASSERT_EQ() causes resource leaks and assertion failures if action registration fails. --
--- Patch [2]: [PATCH 2/2] gpu/tests/gpu_buddy: Add KUnit test for gpu_buddy_allocated_addr_to_block --- commit 3fa726703b9976a0e41d2e06fbab74aae75245bd Author: Tejas Upadhyay <[email protected]> gpu/tests/gpu_buddy: Add KUnit test for gpu_buddy_allocated_addr_to_block Add a new KUnit test gpu_test_buddy_addr_to_block() that validates the gpu_buddy_allocated_addr_to_block() helper which traces a physical address back to its allocated buddy block. > diff --git a/drivers/gpu/tests/gpu_buddy_test.c > b/drivers/gpu/tests/gpu_buddy_test.c > index 89698563c61b2..1f4048d7c823e 100644 > --- a/drivers/gpu/tests/gpu_buddy_test.c > +++ b/drivers/gpu/tests/gpu_buddy_test.c [ ... ] > @@ -1422,6 +1422,103 @@ static void > gpu_test_buddy_alloc_exceeds_max_order(struct kunit *test) > + /* > + * Cleanup runs in LIFO order: > + * 4. free_blocks (lock held) > + * 3. mutex_unlock > + * 2. buddy_fini > + * 1. mutex_destroy > + */ > + mutex_lock(&mgr->lock); > + KUNIT_ASSERT_EQ(test, kunit_add_action(test, kunit_action_mutex_unlock, > &mgr->lock), 0); [Severity: Medium] Does this code result in the mutex being unlocked by a different thread than the one that locked it? KUnit executes test cases in a dedicated kthread to catch crashes and timeouts, but deferred cleanup actions registered via kunit_add_action() run in the parent executor thread after the test kthread exits. Because mgr->lock is acquired here in the test kthread, but the unlock is delegated to kunit_action_mutex_unlock(), the test kthread exits with the mutex held. The executor thread then attempts to unlock a mutex it does not own, triggering lockdep warnings for task exited with locks held and bad unlock balance. > + > + /* 3. Allocate blocks and register block cleanup action inside the lock > context */ > + KUNIT_ASSERT_EQ(test, gpu_buddy_alloc_blocks(&mgr->mm, 0, test_size, > alloc_size, > + chunk_size, > &mgr->allocated_list, 0), 0); > + KUNIT_ASSERT_EQ(test, kunit_add_action(test, kunit_action_free_blocks, > mgr), 0); [Severity: Medium] Can this sequence cause a resource leak and subsequent kernel panic if the action registration fails? kunit_add_action() dynamically allocates an action context using kzalloc_obj() and can fail with -ENOMEM. When wrapped in KUNIT_ASSERT_EQ(), a failure aborts the test kthread without running the action that just failed to register. Because the blocks were acquired before this registration attempt, they are leaked. When the previously registered kunit_action_buddy_fini runs during cleanup, it calls gpu_buddy_assert(mm->avail == mm->size), which will panic due to the leaked blocks. Should this use kunit_add_action_or_reset() instead to guarantee cleanup execution on failure? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
