Re: [PATCH net-next 8/9] 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 3 potential
issue(s) to consider.

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

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] skb_zerocopy() now WARNs and returns -EFAULT on a legitimate,
  user-reachable input: an empty (len == 0) skb without head_frag.
- [Low] The kernel-doc 'Return value' section of skb_zerocopy() lists
  exactly two causes of -EFAULT: 'skb_copy_bits() found some problem…
- [Low] The patch removes a BUG() in skb_zerocopy() that userspace can
  actually reach, but the commit message has no Fixes: tag (the first…

--

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

This commit replaces the BUG_ON(!from->head_frag && !hlen) check in
skb_zerocopy() with a WARN_ON_ONCE() that returns -EFAULT. The commit
message describes the condition as a caller bug. It also says -EFAULT is
already documented for bad skb geometry.

> skb_zerocopy() BUG()s if @from has no head_frag and the caller passed
> hlen == 0, meaning the caller didn't ask for the head to be copied and
> the head can't be referenced as a page either.  The check runs before
> anything is touched, and skb_zerocopy() already documents -EFAULT for
> bad skb geometry.  Warn once and return that.

[Severity: Low]
Should this carry a Fixes: tag?

Userspace can reach the BUG() removed here through openvswitch.
OVS_PACKET_CMD_EXECUTE only needs GENL_UNS_ADMIN_PERM. A 14-byte Ethernet
frame with the actions POP_ETH and then USERSPACE ends up here:

ovs_packet_cmd_execute()
  __dev_alloc_skb(NET_IP_ALIGN + len, GFP_KERNEL)  /* head_frag == 0 */
  ...
  do_execute_actions()
    pop_eth()                    /* skb->len goes from 14 to 0 */
    output_userspace()
      ovs_dp_upcall()
        queue_userspace_packet()
          skb_zerocopy(user_skb, skb, 0, 0)
            BUG_ON(!from->head_frag && !hlen)

There are two likely candidates for the tag. One is the commit that added
the BUG_ON() to skb_zerocopy(). The other is the commit that made POP_ETH
followed by USERSPACE produce empty upcalls.

The first patch in this series, "net: skbuff: don't leave stale bytes in
skb_copy_and_csum_bits()", has a Fixes: tag.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index e29eda2eaf3f6..8c6a45a20eb0f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3906,7 +3906,8 @@ 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 (WARN_ON_ONCE(!from->head_frag && !hlen))
> +		return -EFAULT;

[Severity: Medium]
Can this WARN fire on a valid empty copy?

When len == 0 there is nothing to copy, so hlen == 0 is not a caller bug
in that case. The kernel-doc names skb_zerocopy_headlen() as the source
of @hlen, and it returns 0 for an empty skb without head_frag:

net/core/skbuff.c:skb_zerocopy_headlen() {
    ...
	if (!from->head_frag || ...) {
		hlen = skb_headlen(from);
		if (!hlen)
			hlen = from->len;
	}
    ...
}

The openvswitch path above hits this case. queue_userspace_packet()
computes skb_len = min(skb->len, cutlen), which is 0. hlen is also 0 on
both the OVS_DP_F_UNALIGNED path and the aligned path. The skb was
allocated with GFP_KERNEL, so head_frag is 0.

If the check came after the small payload fast path, this call would
simply return 0:

	if (len <= skb_tailroom(to))
		return skb_copy_bits(from, 0, skb_put(to, len), len);

Because the check comes first, userspace can trigger a WARN splat (or a
panic with panic_on_warn), and the empty upcall fails with -EFAULT.

Would adding len && to the condition, or moving the check below the fast
path, avoid this?

[Severity: Low]
Should the skb_zerocopy() kernel-doc be updated as well? Its return value
section lists only two causes of -EFAULT:

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

The new return happens before skb_copy_bits() is called and has nothing
to do with unreadable frags. The @hlen description ("size of linear
headroom in destination buffer") also doesn't say that hlen must be
non-zero when @from has no head_frag.

The commit message says skb_zerocopy() "already documents -EFAULT for
bad skb geometry". The doc only uses that wording for skb_copy_bits()
failures.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-b4-skbuff-bug-on-v1-0-1b4434c5357c%40toxicpanda.com