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

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

[Alexander Duyck]

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,
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>
---
 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;
+
 	features = netif_skb_features(skb);
 	skb = validate_xmit_vlan(skb, features);
 	if (unlikely(!skb))

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

[Willem de Bruijn]

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))
> 
> 

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

[Paolo Abeni]

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.

/P

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

[Willem de Bruijn]

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);

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

[Alexander Duyck]

On Wed, Nov 4, 2026 at 8:24 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> 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);
>

The problem with that is the order of operations. The issue I am
attempting to address is that there was an e1000e patch that was
trying to make changes to ndo_features_check to address this. That is
when I realized there are numerous drivers that would be encountering
the same issue.

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

[Alexander Duyck]

On Wed, Nov 4, 2026 at 8:34 AM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Wed, Nov 4, 2026 at 8:24 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > 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);
> >
>
> The problem with that is the order of operations. The issue I am
> attempting to address is that there was an e1000e patch that was
> trying to make changes to ndo_features_check to address this. That is
> when I realized there are numerous drivers that would be encountering
> the same issue.

One alternative we could probably look at would be to modify something
like gso_features_check to strip all the GSO features except for
SKB_GSO_FRAGLIST if CHECKSUM_UNECESSARY is set. Thoughts?

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

[Willem de Bruijn]

Alexander Duyck wrote:
> On Wed, Nov 4, 2026 at 8:34 AM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> >
> > On Wed, Nov 4, 2026 at 8:24 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > 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);
> > >
> >
> > The problem with that is the order of operations. The issue I am
> > attempting to address is that there was an e1000e patch that was
> > trying to make changes to ndo_features_check to address this. That is
> > when I realized there are numerous drivers that would be encountering
> > the same issue.
> 
> One alternative we could probably look at would be to modify something
> like gso_features_check to strip all the GSO features except for
> SKB_GSO_FRAGLIST if CHECKSUM_UNECESSARY is set. Thoughts?

That would revert to fully software segmentation right?

It would be preferable to be able to keep TSO, just reset the csum_..
fields.

Though if this is a rare enough path, taking the performance cost there
rather than burdening all other workloads makes sense too.

But I think the current fix is preferable.

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

[netdev-bot+sashiko]

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

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

[Alexander Duyck]

On Thu, Nov 5, 2026 at 9:11 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> 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.

Based on earlier conversation the behavior as documented of actually
doing this completely in software would be suboptimal. If we can just
fixup the packets and then send them on then to be segmented by the
hardware this would be preferred.

The original behavior was that these frames were being passed to the
hardware without setting up segmentation. After this change we set the
proper checksum offsets and values and then segment in the hardware.

Based on the fact that GSO_DODGY skbs already had this issue it isn't
an issue that is introduced by this change. If anything it causes the
two to now fall into the same bucket. If needed one thing I could look
at doing would be to also look at setting this bit on the frames that
are coming in and being re-routed to the Tx from a source that is
running LRO.