Skip to contentSkip to conversation

s2io: only arm hardware LSO for GSO skbs

PATCH In review Michael Pratte 2 participants 5 messages Jun 23

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 s2io: only arm hardware LSO for GSO skbs

s2io_xmit() enables the Xframe/Xframe-II hardware LSO (TCP segmentation) engine whenever the skb's gso_type carries SKB_GSO_TCPV4/TCPV6, and programs the segment size from gso_size:

	offload_type = s2io_offload_type(skb);
	if (offload_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)) {
		txdp->Control_1 |= TXD_TCP_LSO_EN;
		txdp->Control_1 |= TXD_TCP_LSO_MSS(s2io_tcp_mss(skb));
	}

Since commit 51466a7545b7 ("tcp: fill shinfo->gso_type at last moment") tcp_transmit_skb() sets skb_shinfo(skb)->gso_type unconditionally on every TCP skb, including non-GSO frames where gso_size is 0. The driver therefore arms the LSO engine with MSS == 0 for ordinary TCP segments such as the connection's SYN. The Xframe-II LSO engine treats an MSS of 0 as an illegal descriptor and aborts the transmit (lso_err_reg reports LSO6_ABORT), so the frame is dropped before it reaches the MAC. The result is that no TCP can be transmitted on these adapters since v4.2; UDP and ICMP (which never carry SKB_GSO_TCPV4) are unaffected.

Only arm the LSO engine when the skb is actually GSO (gso_size > 0), restoring the pre-4.2 behaviour. Non-GSO TCP frames take the normal transmit path.

Reproduced and fixed on Linux 6.6.67 with an Xframe-II adapter (PCI 17d5:5832); bisected to good v4.1.6 / bad v4.2.2.

Fixes 51466a7545b7 ("tcp: fill shinfo->gso_type at last moment")

---
[ Not upstream and cannot be: the s2io driver was removed from mainline in
commit aba0138eb7d7 ("net: ethernet: neterion: s2io: remove unused driver"). It still ships in the 6.6.y and 6.12.y stable trees, where this bug is present and the patch applies cleanly. Please apply there. ]

 drivers/net/ethernet/neterion/s2io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/neterion/s2io.c +1 -1
index 1e55ccb..9988ff9 100644
@@ -4105,7 +4105,7 @@ static netdev_tx_t s2io_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	offload_type = s2io_offload_type(skb);
-	if (offload_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)) {
+	if ((offload_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)) && skb_is_gso(skb)) {
 		txdp->Control_1 |= TXD_TCP_LSO_EN;
 		txdp->Control_1 |= TXD_TCP_LSO_MSS(s2io_tcp_mss(skb));
 	}
Signature · 1 line
2.54.0
#1 lore raw
Quoted 27 lines · Michael Pratte

s2io_xmit() enables the Xframe/Xframe-II hardware LSO (TCP segmentation) engine whenever the skb's gso_type carries SKB_GSO_TCPV4/TCPV6, and programs the segment size from gso_size:

	offload_type = s2io_offload_type(skb);
	if (offload_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)) {
		txdp->Control_1 |= TXD_TCP_LSO_EN;
		txdp->Control_1 |= TXD_TCP_LSO_MSS(s2io_tcp_mss(skb));
	}

Since commit 51466a7545b7 ("tcp: fill shinfo->gso_type at last moment") tcp_transmit_skb() sets skb_shinfo(skb)->gso_type unconditionally on every TCP skb, including non-GSO frames where gso_size is 0. The driver therefore arms the LSO engine with MSS == 0 for ordinary TCP segments such as the connection's SYN. The Xframe-II LSO engine treats an MSS of 0 as an illegal descriptor and aborts the transmit (lso_err_reg reports LSO6_ABORT), so the frame is dropped before it reaches the MAC. The result is that no TCP can be transmitted on these adapters since v4.2; UDP and ICMP (which never carry SKB_GSO_TCPV4) are unaffected.

Only arm the LSO engine when the skb is actually GSO (gso_size > 0), restoring the pre-4.2 behaviour. Non-GSO TCP frames take the normal transmit path.

Reproduced and fixed on Linux 6.6.67 with an Xframe-II adapter (PCI 17d5:5832); bisected to good v4.1.6 / bad v4.2.2.

Quoted 7 lines
Fixes 51466a7545b7 ("tcp: fill shinfo->gso_type at last moment")

---
[ Not upstream and cannot be: the s2io driver was removed from mainline in
commit aba0138eb7d7 ("net: ethernet: neterion: s2io: remove unused driver"). It still ships in the 6.6.y and 6.12.y stable trees, where this bug is present and the patch applies cleanly. Please apply there. ]

Why not just remove the driver in older kernels as well if it is not being used?

And if it's not being used, why is this patch needed at all?

thanks,

greg k-h

#2 lore raw
Quoted 2 lines · Greg Kroah-Hartman

Why not just remove the driver in older kernels as well if it is not being used?

It is being used - Xframe-II (17d5:5832) in a Supermicro X5DA8 on 6.6. Please keep it in stable.

Quoted 1 line

And if it's not being used, why is this patch needed at all?

It's used and broken: since v4.2 (51466a7545b7) s2io arms LSO with MSS=0 on every non-GSO TCP frame, so the card aborts all TCP TX - links fine, UDP/ICMP ok, but no TCP at all. The one-liner restores it.

Thanks,
Michael

Quoted 45 lines · Greg Kroah-Hartman
Quoted 27 lines · Michael Pratte

s2io_xmit() enables the Xframe/Xframe-II hardware LSO (TCP segmentation) engine whenever the skb's gso_type carries SKB_GSO_TCPV4/TCPV6, and programs the segment size from gso_size:

      offload_type = s2io_offload_type(skb);
      if (offload_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)) {
              txdp->Control_1 |= TXD_TCP_LSO_EN;
              txdp->Control_1 |= TXD_TCP_LSO_MSS(s2io_tcp_mss(skb));
      }

Since commit 51466a7545b7 ("tcp: fill shinfo->gso_type at last moment") tcp_transmit_skb() sets skb_shinfo(skb)->gso_type unconditionally on every TCP skb, including non-GSO frames where gso_size is 0. The driver therefore arms the LSO engine with MSS == 0 for ordinary TCP segments such as the connection's SYN. The Xframe-II LSO engine treats an MSS of 0 as an illegal descriptor and aborts the transmit (lso_err_reg reports LSO6_ABORT), so the frame is dropped before it reaches the MAC. The result is that no TCP can be transmitted on these adapters since v4.2; UDP and ICMP (which never carry SKB_GSO_TCPV4) are unaffected.

Only arm the LSO engine when the skb is actually GSO (gso_size > 0), restoring the pre-4.2 behaviour. Non-GSO TCP frames take the normal transmit path.

Reproduced and fixed on Linux 6.6.67 with an Xframe-II adapter (PCI 17d5:5832); bisected to good v4.1.6 / bad v4.2.2.

Quoted 7 lines
Fixes 51466a7545b7 ("tcp: fill shinfo->gso_type at last moment")

---
[ Not upstream and cannot be: the s2io driver was removed from mainline in
commit aba0138eb7d7 ("net: ethernet: neterion: s2io: remove unused driver"). It still ships in the 6.6.y and 6.12.y stable trees, where this bug is present and the patch applies cleanly. Please apply there. ]

Why not just remove the driver in older kernels as well if it is not being used?

And if it's not being used, why is this patch needed at all?

thanks,

greg k-h

#3 lore raw
Quoted 4 lines · Michael Pratte
Quoted 2 lines · Greg Kroah-Hartman

Why not just remove the driver in older kernels as well if it is not being used?

Quoted 8 lines

It is being used - Xframe-II (17d5:5832) in a Supermicro X5DA8 on 6.6. Please keep it in stable.

Quoted 1 line

And if it's not being used, why is this patch needed at all?

It's used and broken: since v4.2 (51466a7545b7) s2io arms LSO with MSS=0 on every non-GSO TCP frame, so the card aborts all TCP TX - links fine, UDP/ICMP ok, but no TCP at all. The one-liner restores it.

So what happens when 6.6.y goes end-of-life? Why is this driver removed if people are actually using it and willing to fix bugs in it?

I really don't want to take non-upstream patches for obvious reasons. And for code that has been removed already, that goes doubly so.

thanks,

greg k-h

#4 lore raw
Quoted 2 lines · Greg Kroah-Hartman

So what happens when 6.6.y goes end-of-life? Why is this driver removed if people are actually using it and willing to fix bugs in it?

It was removed as "unused," but I use it (Xframe-II in a Supermicro X5DA8) - which is what prompted this. I'll pursue getting it back in mainline instead: a revert of aba0138eb7d7 with the fix on top, which also addresses the EOL question.

Quoted 2 lines

I really don't want to take non-upstream patches for obvious reasons. And for code that has been removed already, that goes doubly so.

Understood - withdrawing the stable request; I'll send it to net-next as a revert + fix instead. Happy to test and fix issues as they come up.

Thanks,
Michael

Quoted 24 lines · Greg Kroah-Hartman
Quoted 4 lines · Michael Pratte
Quoted 2 lines · Greg Kroah-Hartman

Why not just remove the driver in older kernels as well if it is not being used?

Quoted 8 lines

It is being used - Xframe-II (17d5:5832) in a Supermicro X5DA8 on 6.6. Please keep it in stable.

Quoted 1 line

And if it's not being used, why is this patch needed at all?

It's used and broken: since v4.2 (51466a7545b7) s2io arms LSO with MSS=0 on every non-GSO TCP frame, so the card aborts all TCP TX - links fine, UDP/ICMP ok, but no TCP at all. The one-liner restores it.

So what happens when 6.6.y goes end-of-life? Why is this driver removed if people are actually using it and willing to fix bugs in it?

I really don't want to take non-upstream patches for obvious reasons. And for code that has been removed already, that goes doubly so.

thanks,

greg k-h