Re: [PATCH v5 4/5] kunit: add test for refcount_final_put

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