Re: [PATCH net-next v3 6/6] net: kunit: test netmem, page_pool, and skb frag refcounting

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new netmem refcount KUnit cases do not register deferred
  cleanup (for example with kunit_add_action_or_reset()).

--

net: kunit: test netmem, page_pool, and skb frag refcounting

This commit adds KUnit cases to net/core/net_test.c. They check that page
refcounts, devmem binding refs and pp_ref_count stay balanced for regular
pages, page_pool pages and devmem net_iovs across many skb helpers. It
also exports skb_shift() via EXPORT_SYMBOL_IF_KUNIT() so the test links
when built as a module.

> diff --git a/net/core/net_test.c b/net/core/net_test.c
> index bc2ef7612e7df..2bb01430a89f7 100644
> --- a/net/core/net_test.c
> +++ b/net/core/net_test.c
> @@ -457,10 +457,1231 @@ static void ip_tunnel_flags_test_run(struct kunit *test)

[ ... ]

> +#ifdef CONFIG_INET
> +static void netmem_ref_test_tcp_devmem_recv(struct kunit *test)
> +{

[ ... ]

> +	pool = page_pool_create(&pp_params);
> +	if (IS_ERR(pool)) {
> +		sock->sk->sk_state = TCP_CLOSE;
> +		sock_release(sock);
> +		KUNIT_ASSERT_FALSE(test, IS_ERR(pool));
> +	}
> +
> +	ret = percpu_ref_init(&binding.ref, dummy_percpu_ref_release,
> +			      PERCPU_REF_INIT_ATOMIC, GFP_KERNEL);
> +	if (ret) {
> +		page_pool_destroy(pool);
> +		sock->sk->sk_state = TCP_CLOSE;
> +		sock_release(sock);
> +		KUNIT_ASSERT_EQ(test, ret, 0);
> +	}

[ ... ]

> +	/* 1. Receive via tcp_recvmsg_dmabuf -> SO_DEVMEM_DONTNEED release */
> +	skb = alloc_skb(0, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, skb);

[ ... ]

> +	ret = kernel_recvmsg(sock, &msg, &iov, 0, 64,
> +			     MSG_SOCK_DEVMEM | MSG_DONTWAIT);
> +	KUNIT_ASSERT_EQ(test, ret, 64);

[Severity: Low]

If this KUNIT_ASSERT_EQ() fails, KUnit aborts the case and the teardown at
the end of netmem_ref_test_tcp_devmem_recv() never runs. Does that leak
the kernel TCP socket, the page_pool and the percpu_ref data?

The page_pool_create() and percpu_ref_init() failure paths above call
sock_release() and page_pool_destroy() before they assert. The later
checks do not:

    KUNIT_ASSERT_NOT_NULL(test, skb);
    ...
    KUNIT_ASSERT_EQ(test, ret, 64);

This applies to both the first and the second receive. On those paths the
socket's receive queue or sk_user_frags may still point at the on-stack
niov.

The other new cases follow the same pattern:

netmem_ref_test_page() leaks the page if alloc_skb() fails.

netmem_ref_test_pp_page() and netmem_ref_test_skb_segment() leak the
pool, its pages, reg_page and the percpu_ref if any of their
KUNIT_ASSERT_*() checks trip.

Could the teardown be registered with kunit_add_action_or_reset() so it
also runs when an assertion fails?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014173945.373295-1-almasrymina%40google.com