All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	stable@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
	Neal Cardwell <ncardwell@google.com>,
	Yuchung Cheng <ycheng@google.com>,
	"David S. Miller" <davem@davemloft.net>
Subject: [ 01/32] tcp: must unclone packets before mangling them
Date: Fri,  1 Nov 2013 14:43:12 -0700	[thread overview]
Message-ID: <20131101214314.064766224@linuxfoundation.org> (raw)
In-Reply-To: <20131101214313.735463599@linuxfoundation.org>

3.4-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Eric Dumazet <edumazet@google.com>

[ Upstream commit c52e2421f7368fd36cbe330d2cf41b10452e39a9 ]

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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/skbuff.h |   10 ++++++++++
 net/ipv4/tcp_output.c  |    9 ++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -760,6 +760,16 @@ static inline int skb_cloned(const struc
 	       (atomic_read(&skb_shinfo(skb)->dataref) & SKB_DATAREF_MASK) != 1;
 }
 
+static inline int skb_unclone(struct sk_buff *skb, gfp_t pri)
+{
+	might_sleep_if(pri & __GFP_WAIT);
+
+	if (skb_cloned(skb))
+		return pskb_expand_head(skb, 0, 0, pri);
+
+	return 0;
+}
+
 /**
  *	skb_header_cloned - is the header a clone
  *	@skb: buffer to check
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -933,6 +933,9 @@ static void tcp_queue_skb(struct sock *s
 static void tcp_set_skb_tso_segs(const struct sock *sk, struct sk_buff *skb,
 				 unsigned int mss_now)
 {
+	/* Make sure we own this skb before messing gso_size/gso_segs */
+	WARN_ON_ONCE(skb_cloned(skb));
+
 	if (skb->len <= mss_now || !sk_can_gso(sk) ||
 	    skb->ip_summed == CHECKSUM_NONE) {
 		/* Avoid the costly divide in the normal
@@ -1014,9 +1017,7 @@ int tcp_fragment(struct sock *sk, struct
 	if (nsize < 0)
 		nsize = 0;
 
-	if (skb_cloned(skb) &&
-	    skb_is_nonlinear(skb) &&
-	    pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
+	if (skb_unclone(skb, GFP_ATOMIC))
 		return -ENOMEM;
 
 	/* Get a new skb... force flag on. */
@@ -2129,6 +2130,8 @@ int tcp_retransmit_skb(struct sock *sk,
 		int oldpcount = tcp_skb_pcount(skb);
 
 		if (unlikely(oldpcount > 1)) {
+			if (skb_unclone(skb, GFP_ATOMIC))
+				return -ENOMEM;
 			tcp_init_tso_segs(sk, skb, cur_mss);
 			tcp_adjust_pcount(sk, skb, oldpcount - tcp_skb_pcount(skb));
 		}



  reply	other threads:[~2013-11-01 21:52 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-01 21:43 [ 00/32] 3.4.68-stable review Greg Kroah-Hartman
2013-11-01 21:43 ` Greg Kroah-Hartman [this message]
2013-11-01 21:43 ` [ 02/32] tcp: do not forget FIN in tcp_shifted_skb() Greg Kroah-Hartman
2013-11-01 21:43 ` [ 03/32] net: do not call sock_put() on TIMEWAIT sockets Greg Kroah-Hartman
2013-11-01 21:43 ` [ 04/32] net: mv643xx_eth: update statistics timer from timer context only Greg Kroah-Hartman
2013-11-01 21:43 ` [ 05/32] net: mv643xx_eth: fix orphaned statistics timer crash Greg Kroah-Hartman
2013-11-01 21:43 ` [ 06/32] net: heap overflow in __audit_sockaddr() Greg Kroah-Hartman
2013-11-01 21:43 ` [ 07/32] proc connector: fix info leaks Greg Kroah-Hartman
2013-11-01 21:43 ` [ 08/32] ipv4: fix ineffective source address selection Greg Kroah-Hartman
2013-11-01 21:43 ` [ 09/32] can: dev: fix nlmsg size calculation in can_get_size() Greg Kroah-Hartman
2013-11-01 21:43 ` [ 10/32] ipv6: restrict neighbor entry creation to output flow Greg Kroah-Hartman
2013-11-01 21:43 ` [ 11/32] bridge: Correctly clamp MAX forward_delay when enabling STP Greg Kroah-Hartman
2013-11-01 21:43   ` Greg Kroah-Hartman
2013-11-01 21:43 ` [ 12/32] net: vlan: fix nlmsg size calculation in vlan_get_size() Greg Kroah-Hartman
2013-11-01 21:43 ` [ 13/32] l2tp: must disable bh before calling l2tp_xmit_skb() Greg Kroah-Hartman
2013-11-01 21:43 ` [ 14/32] farsync: fix info leak in ioctl Greg Kroah-Hartman
2013-11-01 21:43 ` [ 15/32] unix_diag: fix info leak Greg Kroah-Hartman
2013-11-01 21:43 ` [ 16/32] connector: use nlmsg_len() to check message length Greg Kroah-Hartman
2013-11-01 21:43 ` [ 17/32] bnx2x: record rx queue for LRO packets Greg Kroah-Hartman
2013-11-01 21:43 ` [ 18/32] net: dst: provide accessor function to dst->xfrm Greg Kroah-Hartman
2013-11-01 21:43 ` [ 19/32] sctp: Use software crc32 checksum when xfrm transform will happen Greg Kroah-Hartman
2013-11-01 21:43 ` [ 20/32] sctp: Perform software checksum if packet has to be fragmented Greg Kroah-Hartman
2013-11-01 21:43 ` [ 21/32] wanxl: fix info leak in ioctl Greg Kroah-Hartman
2013-11-01 21:43 ` [ 22/32] net: unix: inherit SOCK_PASS{CRED, SEC} flags from socket to fix race Greg Kroah-Hartman
2013-11-01 21:43 ` [ 23/32] net: fix cipso packet validation when !NETLABEL Greg Kroah-Hartman
2013-11-01 21:43 ` [ 24/32] inet: fix possible memory corruption with UDP_CORK and UFO Greg Kroah-Hartman
2013-11-01 21:43 ` [ 25/32] davinci_emac.c: Fix IFF_ALLMULTI setup Greg Kroah-Hartman
2013-11-01 21:43 ` [ 26/32] ext3: return 32/64-bit dir name hash according to usage type Greg Kroah-Hartman
2013-11-01 21:43 ` [ 27/32] dm snapshot: fix data corruption Greg Kroah-Hartman
2013-11-01 21:43 ` [ 28/32] writeback: fix negative bdi max pause Greg Kroah-Hartman
2013-11-01 21:43 ` [ 29/32] wireless: radiotap: fix parsing buffer overrun Greg Kroah-Hartman
2013-11-01 21:43 ` [ 30/32] USB: serial: ti_usb_3410_5052: add Abbott strip port ID to combined table as well Greg Kroah-Hartman
2013-11-01 21:43 ` [ 31/32] USB: serial: option: add support for Inovia SEW858 device Greg Kroah-Hartman
2013-11-01 21:43 ` [ 32/32] usb: serial: option: blacklist Olivetti Olicard200 Greg Kroah-Hartman
2013-11-02  2:28 ` [ 00/32] 3.4.68-stable review Guenter Roeck
2013-11-02 21:33 ` Shuah Khan
2013-11-04  3:07 ` Satoru Takeuchi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131101214314.064766224@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ncardwell@google.com \
    --cc=stable@vger.kernel.org \
    --cc=ycheng@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.