Re: [PATCH net-next v2 7/8] net: skbuff: don't BUG() on a missing head_frag in skb_zerocopy()

[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 commit message calls `!from->head_frag && !hlen` in
  skb_zerocopy() a caller bug or "bad skb geometry".
- [Low] The patch adds a new -EFAULT return to skb_zerocopy() without
  updating its kernel-doc.

--

net: skbuff: don't BUG() on a missing head_frag in skb_zerocopy()

This patch replaces the BUG_ON() in skb_zerocopy(), which fires when @from
has no head_frag and hlen is 0. In its place is a DEBUG_NET_WARN_ON_ONCE()
and an -EFAULT return. The commit message describes the condition as bad
skb geometry passed in by the caller.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7fd2f8142cc41..629de22d98e48 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3913,7 +3913,10 @@ skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
>  	struct page *page;
>  	unsigned int offset;
>  
> -	BUG_ON(!from->head_frag && !hlen);
> +	if (unlikely(!from->head_frag && !hlen)) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return -EFAULT;
> +	}
>  
>  	/* dont bother with small payloads */
>  	if (len <= skb_tailroom(to))

[Severity: Medium]
Is this condition always a caller bug? The commit message says:

  "The check runs before anything is touched, and skb_zerocopy() already
  documents -EFAULT for bad skb geometry."

An ordinary zero-length packet from Open vSwitch seems able to reach it:

ovs_packet_cmd_execute()
  __dev_alloc_skb(NET_IP_ALIGN + len, GFP_KERNEL) /* kmalloc head */
  ... POP_ETH action on a 14 byte untagged frame
    pop_eth()->skb_eth_pop()->skb_pull_rcsum(skb, ETH_HLEN) /* len 0 */
  ... USERSPACE action
    output_userspace()->ovs_dp_upcall()->queue_userspace_packet()
      skb_zerocopy(user_skb, skb, 0, 0)

Because the allocation uses GFP_KERNEL, the head comes from kmalloc and
head_frag stays 0.

In queue_userspace_packet(), skb_len = min(skb->len, cutlen) is 0, and
hlen is 0 on both branches:

	if (dp->user_features & OVS_DP_F_UNALIGNED)
		hlen = min(skb_zerocopy_headlen(skb), cutlen);
	else
		hlen = skb_len;

When a !head_frag skb has no linear data, skb_zerocopy_headlen() returns
from->len, which is 0 here.

OVS_PACKET_CMD_EXECUTE uses GENL_UNS_ADMIN_PERM, so CAP_NET_ADMIN inside a
user namespace is enough to get here. With the old BUG_ON(), that made
this a crash that userspace could trigger.

Should the commit message describe this path and carry a Fixes: tag and a
stable note, so stable kernels pick up the change?

With len == 0, the small-payload fast path below always copies zero bytes
and returns 0. That is what an skb with head_frag already gets.

After this patch, the same request returns -EFAULT for kmalloc-head skbs.
The upcall is then dropped and counted in n_lost. On CONFIG_DEBUG_NET
kernels it also fires a WARN that userspace can trigger, and with
panic_on_warn set that WARN panics the machine.

nfqnl_build_packet_message() never hits this because it only calls
skb_zerocopy() when data_len is non-zero. queue_userspace_packet() has no
such guard.

Could the check move below the "len <= skb_tailroom(to)" fast path? Or
could it be enforced only when the head really has to be referenced as a
page, i.e. hlen == 0 && plen > 0?

[Severity: Low]
This isn't a bug, but the kernel-doc for skb_zerocopy() still gives only
these causes for -EFAULT:

 *	-EFAULT: skb_copy_bits() found some problem with skb geometry, or readable head
 *      payload would be mixed with unreadable frags.

The new early return gives -EFAULT before skb_copy_bits() or any frag
readability check runs.

Should the Return value section mention this case? It could also state the
precondition the old BUG_ON() enforced: hlen must be non-zero when @from
has no head_frag.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b%40toxicpanda.com