sunvnet: don't change gso data on clones

This patch unclones an skb for the case where the sunvnet driver needs to change the segmentation size so that it doesn't interfere with TCP SACK's use of them.

---
 drivers/net/ethernet/sun/sunvnet.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

index 2b10b85..22e0cad 100644
@@ -1192,23 +1192,16 @@ static int vnet_handle_offloads(struct vnet_port *port, struct sk_buff *skb)
 	skb_pull(skb, maclen);
 
 	if (port->tso && gso_size < datalen) {
+		if (skb_unclone(skb, GFP_ATOMIC))
+			goto out_dropped;
+
 		/* segment to TSO size */
 		skb_shinfo(skb)->gso_size = datalen;
 		skb_shinfo(skb)->gso_segs = gso_segs;
-
-		segs = skb_gso_segment(skb, dev->features & ~NETIF_F_TSO);
-
-		/* restore gso_size & gso_segs */
-		skb_shinfo(skb)->gso_size = gso_size;
-		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - hlen,
-							 gso_size);
-	} else
-		segs = skb_gso_segment(skb, dev->features & ~NETIF_F_TSO);
-	if (IS_ERR(segs)) {
-		dev->stats.tx_dropped++;
-		dev_kfree_skb_any(skb);
-		return NETDEV_TX_OK;
 	}
+	segs = skb_gso_segment(skb, dev->features & ~NETIF_F_TSO);
+	if (IS_ERR(segs))
+		goto out_dropped;
 
 	skb_push(skb, maclen);
 	skb_reset_mac_header(skb);

@@ -1246,6 +1239,10 @@ static int vnet_handle_offloads(struct vnet_port *port, struct sk_buff *skb)
 	if (!(status & NETDEV_TX_MASK))
 		dev_kfree_skb_any(skb);
 	return status;
+out_dropped:
+	dev->stats.tx_dropped++;
+	dev_kfree_skb_any(skb);
+	return NETDEV_TX_OK;
 }
 
 static int vnet_start_xmit(struct sk_buff *skb, struct net_device *dev)

1.7.1

Hmm... this would point to a TCP bug ?

What happens if not GSO is needed, TCP corrupts data currently read/processed by NIC/driver ?

TCP should have required skb_unclone() where needed.

I don't think I understand your concern. This problem can result in a panic using sunvnet because the sunvnet driver is changing the original skb, which is always, or at least almost always, a clone. TCP uses gso_segs to track packet counts, so changing it in the driver can result in bad math-- TCP assumes its copy of the clone's data shouldn't change (of course).

A driver that doesn't change the segmentation or original data doesn't need to care whether it's a clone or not-- it'll free it and drop a reference. Since sunvnet is changing the gso_size and gso_segs, it needs to unclone first.

						+-DLS

Well, we had a very hard to find bug in TCP stack, I want to make sure we fixed all relevant points.

commit c52e2421f7368fd36cbe330d2cf41b10452e39a9

Author: Eric Dumazet <[email protected]>
Date:   Tue Oct 15 11:54:30 2013 -0700

tcp: must unclone packets before mangling them

TCP stack should make sure it owns skbs before mangling them.

We had various crashes using bnx2x, and it turned out gso_size was cleared right before bnx2x driver was populating TC descriptor of the _previous_ packet send. TCP stack can sometime retransmit packets that are still in Qdisc.

Of course we could make bnx2x driver more robust (using ACCESS_ONCE(shinfo->gso_size) for example), but the bug is TCP stack.

We have identified two points where skb_unclone() was needed.

This patch adds a WARN_ON_ONCE() to warn us if we missed another fix of this kind.

Kudos to Neal for finding the root cause of this bug. Its visible using small MSS.

Yes, I've seen that patch. The bug I'm fixing has the same symptoms, but the mangling is being done by the sunvnet driver and the fix is the same-- don't mangle packets that are cloned (and unclone them if you need to mangle them).

The original sunvnet code changed gso_size temporarily, but that still had a race where TCP could see the driver-modified gso_size with low probability and end up with a negative packets-in-flight.

The unclone removes that bug, created (by me) after your fix, but in the sunvnet driver with the addition of TSO support.

						+-DLS

...

From: David L Stevens <[email protected]>
Date: Wed, 11 Feb 2015 08:20:17 -0500

Applied, thanks.