Date: Wed, 23 Jan 2008 20:58:37 -0800 From: "Kip Macy" <kip.macy@gmail.com> To: "Andre Oppermann" <andre@freebsd.org> Cc: Mike Silbersack <silby@freebsd.org>, kmacy@freebsd.org, cvs-src@freebsd.org, cvs-all@freebsd.org, src-committers@freebsd.org, freebsd-net@freebsd.org Subject: Re: cvs commit: src/sys/netinet tcp_syncache.c Message-ID: <b1fa29170801232058n26f59928ue7d36865b1ff1561@mail.gmail.com> In-Reply-To: <4797B77E.2090605@freebsd.org> References: <200711200656.lAK6u4bc021279@repoman.freebsd.org> <4797B77E.2090605@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Did you talk to the original submitter? Note that FreeBSD's TCP stack
is for use in servers and is not intending as a validating TCP stack.
If you would like it to serve as such you would better served by
tracking down the ANVL tests that FreeBSD fails. Also note that there
is no MUST in the following sentence:
"For simplicity and symmetry, we specify that
timestamps always be sent and echoed in both directions."
So it is clearly open to interpretation.
-Kip
On Jan 23, 2008 1:54 PM, Andre Oppermann <andre@freebsd.org> wrote:
>
> 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.
> _______________________________________________
> freebsd-net@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?b1fa29170801232058n26f59928ue7d36865b1ff1561>