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

[netdev-bot+sashiko]

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] None of the new test cases register KUnit cleanup actions.

--

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

This adds KUnit cases to net/core/net_test.c. They check that non-pp
refcounts and page_pool pp_ref_count stay balanced across regular pages,
page_pool pages, and net_iovs, through the various skb frag 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 9c3a590865d26..6f21e6baf7fb3 100644
> --- a/net/core/net_test.c
> +++ b/net/core/net_test.c
> @@ -370,10 +370,1213 @@ static void ip_tunnel_flags_test_run(struct kunit *test)

[ ... ]

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

[ ... ]

> +	KUNIT_ASSERT_EQ(test,
> +			sock_create_kern(&init_net, AF_INET, SOCK_STREAM,
> +					 IPPROTO_TCP, &sock),
> +			0);
> +	sock->sk->sk_state = TCP_ESTABLISHED;
> +	inet_csk(sock->sk)->icsk_ack.rcv_mss = 64;
> +	tcp_sk(sock->sk)->copied_seq = 0;
> +
> +	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);
> +	skb->pp_recycle = 1;
> +	skb_add_rx_frag_netmem(skb, 0, niov_nm, 0, 64, PAGE_SIZE);
> +	napi_pp_get_page(niov_nm);
> +	TCP_SKB_CB(skb)->seq = 0;
> +	TCP_SKB_CB(skb)->end_seq = 64;
> +	skb_queue_tail(&sock->sk->sk_receive_queue, skb);
> +
> +	msg.msg_control = cmsg_buf;
> +	msg.msg_controllen = sizeof(cmsg_buf);
> +	ret = kernel_recvmsg(sock, &msg, &iov, 0, 64,
> +			     MSG_SOCK_DEVMEM | MSG_DONTWAIT);
> +	KUNIT_ASSERT_EQ(test, ret, 64);

[Severity: Low]
If this assert fails, is the kernel TCP socket leaked along with the pool
and the percpu_ref data of the on-stack binding?

The page_pool_create() and percpu_ref_init() failure paths above release
everything by hand before asserting. The later asserts do not, which
includes this one, the KUNIT_ASSERT_NOT_NULL(test, skb) checks, and the
second KUNIT_ASSERT_EQ(test, ret, 64) after the second kernel_recvmsg().
None of sock_release(), page_pool_destroy() or percpu_ref_exit() run on
those paths.

The leaked socket's sk_receive_queue or sk_user_frags can still refer to
the on-stack niov. Nothing touches them afterwards, though, because the
socket is never released.

Other new cases follow the same pattern:

  netmem_ref_test_page(): the alloc_skb() assert runs while the test
  still owns the page from alloc_page().

  netmem_ref_test_pp_page(): the page_pool_dev_alloc_pages() assert
  skips page_pool_destroy().

  netmem_ref_test_release_data(): the percpu_ref_init() assert runs
  while the pp page still holds extra refs, so the pool and page leak.

  netmem_ref_test_skb_segment(): the reg_page, percpu_ref_init() and
  skb allocation asserts come after the pool and pp_page are set up,
  with no cleanup.

Could these use kunit_add_action_or_reset() or similar? Then teardown
would still happen when an assert aborts the case.

> +	KUNIT_EXPECT_EQ(test, atomic_long_read(&niov.desc.pp_ref_count), 3L);
> +	KUNIT_EXPECT_EQ(test, atomic_long_read(&binding.ref.data->count),
> +			base_iov_ref);

[ ... ]

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