Re: [PATCH net 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 2 potential
issue(s) to consider.

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

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 new test alloc_skb_with_frags_test_tail() in
  net/core/net_test.c only passes if every full-size high-order…
- [Low] The commit message of "net: add a KUnit test for the
  alloc_skb_with_frags() tail split" says the allocation failed with…

--

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

This commit adds a KUnit case, alloc_skb_with_frags_test_tail(), to
net/core/net_test.c. The test asks alloc_skb_with_frags() for
MAX_SKB_FRAGS - 2 full PAGE_ALLOC_COSTLY_ORDER chunks plus a 7 page tail.
It then asserts that the allocation succeeds and that the frag sizes add
up to the requested length.

> Allocate MAX_SKB_FRAGS - 2 full chunks of PAGE_ALLOC_COSTLY_ORDER plus
> a 7 page tail. Rounding the tail down to 4 + 2 + 1 pages would need
> one frag more than MAX_SKB_FRAGS, which made the allocation fail with
> -ENOBUFS before the previous patch.

[Severity: Low]
This isn't a bug, but could the commit message name the fix this test
depends on, instead of saying "the previous patch"?

Once the series is merged, "the previous patch" no longer points to
anything in git history or in stable trees. The test fails on any tree
that lacks "net: do not run out of frags in alloc_skb_with_frags()".
Nothing in this commit tells a backporter or AUTOSEL about that
dependency.

Would it help to reference that commit by its subject (or sha once it
is merged)?

> diff --git a/net/core/net_test.c b/net/core/net_test.c
> index 2b5080c226636..5a451aaf37c9b 100644
> --- a/net/core/net_test.c
> +++ b/net/core/net_test.c
> @@ -1721,7 +1721,31 @@ static void netmem_ref_test_tcp_devmem_recv(struct kunit *test)
>  #endif
>  #endif
>  
> +static void alloc_skb_with_frags_test_tail(struct kunit *test)
> +{
> +	/* MAX_SKB_FRAGS - 2 full chunks plus a 7 page tail: rounding the
> +	 * tail down to 4 + 2 + 1 pages would need one frag too many.
> +	 */
> +	unsigned long len = (MAX_SKB_FRAGS - 2) *
> +			    (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER) +
> +			    7 * PAGE_SIZE;
> +	unsigned long frags_len = 0;
> +	struct sk_buff *skb;
> +	int err, i;
> +
> +	skb = alloc_skb_with_frags(0, len, PAGE_ALLOC_COSTLY_ORDER, &err,
> +				   GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL_MSG(test, skb, "err %d", err);

[Severity: Medium]
Can this assertion fail on a system with fragmented memory, even with
the fix applied?

For order > 0, alloc_skb_with_frags() in net/core/skbuff.c clears
__GFP_DIRECT_RECLAIM. A failed order-3 allocation then drops the order
for good:

net/core/skbuff.c:alloc_skb_with_frags() {
    ...
		if (nr_frags == MAX_SKB_FRAGS)
			goto failure;
    ...
		if (order) {
			page = alloc_pages((gfp_mask & ~__GFP_DIRECT_RECLAIM) |
					   __GFP_COMP |
					   __GFP_NOWARN,
					   order);
			if (!page) {
				order--;
				continue;
			}
    ...
}

The tail branch never raises the order again, because order_base_2(pages)
and ilog2(pages) are never above the current order.

This length needs MAX_SKB_FRAGS - 1 frags at order 3, which leaves one
frag spare. Take MAX_SKB_FRAGS = 17 with 4K pages:

  len = 15 * 32K + 28K
  14 order-3 chunks succeed, then the 15th fails -> order 2
  60K left, three 16K frags -> nr_frags == 17, 12K still left
  goto failure -> NULL, err = -ENOBUFS

So a failure of any of the full-size order-3 chunks leaves too few frags
for the rest of the data. Only a failure of the final tail allocation
can recover, through the hweight_long()/order_base_2() branch.

When this happens, the assertion prints "err -105". That is the same
symptom the commit message gives for the unfixed kernel, so a failure
caused by fragmentation looks like the regression the test is meant to
catch.

NET_TEST is tristate, so the test can be loaded with modprobe on a
long-running host. With 64K pages, each full chunk is 512K of
contiguous memory.

Would it be more reliable to call kunit_skip() when the order-3
allocations cannot be satisfied? Another option is to build the skb so
the page sizes are fixed, rather than asserting that the allocation
always succeeds.

> +
> +	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/20261108100617.23840-1-yanhaishuang%40cmss.chinamobile.com