[PATCH net-next] sunvnet: don't change gso data on clones

[PATCH net-next] sunvnet: don't change gso data on clones

[David L Stevens]

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.

Signed-off-by: David L Stevens <david.stevens@oracle.com>
---
 drivers/net/ethernet/sun/sunvnet.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunvnet.c b/drivers/net/ethernet/sun/sunvnet.c
index 2b10b85..22e0cad 100644
--- a/drivers/net/ethernet/sun/sunvnet.c
+++ b/drivers/net/ethernet/sun/sunvnet.c
@@ -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

Re: [PATCH net-next] sunvnet: don't change gso data on clones

[Eric Dumazet]

On Wed, 2015-02-11 at 08:20 -0500, David L Stevens wrote:
> 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.
> 
> Signed-off-by: David L Stevens <david.stevens@oracle.com>
> ---

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.

Re: [PATCH net-next] sunvnet: don't change gso data on clones

[David L Stevens]

On 02/11/2015 08:56 AM, Eric Dumazet wrote:
> On Wed, 2015-02-11 at 08:20 -0500, David L Stevens wrote:
>> 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.
>>
>> Signed-off-by: David L Stevens <david.stevens@oracle.com>
>> ---
> 
> Hmm... this would point to a TCP bug ?
> 
> What happens if not GSO is needed, TCP corrupts data currently
> read/processed by NIC/driver ?

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

Re: [PATCH net-next] sunvnet: don't change gso data on clones

[Eric Dumazet]

On Wed, 2015-02-11 at 12:35 -0500, David L Stevens wrote:

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

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 <edumazet@google.com>
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.
    
    Signed-off-by: Eric Dumazet <edumazet@google.com>
    Signed-off-by: Neal Cardwell <ncardwell@google.com>
    Cc: Yuchung Cheng <ycheng@google.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH net-next] sunvnet: don't change gso data on clones

[David L Stevens]

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

On 02/11/2015 09:52 PM, Eric Dumazet wrote:

> 
> 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 <edumazet@google.com>
> 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.
...

Re: [PATCH net-next] sunvnet: don't change gso data on clones

[Eric Dumazet]

On Wed, 2015-02-11 at 08:20 -0500, David L Stevens wrote:
> 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.
> 
> Signed-off-by: David L Stevens <david.stevens@oracle.com>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next] sunvnet: don't change gso data on clones

[David Miller]

From: David L Stevens <david.stevens@oracle.com>
Date: Wed, 11 Feb 2015 08:20:17 -0500

> 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.
> 
> Signed-off-by: David L Stevens <david.stevens@oracle.com>

Applied, thanks.