Subject: Re: netinet/tcp_input.c - 1323 timestamp handling broken
To: None <tech-net@netbsd.org>
From: Christos Zoulas <christos@tac.gw.com>
List: tech-net
Date: 04/21/2005 18:19:22
In article <d494kl$524$1@serpens.de>,
Michael van Elst <mlelstv@serpens.de> wrote:
>rpaulo@netbsd-pt.org (Rui Paulo) writes:
>
>>On 2005-04-17, David Brownlee <abs@absd.org> wrote:
>
>>> +       if (opti.ts_present && opti.ts_ecr) {
>>> +               u_int32_t now;
>>> +
>>> +               /*
>>> +                * Calculate the RTT from the returned time stamp and the
>>> +                * connection's time base.  If the time stamp is later than
>>> +                * the current time, fall back to non-1323 RTT calculation.
>>> +                */
>>> +               now = TCP_TIMESTAMP(tp);
>>> +               if (SEQ_GEQ(now, opti.ts_ecr))
>>> +                       opti.ts_ecr = now - opti.ts_ecr + 1;
>>> +               else
>>> +                       opti.ts_ecr = 0;
>>> +       }
>
>
>>This is somewhat a problem. This patch closes a bug when two TCP packets
>>come in with the same timestamp (see PR/22551), so removing that code is
>>also a problem.
>
>
>I don't know what the patch should do but it effectively replaces
>opti.ts_ecr (which is a timestamp) with a computed round trip time
>which is then used again as a timestamp later in the code as in:
>
>            if (opti.ts_present && opti.ts_ecr)
>                    tcp_xmit_timer(tp,
>                      TCP_TIMESTAMP(tp) - opti.ts_ecr + 1);
>
>
>To me the 'then' case looks completely bogus and should be removed,
>the 'else' case protects against timestamps from the future (and
>avoids the panic from PR/22551.

The problem is that there the patch needs two more pieces to work: 

Index: tcp_input.c
===================================================================
RCS file: /cvsroot/src/sys/netinet/tcp_input.c,v
retrieving revision 1.212
retrieving revision 1.213
diff -u -r1.212 -r1.213
--- tcp_input.c	21 Dec 2004 05:51:31 -0000	1.212
+++ tcp_input.c	26 Jan 2005 21:49:27 -0000	1.213
@@ -1537,8 +1553,7 @@
 				 */
 				++tcpstat.tcps_predack;
 				if (opti.ts_present && opti.ts_ecr)
-					tcp_xmit_timer(tp,
-					  TCP_TIMESTAMP(tp) - opti.ts_ecr + 1);
+					tcp_xmit_timer(tp, opti.ts_ecr);
 				else if (tp->t_rtttime &&
 				    SEQ_GT(th->th_ack, tp->t_rtseq))
 					tcp_xmit_timer(tp,
@@ -2167,7 +2171,7 @@
 		 * Recompute the initial retransmit timer.
 		 */
 		if (opti.ts_present && opti.ts_ecr)
-			tcp_xmit_timer(tp, TCP_TIMESTAMP(tp) - opti.ts_ecr + 1);
+			tcp_xmit_timer(tp, opti.ts_ecr);
 		else if (tp->t_rtttime && SEQ_GT(th->th_ack, tp->t_rtseq))
 			tcp_xmit_timer(tp, tcp_now - tp->t_rtttime);