Re: [PATCH net v2 1/2] net: do not run out of frags in alloc_skb_with_frags()

[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] alloc_skb_with_frags() (net/core/skbuff.c) can still fail with
  -ENOBUFS for a data_len that passed its up-front check, and the vhost-…

--

net: do not run out of frags in alloc_skb_with_frags()

This patch changes alloc_skb_with_frags() so that it no longer runs out
of frags when it rounds down the tail or falls back to a lower order.
When lowering the order would need more frags than are left, it retries
the same order once with the caller's direct reclaim and __GFP_NORETRY,
and fails if that retry also fails.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index a7d555b3d2f28..da9025e6f0c3c 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -6906,15 +6906,41 @@ struct sk_buff *alloc_skb_with_frags(unsigned long header_len,

[ ... ]

>  		if (order) {
> +			unsigned long smaller = PAGE_SIZE << (order - 1);
> +			bool can_shrink;
> +
> +			/* Only fall back to smaller pages if the frags left
> +			 * can still hold the rest. If not, try this order
> +			 * once more with the caller's direct reclaim.
> +			 */
> +			can_shrink = DIV_ROUND_UP(data_len, smaller) <=
> +				     MAX_SKB_FRAGS - nr_frags;
> +
>  			page = alloc_pages((gfp_mask & ~__GFP_DIRECT_RECLAIM) |
>  					   __GFP_COMP |
>  					   __GFP_NOWARN,
>  					   order);
> +			if (!page && !can_shrink &&
> +			    gfpflags_allow_blocking(gfp_mask))
> +				page = alloc_pages(gfp_mask | __GFP_COMP |
> +						   __GFP_NOWARN | __GFP_NORETRY,
> +						   order);
>  			if (!page) {
> +				if (!can_shrink)
> +					goto failure;

[Severity: Medium]
Can a guest still livelock the vhost worker through this path?

can_shrink is false for any data_len above
(MAX_SKB_FRAGS - nr_frags) * (PAGE_SIZE << (order - 1)). For the first
chunk with order 3 and 4K pages, that is anything above 278528 bytes.
If the attempt without direct reclaim and the __GFP_NORETRY retry both
fail, alloc_skb_with_frags() still returns -ENOBUFS. The data_len passed
the up-front check, and order-0 pages may be plentiful.

A guest can reach this size. tun_alloc_skb() clamps the paged part to
exactly the limit:

drivers/net/tun.c:tun_alloc_skb() {
    ...
	if (len - linear > MAX_SKB_FRAGS * (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
		linear = len - MAX_SKB_FRAGS * (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER);
    ...
}

So the guest can choose a paged length of up to 557056 bytes through the
descriptor chain. Tap and af_packet clamp to the same value.

vhost-net still treats -ENOBUFS as transient:

drivers/vhost/net.c:handle_tx_copy() {
    ...
			if (err == -EAGAIN || err == -ENOMEM || err == -ENOBUFS) {
				vhost_discard_vq_desc(vq, 1, ndesc);
				vhost_net_enable_vq(net, vq);
				break;
			}
    ...
}

handle_tx_zerocopy() also retries on -ENOBUFS.

On a host with fragmented memory, wouldn't this give the same 100% CPU
vhost spin with no traffic that the commit message describes?

Previously the order-3 attempt failed fast. Now every pass also does the
order-3 GFP_KERNEL | __GFP_NORETRY allocation. For this request,
__alloc_pages_slowpath() runs async compaction, one direct reclaim pass
and direct compaction before it gives up. Each pass can evict host page
cache and slab, and the guest controls how often this happens.

This seems to partly undo fb05e7a89f50 ("net: don't wait for order-3
page allocation").

The commit message only says "fail only if that fails too". It does not
say that this failure still livelocks vhost-net, or that the vhost retry
loop now runs direct reclaim. The note that the retry-failure case was
not tested appears only below the --- line, so it will not be in the
commit.

The KUnit test in the next patch does not change net/core/skbuff.c, so
this case is still there at the end of the series.

Could the commit message describe this remaining failure case?

>  				order--;
>  				continue;
>  			}

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