diff mbox

[net-next] tcp: avoid sending zero TSval

Message ID 200908312044.56235.opurdila@ixiacom.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Octavian Purdila Aug. 31, 2009, 5:44 p.m. UTC
Per RFC1323, zero TSecr is considered invalid. Thus we must avoid when
possible sending a zero TSval.

Currently, we use the least significant 32 bits of jiffies to fill in
TSval. But that can wrap around to zero (in 5 minutes after reboot,
and every 49 days after that in the worst case).

This patch approximate a wrap-around zero TSval to 1. This is better
then emitting a value which will be ignored.

Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>
---
 net/ipv4/tcp_output.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

Comments

David Miller Sept. 2, 2009, 1:16 a.m. UTC | #1
From: Octavian Purdila <opurdila@ixiacom.com>
Date: Mon, 31 Aug 2009 20:44:56 +0300

> Per RFC1323, zero TSecr is considered invalid. Thus we must avoid when
> possible sending a zero TSval.
> 
> Currently, we use the least significant 32 bits of jiffies to fill in
> TSval. But that can wrap around to zero (in 5 minutes after reboot,
> and every 49 days after that in the worst case).
> 
> This patch approximate a wrap-around zero TSval to 1. This is better
> then emitting a value which will be ignored.
> 
> Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>

Ok, I've changed my mind again.  I think we need to go with
a solution like this.

Even if we could somehow justify allowing zero timestamps,
I just checked some other stacks and all of them ignore zero
tsecr values.  So we can't make that kind of change no matter
what.

This patch needs some changes.

We have to adjust the tests we make against tsecr.

If we bump up a zero jiffies to one in an advertised timestamp,
then we get back a tsecr value of one, and jiffies is still
zero, we should use a comparison value of one not zero.

This is not trivial.  You might think it's OK to handle all of
this by just adjusting the definition of tcp_time_stamp but that
gets used by a lot of other things in the stack so those side
effects need to be analyzed.

Grepping around also shows that we also have some code that doesn't
handle jiffies wraparound at all, f.e. check out the rcv_tsecr tests
in net/ipv4/tcp_lp.c :-/
--
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 mbox

Patch

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 4e00442..607d675 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -415,7 +415,10 @@  static void tcp_options_write(__be32 *ptr, struct tcp_sock *tp,
 				       (TCPOPT_TIMESTAMP << 8) |
 				       TCPOLEN_TIMESTAMP);
 		}
-		*ptr++ = htonl(opts->tsval);
+		/* Currently, opts->tsval always comes from tcp_time_stamp.
+		 * Thus, if zero, its caused by jiffie wrap-around, and we can
+		 * approximate it to one to avoid a later TSecr drop */
+		*ptr++ = htonl(opts->tsval?:1);
 		*ptr++ = htonl(opts->tsecr);
 	}