Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] KUNIT_ASSERT_* usage bypasses test cleanup on failure, permanently leaking memory and leaving a global trace probe registered. - [Medium] Shared test state is initialized after the global tracepoint probe is registered, creating a data race with concurrent kernel threads. - [Medium] Missing wake_up_var in the tracepoint probe causes a 10-second hang on test failure. --
--- Patch [4]: [PATCH v5 4/5] kunit: add test for refcount_final_put --- commit fef38efeda6f44aecbf042d2f2cbb9c0d0c7ccb4 Author: Eugene Mavick <[email protected]> kunit: add test for refcount_final_put Add a KUnit test suite for the refcount_final_put tracepoint. The test registers a probe function and triggers both refcount_t and percpu_ref final put paths, verifying that the tracepoint fires correctly and that the recorded fields match expected values. > diff --git a/lib/tests/refcount_trace_kunit.c > b/lib/tests/refcount_trace_kunit.c > new file mode 100644 > index 0000000000000..a57e9e4cfa427 > --- /dev/null > +++ b/lib/tests/refcount_trace_kunit.c > @@ -0,0 +1,141 @@ [ ... ] > +struct data { > + unsigned long caller; > + unsigned long ip; > + const void *obj; > + atomic_t count; > +}; > + > +struct data capture; > + > +const void *chk_obj; > + > +#define test_init() > \ > + do { > \ > + KUNIT_EXPECT_FALSE( > \ > + test, register_trace_refcount_final_put(probe, NULL)); > \ > + > \ > + atomic_set_release(&capture.count, 0); > \ > + > \ > + chk_obj = &obj; > \ > + } while (0) [Severity: Medium] Does registering this system-wide tracepoint before initializing capture.count and chk_obj create a data race? If a concurrent kernel thread triggers a refcount final put immediately after register_trace_refcount_final_put() succeeds, it will execute probe() and locklessly read chk_obj before the test thread initializes it here. Would it be safer to initialize the test state variables prior to registering the probe? > + > + > +#define test_exit() > \ > + do { > \ > + /* wait for probe completion */ > \ > + int notimeout = wait_var_event_timeout( > \ > + &capture.count, > \ > + atomic_read_acquire(&capture.count), > \ > + msecs_to_jiffies(10000) > \ > + ); > \ [Severity: Medium] Will this wait unconditionally hang for the full 10-second timeout on a test failure? The test thread waits here for capture.count to change, but looking at probe(), the state is incremented with atomic_inc_return_release() without a corresponding wake_up_var(&capture.count). If the tracepoint fails to fire synchronously, does the missing wakeup leave the thread permanently sleeping until the timeout expires? > + > \ > + unregister_trace_refcount_final_put(probe, NULL); > \ > + tracepoint_synchronize_unregister(); > \ > + > \ > + KUNIT_ASSERT_TRUE(test, notimeout); > \ [Severity: High] Is it safe to use KUNIT_ASSERT_TRUE() here before the test cleanup has finished? If this assertion fails (e.g. the wait times out), the test aborts immediately and skips the remainder of test_exit(). For tests like test_percpu_ref_put_many(), this means percpu_ref_exit() is never called, resulting in a memory leak. > + > \ > + KUNIT_EXPECT_EQ(test, atomic_read_acquire(&capture.count), 1); > \ [ ... ] > +static void test_percpu_ref_put_many(struct kunit *test) > +{ > + struct percpu_ref obj; > + > + test_init(); > + > + KUNIT_ASSERT_FALSE(test, percpu_ref_init(&obj, dummy_release, 0, > GFP_KERNEL)); [Severity: High] Does using KUNIT_ASSERT_FALSE() here risk leaving the global trace probe permanently registered? If percpu_ref_init() fails, KUNIT_ASSERT_FALSE() will abort the test execution and skip test_exit(). This leaves the probe function registered globally. When the KUnit module is later unloaded, any subsequent refcount final put in the system will invoke the freed probe function, causing a kernel panic. Should this test use kunit_add_action() for deferred cleanup to ensure the tracepoint is always unregistered? > + > + percpu_ref_get(&obj); > + percpu_ref_get(&obj); [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
