Re: [net PATCH] net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist

[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] In validate_xmit_skb() (net/core/dev.c), a non-fraglist
  CHECKSUM_UNNECESSARY GSO skb is relabelled CHECKSUM_NONE before…

--

net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist

This commit changes validate_xmit_skb() to relabel any non-fraglist
CHECKSUM_UNNECESSARY GSO skb, such as a hardware LRO aggregate sent out
through tc mirred, as CHECKSUM_NONE. The stated intent is that
netif_needs_gso() then segments these skbs in software. Without the
change, drivers that only set up TSO for CHECKSUM_PARTIAL send one
oversized frame with a bad checksum.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index f32353b0467cd..28b4a652d037f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4207,6 +4207,17 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
>  	if (unlikely(!skb))
>  		goto out_null;
>  
> +	/* Only fraglist GSO, whose segments already carry valid checksums,
> +	 * can be handed to a device as CHECKSUM_UNNECESSARY. Any other GSO
> +	 * skb has no csum_start for the device to segment from. NONE means
> +	 * the same on transmit, and has netif_needs_gso() segment it in
> +	 * software.
> +	 */
> +	if (unlikely(skb->ip_summed == CHECKSUM_UNNECESSARY &&
> +		     skb_is_gso(skb) &&
> +		     !(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST)))
> +		skb->ip_summed = CHECKSUM_NONE;
> +
>  	features = netif_skb_features(skb);

[Severity: Medium]

On a TSO-capable egress device, does this path actually reach software
segmentation?

ip_summed is already CHECKSUM_NONE when netif_skb_features() runs, so
the driver's ndo_features_check() sees a non-PARTIAL skb.
i40e_features_check(), ice_features_check() and fbnic_features_check()
all return early in that case and leave NETIF_F_GSO_MASK set:

drivers/net/ethernet/intel/i40e/i40e_main.c:i40e_features_check() {
	if (skb->ip_summed != CHECKSUM_PARTIAL)
		return features;
    ...
}

harmonize_features() also skips its can_checksum_protocol() test for
CHECKSUM_NONE.

netif_needs_gso() then returns true, and __skb_gso_segment() runs with
NETIF_F_TSO/TSO6 still in features. tcp4_gso_segment() (and
tcp6_gso_segment()) turns the skb back into CHECKSUM_PARTIAL:

net/ipv4/tcp_offload.c:tcp4_gso_segment() {
	if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) {
		...
		th->check = 0;
		skb->ip_summed = CHECKSUM_PARTIAL;
		__tcp_v4_send_check(skb, iph->saddr, iph->daddr);
	}

	return tcp_gso_segment(skb, features);
}

Since the device can do TSO, tcp_gso_segment() then returns NULL:

net/ipv4/tcp_offload.c:tcp_gso_segment() {
    ...
	if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
		...
		segs = NULL;
		goto out;
	}
    ...
}

validate_xmit_skb() treats NULL as "not segmented" and passes the
original skb to the driver as a CHECKSUM_PARTIAL TSO request.

If so, the new comment ("has netif_needs_gso() segment it in software")
and the commit message ("The netif_needs_gso function then segments it
in software") don't match what happens on most real NICs. fbnic
advertises NETIF_F_TSO/TSO6. Did the MSS-sized frames in the QEMU test
come from the emulated device's TSO rather than from software GSO?

A further concern is that the driver's TSO validation never sees the
skb in the form that is finally offloaded. i40e and ice drop GSO when
gso_size < 64 ("We cannot support GSO if the MSS is going to be less
than 64 bytes"). fbnic checks L2/L3 header length, alignment and
csum_offset limits. All of these checks are skipped for these skbs.

mlx5 LRO sets gso_size to an average over the aggregated segments:

drivers/net/ethernet/mellanox/mlx5/core/en_rx.c:mlx5e_build_rx_skb() {
    ...
	skb_shinfo(skb)->gso_size = DIV_ROUND_UP(cqe_bcnt - hdrlen, lro_num_seg);
    ...
}

Could a remote peer that sends small segments produce an aggregate that
gets redirected to i40e or ice as a TSO request with an MSS below 64?

DODGY CHECKSUM_NONE GSO skbs already take this bypass. This patch sends
every LRO-style CHECKSUM_UNNECESSARY TCP GSO skb through it as well.

Would one of these options give the documented behaviour?

- Clear NETIF_F_GSO_MASK from features for the converted skbs after
  netif_skb_features().
- Before netif_skb_features(), turn the skbs into proper
  CHECKSUM_PARTIAL skbs (cow the header, set csum_start and
  csum_offset, write the pseudo-header checksum). Then
  ndo_features_check() validates what is actually offloaded.

>  	skb = validate_xmit_vlan(skb, features);
>  	if (unlikely(!skb))

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179372577673.49909.11253315483074546903.stgit%40ahduyck-xeon-server.home.arpa