Re: [PATCH net-next 5/5] net: drop GSO skbs instead of handing them to XDP

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · 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).

- [High] Regression in tun/tap with native XDP attached: guest and
  vnet_hdr TSO/USO traffic is now silently dropped.
- [Medium] netdevsim with XDP attached now drops TCP TSO traffic from its
  peer.
- [Medium] veth: explicitly dropped GSO packets bypass kfree_skb
  tracepoints
- [Low] The patch describes and fixes a user-visible bug: generic XDP is
  handed GRO-coalesced/GSO skbs, e.g. generic XDP on bond0 over GRO-…
- [Low] In the generic XDP path, the new GSO check sits in
  bpf_prog_run_generic_xdp(), which runs after…

--

net: drop GSO skbs instead of handing them to XDP

This commit adds xdp_skb_feature_check(). It drops GSO skbs, with a
ratelimited warning, before they can reach an XDP program. The check is
called from bpf_prog_run_generic_xdp() and veth_convert_skb_to_xdp_buff().
The motivation is stacked devices such as bonding, and generic XDP on veth,
where GRO-coalesced packets can still get to the program.

> These precautions are not 100% today, and IMHO fixing
> that is impossible. Let's explicitly drop GSO skbs on input to XDP.

[ ... ]

> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

[Severity: Low]
The commit message describes a bug that exists today: generic XDP on bond0
over GRO-enabled slaves, and generic XDP on veth. There is no Fixes: tag,
though. Should one be added if this is meant for net?

The change also affects callers that send GSO skbs to generic XDP on
purpose, such as tun and netdevsim. An origin reference would help stable
backporters decide where the change applies.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5c67e555f82d6..8c9dcb4126bd3 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[ ... ]

> @@ -5551,6 +5552,9 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>  	u32 metalen, act;
>  	int off;
>  
> +	if (unlikely(xdp_skb_feature_check(skb)))
> +		return XDP_DROP;
> +

[Severity: Medium]
Will anyone chasing lost packets be able to see this drop?

The new branch jumps to the existing drop label. That label frees the
skb with consume_skb() and sets *pskb to NULL. consume_skb() fires the
consume_skb tracepoint, not kfree_skb, so dropwatch, perf on
skb:kfree_skb and similar tools never see these packets.

The caller, veth_xdp_rcv_skb(), bumps stats->rx_drops and then calls
kfree_skb(skb) on its drop path. By then skb is already NULL, so that
call is a no-op and the drop is still reported as a normal consume.

The generic XDP call site behaves differently. There the XDP_DROP
returned by bpf_prog_run_generic_xdp() reaches the do_drop label in
netif_receive_generic_xdp(), which uses kfree_skb(). So the two callers
of xdp_skb_feature_check() report the same policy drop in two different
ways. Returning -ENOMEM is also misleading, since nothing failed to
allocate here.

The consume_skb() at the drop label predates this patch; it covers the
skb_pp_cow_data() failure. That is arguably an error path as well.
Would it make sense to free with kfree_skb_reason() for the GSO case?
A dedicated drop reason for GSO skbs reaching XDP could then be used by
both veth and the generic XDP path.

[Severity: High]
Does this break tun/tap when a native XDP program is attached?

tun sends packets whose virtio_net_hdr has a gso_type to the generic XDP
path on purpose, because its native XDP can't handle GSO:

drivers/net/tun.c:tun_build_skb() {
    ...
	if (hdr->gso_type || !xdp_prog) {
		*skb_xdp = 1;
		return __tun_build_skb(tfile, alloc_frag, buf, buflen, len,
				       pad, metasize);
    ...
}

The comment in tun_get_user() documents this:

		/* For the packet that is not easy to be processed
		 * (e.g gso or jumbo packet), we will do it at after
		 * skb was created with generic XDP routine.
		 */

Once tun_vnet_hdr_tnl_to_skb() has set gso_size, the path becomes:

tun_get_user()
  do_xdp_generic()
    netif_receive_generic_xdp()
      bpf_prog_run_generic_xdp()
        xdp_skb_feature_check() -> XDP_DROP

The vhost-net batched path in tun_xdp_one() does the same:

	if (xdp_prog) {
		if (gso->gso_type) {
			skb_xdp = true;
			goto build;
		}
    ...
	if (skb_xdp) {
		ret = do_xdp_generic(xdp_prog, &skb);
		if (ret != XDP_PASS) {
			ret = 0;
			goto out;

tun treats any result other than XDP_PASS as consumed. tun_get_user()
returns total_len and tun_xdp_one() returns 0. QEMU or vhost therefore sees
success, but every guest TSO/USO frame is dropped. This happens even when
the program only returns XDP_PASS.

Unlike veth_xdp_set(), tun_xdp_set() does nothing to stop the writer from
producing GSO frames. Guest TX TSO is negotiated between the guest and the
VMM, so the admin has no receive offload to turn off. That makes the
"receive offloads are still on" warning misleading here.

The commit message says driver XDP "is not an issue", but tun's native XDP
depends on generic XDP for GSO frames. Would tun need its own handling,
such as segmenting or exempting these skbs?

[Severity: Medium]
Could this also blackhole TCP bulk traffic on netdevsim when XDP is
attached?

nsim_setup() advertises NETIF_F_TSO. nsim_start_xmit() only calls
skb_linearize() before forwarding the skb to the peer, so gso_size is
kept. The peer then runs its program through do_xdp_generic():

drivers/net/netdevsim/netdev.c:nsim_rcv() {
    ...
			if (skb->ip_summed == CHECKSUM_PARTIAL)
				skb_checksum_help(skb);
			ret = do_xdp_generic(xdp_prog, &skb);
			if (ret != XDP_PASS) {
				dev_dstats_rx_add(dev, skblen);
				continue;
    ...
}

netdevsim never clears TSO on the peer when XDP is installed. Every TSO
skb would now get XDP_DROP here and still be counted as received.

For CHECKSUM_PARTIAL GSO skbs, this combination already triggered
skb_warn_bad_offload() via skb_checksum_help() before this patch. The
existing ping.py TCP subtests only send a short string, so they are
unlikely to catch this.

[Severity: Low]
This isn't a bug, but should the check move to the top of
netif_receive_generic_xdp()?

GRO/GSO skbs are almost always nonlinear. By the time
bpf_prog_run_generic_xdp() runs, netif_receive_generic_xdp() has already
done this:

	if (skb_cloned(skb) || skb_is_nonlinear(skb) ||
	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
		if (netif_skb_check_for_xdp(pskb, xdp_prog))
			goto do_drop;
	}

If the program lacks xdp_has_frags, or the page pool copy would be too
large, netif_skb_check_for_xdp() falls back to
pskb_expand_head(..., GFP_ATOMIC) plus skb_linearize(). That means a
high-order atomic allocation and a full copy of a 64KB (or BIG TCP sized)
aggregate, and the skb is then dropped right away.

The veth hunk already places the check before skb_pp_cow_data().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928223648.2739371-1-kuba%40kernel.org