cvs commit: src/sys/netinet tcp_syncache.c

[Andre Oppermann]

Mike Silbersack wrote:
> silby       2007-11-20 06:56:04 UTC
> 
>   FreeBSD src repository
> 
>   Modified files:
>     sys/netinet          tcp_syncache.c 
>   Log:
>   Comment out the syncache's test which ensures that hosts which negotiate TCP
>   timestamps in the initial SYN packet actually use them in the rest of the
>   connection.  Unfortunately, during the 7.0 testing cycle users have already
>   found network devices that violate this constraint.
>   
>   RFC 1323 states 'and may send a TSopt in other segments' rather than
>   'and MUST send', so we must allow it.
>   
>   Discovered by: Rob Zietlow
>   Tracked down by: Kip Macy
>   PR: bin/118005
>   
>   Revision  Changes    Path
>   1.134     +6 -0      src/sys/netinet/tcp_syncache.c
 >
> kmacy       2007-12-12 06:11:50 UTC
> 
>   FreeBSD src repository
> 
>   Modified files:
>     sys/netinet          tcp_syncache.c 
>   Log:
>   Remove spurious timestamp check. RFC 1323 explicitly states that timestamps MAY
>   be transmitted if negotiated.
>   
>   Revision  Changes    Path
>   1.138     +1 -17     src/sys/netinet/tcp_syncache.c

These changes and their rationale are not really correct.  The citation
from RFC1323 is grossly misrepresented.  The partly cited sentence says 
that timestamps may only be transmitted if they were negotiated. It
doesn't say that timestamps may or may not be sent after they were
negotiated.  In fact RFC1323 Section 3.2 explicitly states: "For
simplicity and symmetry, we specify that timestamps always be sent
and echoed in both directions."  It is also the unchallenged consensus
view of the IETF TCPM is that timestamps must be sent when negotiated
[1].  It is not allowed to not include them when agreed so at SYN time.
Any deviation of this is a violation of the RFC.  Thus the test was
correct and valid.

OTOH the enforcement of this rule wasn't really there before and it
may be argued that we've got a POLA violation here.  A careful reading
of the referenced PR bin/118005 and its audit trail do not support the
conclusion that the enforcement is the root cause of the symptoms
described.  The submitter makes a clear distinction between inside and
outside of his LAN.  Inside everything continues to work just fine.
It's outside of his LAN that certain machines can't establish
connections to the upgraded machine.  The cited outside machines are
FreeBSD 6.2, OpenBSD 4.1 and Linux RHEL 4U4.  All these operating
systems correctly implement RFC1323 timestamps and do not omit them
once negotiated.  Hence it is very likely that the root cause is in
some device along the path munging the packets in a non-RFC conform
way.  Perhaps a reverse-NAT gateway on his Internet gateway.  Or some
other packet rewriting system.  The PR needs to reopened and the root
cause properly inspected.

Based on the rationale above I'd like to re-instate the test in HEAD
because a) it is correct and b) even if disabled to serve as education
for future readers of the SYN/SYN-ACK/ACK path.  If the test has to
remain disabled we should include a clear description including which
systems and their revisions fail the test.  Alternatively the test
should be modified to disable sending of timestamps if this situation
occurs as they are a pointless waste of bits anyway as they won't ever
be returned back to us to measure anything.


[1] http://www1.ietf.org/mail-archive/web/tcpm/current/msg02507.html
     and (many) following messages in that thread

-- 
Andre

Index: tcp_syncache.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/tcp_syncache.c,v
retrieving revision 1.141
diff -u -p -r1.141 tcp_syncache.c
--- tcp_syncache.c      19 Dec 2007 16:56:28 -0000      1.141
+++ tcp_syncache.c      23 Jan 2008 21:33:49 -0000
@@ -914,12 +914,41 @@ syncache_expand(struct in_conninfo *inc,
                 goto failed;
         }

+       /*
+        * If timestamps were present in the SYN and we accepted
+        * them in our SYN|ACK RFC1323 Section 3.2 requires them
+        * to be present from now on.  And vice versa.
+        */
+       if ((sc->sc_flags & SCF_TIMESTAMP) && !(to->to_flags & TOF_TS)) {
+               if ((s = tcp_log_addrs(inc, th, NULL, NULL)))
+                       log(LOG_DEBUG, "%s; %s: Timestamp missing, "
+                           "segment rejected\n", s, __func__);
+#ifdef TCP_BROKEN_TS
+               /*
+                * There appear to be non-conformant devices that
+                * negotiate timestamps but then fail to include
+                * them in every segment from then on.
+                *
+                * Evidence of this really happening is inconclusive
+                * and specific operating systems or firmware and their
+                * revisions are not known.
+                *
+                * For a work-around disable sending of timestamps
+                * as they become a pointless waste of bandwidth
+                * if we won't get them reflected back anyway.
+                */
+               sc->sc_flags &= ~SCF_TIMESTAMP;
+#else
+               goto failed;
+#endif
+       }
         if (!(sc->sc_flags & SCF_TIMESTAMP) && (to->to_flags & TOF_TS)) {
                 if ((s = tcp_log_addrs(inc, th, NULL, NULL)))
                         log(LOG_DEBUG, "%s; %s: Timestamp not expected, "
                             "segment rejected\n", s, __func__);
                 goto failed;
         }
+
         /*
          * If timestamps were negotiated the reflected timestamp
          * must be equal to what we actually sent in the SYN|ACK.