[PATCH][v4] tcp: fix ICMP-RTO war

Make sure, that TCP has a nonzero RTT estimation after three-way
handshake. Currently, a listening TCP has a value of 0 for srtt,
rttvar and rto right after the three-way handshake is completed
with TCP timestamps disabled.
This will lead to corrupt RTO recalculation and retransmission
flood when RTO is recalculated on backoff reversion as introduced
in "Revert RTO on ICMP destination unreachable"
(f1ecd5d9e7366609d640ff4040304ea197fbc618).
This behaviour can be provoked by connecting to a server which
"responds first" (like SMTP) and rejecting every packet after
the handshake with dest-unreachable, which will lead to softirq
load on the server (up to 30% per socket in some tests).

Thanks to Ilpo Jarvinen for providing debug patches and to
Denys Fedoryshchenko for reporting and testing.

Changes since v3: Removed bad characters in patchfile.

Reported-by: Denys Fedoryshchenko <redacted>
Signed-off-by: Damian Lukowski <redacted>
---
 net/ipv4/tcp_input.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 28e0296..da7173b 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c

@@ -5783,12 +5783,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 
 				/* tcp_ack considers this ACK as duplicate
 				 * and does not calculate rtt.
-				 * Fix it at least with timestamps.
+				 * Force it here.
 				 */
-				if (tp->rx_opt.saw_tstamp &&
-				    tp->rx_opt.rcv_tsecr && !tp->srtt)
-					tcp_ack_saw_tstamp(sk, 0);
-
+				tcp_ack_update_rtt(sk, 0, 0);
+
 				if (tp->rx_opt.tstamp_ok)
 					tp->advmss -= TCPOLEN_TSTAMP_ALIGNED;
 
-- 1.6.4.4 

From: Damian Lukowski <redacted>
Date: Wed, 10 Feb 2010 20:06:42 +0100

Make sure, that TCP has a nonzero RTT estimation after three-way
handshake. Currently, a listening TCP has a value of 0 for srtt,
rttvar and rto right after the three-way handshake is completed
with TCP timestamps disabled.
This will lead to corrupt RTO recalculation and retransmission
flood when RTO is recalculated on backoff reversion as introduced
in "Revert RTO on ICMP destination unreachable"
(f1ecd5d9e7366609d640ff4040304ea197fbc618).
This behaviour can be provoked by connecting to a server which
"responds first" (like SMTP) and rejecting every packet after
the handshake with dest-unreachable, which will lead to softirq
load on the server (up to 30% per socket in some tests).

Thanks to Ilpo Jarvinen for providing debug patches and to
Denys Fedoryshchenko for reporting and testing.

Changes since v3: Removed bad characters in patchfile.

Reported-by: Denys Fedoryshchenko <redacted>
Signed-off-by: Damian Lukowski <redacted>

Thanks for doing this work, I'll study these issues
and review this patch today.

From: Damian Lukowski <redacted>
Date: Wed, 10 Feb 2010 20:06:42 +0100

Make sure, that TCP has a nonzero RTT estimation after three-way
handshake. Currently, a listening TCP has a value of 0 for srtt,
rttvar and rto right after the three-way handshake is completed
with TCP timestamps disabled.
This will lead to corrupt RTO recalculation and retransmission
flood when RTO is recalculated on backoff reversion as introduced
in "Revert RTO on ICMP destination unreachable"
(f1ecd5d9e7366609d640ff4040304ea197fbc618).
This behaviour can be provoked by connecting to a server which
"responds first" (like SMTP) and rejecting every packet after
the handshake with dest-unreachable, which will lead to softirq
load on the server (up to 30% per socket in some tests).

Thanks to Ilpo Jarvinen for providing debug patches and to
Denys Fedoryshchenko for reporting and testing.

Changes since v3: Removed bad characters in patchfile.

Reported-by: Denys Fedoryshchenko <redacted>
Signed-off-by: Damian Lukowski <redacted>

Applied, thank you very much for fixing this bug.

First of all I want to let you know that I've no objection to this fix 
itself (DaveM applied it already), it certainly does it's jobs in 
preventing invalid < RTO_MIN state. However, I wonder if we could do 
further improvements in this area...

On Wed, 10 Feb 2010, Damian Lukowski wrote:

...Here a zero seq_rtt is given to RTT estimator (it will be effective 
only in the case w/o timestamps, TS case recalculates it from the stored 
timestamps). Maybe we could use some field (timestamp related one comes to 
my mind) in request sock to get a real RTT estimate for non-timestamp case 
too. ...It seems possible to me, though tricky because the request_sock is 
no longer that easily available here so some parameter passing would be 
needed.


-- 
 i.

From: "Ilpo Järvinen" <redacted>
Date: Tue, 16 Feb 2010 14:45:25 +0200 (EET)

On Wed, 10 Feb 2010, Damian Lukowski wrote:

quoted

@@ -5783,12 +5783,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 
 				/* tcp_ack considers this ACK as duplicate
 				 * and does not calculate rtt.
-				 * Fix it at least with timestamps.
+				 * Force it here.
 				 */
-				if (tp->rx_opt.saw_tstamp &&
-				    tp->rx_opt.rcv_tsecr && !tp->srtt)
-					tcp_ack_saw_tstamp(sk, 0);
-
+				tcp_ack_update_rtt(sk, 0, 0);
+

...Here a zero seq_rtt is given to RTT estimator (it will be effective 
only in the case w/o timestamps, TS case recalculates it from the stored 
timestamps). Maybe we could use some field (timestamp related one comes to 
my mind) in request sock to get a real RTT estimate for non-timestamp case 
too. ...It seems possible to me, though tricky because the request_sock is 
no longer that easily available here so some parameter passing would be 
needed.

Agreed.

But even more simply I think we should make even the current
tcp_ack_update_rtt() call here conditional on at least
tp->srtt being zero.

Damian do you at least agree with that?