* [net PATCH] net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist
@ 2026-11-03 17:09 Alexander Duyck
2026-11-04 2:51 ` Willem de Bruijn
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Duyck @ 2026-11-03 17:09 UTC (permalink / raw)
To: netdev
Cc: andrew+netdev, davem, edumazet, horms, kernel-team, kuba, pabeni,
tactii
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))
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [net PATCH] net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist
2026-11-03 17:09 [net PATCH] net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist Alexander Duyck
@ 2026-11-04 2:51 ` Willem de Bruijn
2026-11-04 8:02 ` Paolo Abeni
0 siblings, 1 reply; 6+ messages in thread
From: Willem de Bruijn @ 2026-11-04 2:51 UTC (permalink / raw)
To: Alexander Duyck, netdev
Cc: andrew+netdev, davem, edumazet, horms, kernel-team, kuba, pabeni,
tactii
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))
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net PATCH] net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist
2026-11-04 2:51 ` Willem de Bruijn
@ 2026-11-04 8:02 ` Paolo Abeni
2026-11-04 16:24 ` Willem de Bruijn
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2026-11-04 8:02 UTC (permalink / raw)
To: Willem de Bruijn, Alexander Duyck, netdev
Cc: andrew+netdev, davem, edumazet, horms, kernel-team, kuba, tactii
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net PATCH] net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist
2026-11-04 8:02 ` Paolo Abeni
@ 2026-11-04 16:24 ` Willem de Bruijn
2026-11-04 16:34 ` Alexander Duyck
0 siblings, 1 reply; 6+ messages in thread
From: Willem de Bruijn @ 2026-11-04 16:24 UTC (permalink / raw)
To: Paolo Abeni, Willem de Bruijn, Alexander Duyck, netdev
Cc: andrew+netdev, davem, edumazet, horms, kernel-team, kuba, tactii
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net PATCH] net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist
2026-11-04 16:24 ` Willem de Bruijn
@ 2026-11-04 16:34 ` Alexander Duyck
2026-11-04 16:39 ` Alexander Duyck
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Duyck @ 2026-11-04 16:34 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Paolo Abeni, netdev, andrew+netdev, davem, edumazet, horms,
kernel-team, kuba, tactii
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net PATCH] net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist
2026-11-04 16:34 ` Alexander Duyck
@ 2026-11-04 16:39 ` Alexander Duyck
0 siblings, 0 replies; 6+ messages in thread
From: Alexander Duyck @ 2026-11-04 16:39 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Paolo Abeni, netdev, andrew+netdev, davem, edumazet, horms,
kernel-team, kuba, tactii
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-11-04 16:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-03 17:09 [net PATCH] net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist Alexander Duyck
2026-11-04 2:51 ` Willem de Bruijn
2026-11-04 8:02 ` Paolo Abeni
2026-11-04 16:24 ` Willem de Bruijn
2026-11-04 16:34 ` Alexander Duyck
2026-11-04 16:39 ` Alexander Duyck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox