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

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

Paolo Abeni wrote:
> On 11/4/26 03:51, Willem de Bruijn wrote:
> > 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.
> I understand fraglist GSO has some use-case, but I'm really wondering if
> it's really worthy the pain?
> 
> I'll personally think we should consider removing it entirely.

If non-fraglist TSO packets with CHECKSUM_UNNECESSARY can be built and
drivers are incapable of handling them, this has to be fixed up.

The GSO stack trick of fixing up csum_start/csum_off and then handing off
the TSO packet to the driver is a neat solution.

Though that currently only works for TSO, not yet for USO.

Apparently with a bit deeper restructuring the check can be added without
additional overhead to the common hot path:

+/* Like netif_needs_gso(), but only fraglist GSO, whose segments already
+ * carry valid checksums, may reach the device as CHECKSUM_UNNECESSARY.
+ * Any other GSO skb has no csum_start for the device to segment from.
+ *
+ * The extra test sits behind skb_is_gso() and ip_summed != CHECKSUM_PARTIAL,
+ * which netif_needs_gso() evaluates anyway, so non-GSO and regular
+ * CHECKSUM_PARTIAL GSO skbs see no additional branches.
+ */
+static bool validate_xmit_needs_gso(const struct sk_buff *skb,
+                                   netdev_features_t features)
+{
+       if (!skb_is_gso(skb))
+               return false;
+       if (!skb_gso_ok(skb, features))
+               return true;
+       if (likely(skb->ip_summed == CHECKSUM_PARTIAL))
+               return false;
+       return skb->ip_summed != CHECKSUM_UNNECESSARY ||
+              !(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST);
+}
+

@@ -4222,10 +4243,18 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
        if (unlikely(!skb))
                goto out_null;

-       if (netif_needs_gso(skb, features)) {
+       if (validate_xmit_needs_gso(skb, features)) {
                unsigned int max_segs = 0;
                struct sk_buff *segs;

+               /* NONE means the same as UNNECESSARY on transmit, but has
+                * __skb_gso_segment() unshare the header before the protocol
+                * GSO code rewrites the L4 checksum and sets CHECKSUM_PARTIAL.
+                */
+               if (skb->ip_summed == CHECKSUM_UNNECESSARY &&
+                   !(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST))
+                       skb->ip_summed = CHECKSUM_NONE;
+
                if (unlikely(!gso_within_dev_limits(skb, dev)))
                        max_segs = skb_gso_output_max_segs(skb, dev);