Re: [PATCH net v2 2/2] net: add a KUnit test for the alloc_skb_with_frags() tail split

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The skip probe in alloc_skb_with_frags_test_tail()
  (net/core/net_test.c) cannot do what its comment says ("Don't report a…

--

net: add a KUnit test for the alloc_skb_with_frags() tail split

This adds a KUnit case that asks alloc_skb_with_frags() for
MAX_SKB_FRAGS - 2 order-3 chunks plus a 7 page tail. Before the fix it
references, splitting that tail ran out of frags. If the allocation fails
and a single order-3 page can't be allocated either, the test is meant to
skip rather than fail.

> diff --git a/net/core/net_test.c b/net/core/net_test.c
> index 2b5080c226636..de4d246f2f7ab 100644
> --- a/net/core/net_test.c
> +++ b/net/core/net_test.c
> @@ -1721,7 +1721,42 @@ static void netmem_ref_test_tcp_devmem_recv(struct kunit *test)

[ ... ]

> +	skb = alloc_skb_with_frags(0, len, PAGE_ALLOC_COSTLY_ORDER, &err,
> +				   GFP_KERNEL);
> +	if (!skb) {
> +		struct page *page;
> +
> +		/* Don't report a lack of high-order pages as a failure. */
> +		page = alloc_pages(GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY,
> +				   PAGE_ALLOC_COSTLY_ORDER);
> +		if (!page)
> +			kunit_skip(test, "no order-%d pages",
> +				   PAGE_ALLOC_COSTLY_ORDER);
> +		__free_pages(page, PAGE_ALLOC_COSTLY_ORDER);
> +	}
> +	KUNIT_ASSERT_NOT_NULL_MSG(test, skb, "err %d", err);

[Severity: Medium]
Does probing for a single order-3 page catch the case the comment
describes?

For this len, alloc_skb_with_frags() never lets a leading chunk drop
below order 3. At nr_frags = k, the check is:

net/core/skbuff.c:alloc_skb_with_frags() {
    ...
			can_shrink = DIV_ROUND_UP(data_len, smaller) <=
				     MAX_SKB_FRAGS - nr_frags;
    ...
}

This works out to 2*MAX_SKB_FRAGS - 2 - 2k <= MAX_SKB_FRAGS - k. That
holds only once k >= MAX_SKB_FRAGS - 2. So each of the first
MAX_SKB_FRAGS - 2 chunks has to get an order-3 page, or the function
does goto failure. In total the call needs roughly MAX_SKB_FRAGS - 1
order-3 pages, not one.

When the call fails partway through, the failure path does:

failure:
	kfree_skb(skb);
	return NULL;

This frees every order-3 page already placed in the frags. Since
pcp_allowed_order(3) is true, those pages land on this CPU's per-CPU
free lists right before the probe runs. The single
alloc_pages(..., PAGE_ALLOC_COSTLY_ORDER) probe will then almost always
succeed, the skip is not taken, and KUNIT_ASSERT_NOT_NULL_MSG() reports
a failure.

So the test seems to skip only when the very first order-3 allocation
fails. A host with some order-3 pages, but fewer than MAX_SKB_FRAGS - 1,
would still report a failure. That doesn't match "so that a host with
fragmented memory does not report this as a regression" in the commit
message.

Could the probe check the real requirement instead? For example, it
could try to hold MAX_SKB_FRAGS - 1 order-3 pages at once, or treat
-ENOBUFS as a skip unless a stronger check shows enough pages were
available.

> +
> +	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> +		frags_len += skb_frag_size(&skb_shinfo(skb)->frags[i]);
> +	KUNIT_EXPECT_EQ(test, frags_len, len);
> +
> +	kfree_skb(skb);
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110162418.28322-1-yanhaishuang%40cmss.chinamobile.com