Thread (6 messages) 6 messages, 2 authors, 2013-02-28

Re: [PATCH net] tcp: Don't collapse if resulting skb could overflow skb->csum_start

From: Eric Dumazet <hidden>
Date: 2013-02-28 16:18:49

On Thu, 2013-02-28 at 10:26 +0000, Thomas Graf wrote:
quoted hunk ↗ jump to hunk
If a TCP retransmission gets partially ACKed and collapsed multiple
times it is possible for the headroom to grow beyond 64K which will
overflow the 16bit skb->csum_start which is based on the start of
the headroom. It has been observed rarely in the wild with IPoIB due
to the 64K MTU.

With this patch, the overflow has not been observed for over a week
while previously it would occur within ~ 1 day.

A big thank you to Jim Foraker [off-list ref] and the team at
LLNL for helping out with the investigation and testing.

Reported-by: Jim Foraker <redacted>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/ipv4/tcp_output.c | 6 ++++++
 1 file changed, 6 insertions(+)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e2b4461..1902fee 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2305,6 +2305,12 @@ static void tcp_retrans_try_collapse(struct sock *sk, struct sk_buff *to,
 		if (after(TCP_SKB_CB(skb)->end_seq, tcp_wnd_end(tp)))
 			break;
 
+		/* Never collapse if the resulting headroom + data exceeds
+		 * 64K as that is the maximum csum_start can cover.
+		 */
+		if (skb_headroom(to) + to->len + skb->len > 0xFFFF)
+			break;
+
 		tcp_collapse_retrans(sk, to);
 	}
 }
but.... what is the value of skb_availroom(to) ?

The earlier test at line 2302 should already guard this case ?

               /* Punt if not enough space exists in the first SKB for
                 * the data in the second
                 */
                if (skb->len > skb_availroom(to))
                        break;
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help