Re: [PATCH net-next 07/15] net: skbuff: let skb_zerocopy() copy a short skb without head_frag

[netdev-bot+sashiko@xxxxxxxxxx]

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 Fixes: tag points at the wrong commit, so stable kernels
  will not get this fix even though they still have a BUG() that an…

--

net: skbuff: let skb_zerocopy() copy a short skb without head_frag

This patch moves the !from->head_frag && !hlen check in skb_zerocopy()
below the small-payload skb_copy_bits() path. A short copy from an skb
without a head_frag, including a zero-length one, now succeeds. The
motivating case is an Open vSwitch OVS_PACKET_CMD_EXECUTE request doing
POP_ETH followed by USERSPACE, which ends up calling
skb_zerocopy(user_skb, skb, 0, 0).

Link: 
https://lore.kernel.org/all/179173804997.434549.4450793967361922984@xxxxxxxxxx/

> That used to BUG(), and now the
> upcall fails and the packet is counted as lost, with a WARN on
> CONFIG_DEBUG_NET kernels.  OVS_PACKET_CMD_EXECUTE only needs
> CAP_NET_ADMIN in the user namespace that owns the network namespace,
> so with the openvswitch module loaded any user can do this from their
> own namespace.

[ ... ]

> Fixes: 433380f5ab8f ("net: skbuff: don't BUG() on a missing head_frag in 
> skb_zerocopy()")

[Severity: Medium]
Is this the right commit for the Fixes: tag?

433380f5ab8f only turned the existing BUG_ON() into an -EFAULT return.
It went in through net-next as part of the "net: skbuff: replace most
BUG_ON()s with error returns" series, with no Fixes: or Cc: stable tag,
so no stable tree has it.

The ordering problem is older than that. Before 433380f5ab8f,
skb_zerocopy() looked like this:

    BUG_ON(!from->head_frag && !hlen);

    /* dont bother with small payloads */
    if (len <= skb_tailroom(to))
        return skb_copy_bits(from, 0, skb_put(to, len), len);

The BUG_ON() has been in that spot since af2806f8f90a1 ("net: Export
skb_zerocopy() to zerocopy from one skb to another") in 2013.

On those kernels, the path in the commit message reaches the BUG_ON():

ovs_packet_cmd_execute()
  __dev_alloc_skb(NET_IP_ALIGN + len, GFP_KERNEL)   /* head_frag == 0 */
  ... POP_ETH leaves skb->len == 0 ...
  ovs_dp_upcall()
    queue_userspace_packet()
      skb_zerocopy(user_skb, skb, 0, 0)
        BUG_ON(!from->head_frag && !hlen)

With the current tag, stable tooling would only pick this up for trees
that already return -EFAULT. Wouldn't every stable tree without
433380f5ab8f keep a BUG() that a user can trigger from their own
namespace?

Should this carry Fixes: af2806f8f90a1 instead, or the OVS Ethernet
push/pop commit that made an empty-skb upcall reachable? It should also
probably have Cc: stable and go through net rather than net-next.

A stable backport would also need to move the old BUG_ON() below the
len <= skb_tailroom(to) return, since those trees don't have the
-EFAULT conversion.

-- 
Sashiko AI review · 
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-b4-skbuff-bug-on-followups-v1-0-41dbef50b9f7%40toxicpanda.com