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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net/kunit: kernel panic in TCP devmem test due to unset skb->unreadable
--

commit 432a5016a6d079e3237437e79dd79f25313c4f7a
Author: Mina Almasry <almasrymina@google.com>

net: kunit: test netmem, page_pool, and skb frag refcounting
    
Verify that non-pp (page._refcount / binding->ref) and page_pool
(pp_ref_count) references stay balanced without cross-counter leaks or
underflows across regular pages, page_pool pages, non-pp net_iovs, and
page_pool net_iovs with both pp_recycle=0 and pp_recycle=1.

> 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)
[ ... ]
> +static void netmem_ref_test_tcp_devmem_recv(struct kunit *test)
> +{
[ ... ]
> +	/* 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);

[Severity: Medium]
Does this code need to set skb->unreadable = 1 when injecting this devmem
fragment?

Without it, tcp_recvmsg_locked() checks skb_frags_readable(skb) and returns
true. The TCP stack will attempt to copy the payload using
skb_copy_datagram_msg() instead of processing it via tcp_recvmsg_dmabuf().

This leads to a kernel panic when __skb_datagram_iter() executes
skb_frag_foreach_page() and retrieves the page pointer via netmem_to_page().
Because the underlying memory is a net_iov, netmem_to_page() returns NULL and
the subsequent kmap_local_page(NULL) call crashes the kernel during test
execution.

> +
> +	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);
> +	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);
[ ... ]
> +	/* 2. Receive second packet & destroy socket to test
> +	 * tcp_release_user_frags() -> napi_pp_put_page().
> +	 */
> +	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 = 64;
> +	TCP_SKB_CB(skb)->end_seq = 128;
> +	skb_queue_tail(&sock->sk->sk_receive_queue, skb);

[Severity: Medium]
Will this second injected SKB also trigger the same panic since it lacks
skb->unreadable = 1?

> +
> +	msg.msg_control = cmsg_buf;
> +	msg.msg_controllen = sizeof(cmsg_buf);
> +	ret = kernel_recvmsg(sock, &msg, &iov, 0, 64,
> +			     MSG_SOCK_DEVMEM | MSG_DONTWAIT);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012082148.1698596-1-almasrymina@google.com?part=6