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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> The netif_needs_gso function lets a GSO skb reach the device unsegmented
> when it is CHECKSUM_PARTIAL or CHECKSUM_UNNECESSARY. The second case was
> added for FCoE, whose transmit path marked CRC offload that way at the
> time. Commit 253aab0597d9 ("fcoe: Use CHECKSUM_PARTIAL to indicate CRC
> offload") moved FCoE back to CHECKSUM_PARTIAL but left the exemption in
> place, with nothing left that needed it. Fraglist GSO, added later, is
> its only user today, as its segments already carry valid checksums.
> 
> A TCP GSO skb can still arrive as CHECKSUM_UNNECESSARY without being
> fraglist. Hardware LRO builds its aggregates that way, for example mlx5
> sets SKB_GSO_TCPV4 or SKB_GSO_TCPV6 and CHECKSUM_UNNECESSARY, without
> SKB_GSO_DODGY. IP forwarding and bridging turn LRO off, but a tc mirred
> redirect does not, so such an skb can be sent out another device. It
> has no csum_start and its TCP checksum field is whatever the aggregate
> left there. Drivers that only set up TSO for CHECKSUM_PARTIAL, such as
> fbnic, i40e and ice, then send the whole skb as one oversized frame
> with a bad checksum.
> 
> Have the validate_xmit_skb function mark any CHECKSUM_UNNECESSARY GSO
> skb other than fraglist as CHECKSUM_NONE, which means the same on
> transmit. The netif_needs_gso function then segments it in software,

tcp[46]_gso_segment convert to CHECKSUM_PARTIAL and then
tcp_gso_segment will send it on to hardware if skb_gso_ok with
NETIF_F_GSO_ROBUST.

> and the __skb_gso_segment function unshares the header before the
> protocol code rewrites the L4 checksum field. The callers of the
> netif_needs_gso function that decide on segmentation for themselves,
> such as tap, are left as they are.
> 
> Found by code inspection. In QEMU with an emulated fbnic, TCPv4 and
> TCPv6 GSO skbs built this way each went out as a single 11 KB frame
> with a bad TCP checksum. With this change they are segmented into
> MSS-sized frames with valid checksums.
> 
> Fixes: 253aab0597d9 ("fcoe: Use CHECKSUM_PARTIAL to indicate CRC offload")
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

With the above aside. Not important enough to respin imho.

> ---
>  net/core/dev.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index f32353b0467c..28b4a652d037 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;
> +

It's unfortunate that we need to add yet another branch in the hot path.
But I also do not see a good alternative.

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