| Message ID | 54DB5711.3040705@oracle.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. ... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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(-)