From owner-svn-src-head@freebsd.org Thu Nov 19 23:39:47 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 1AB9B477500; Thu, 19 Nov 2020 23:39:47 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from drew.franken.de (mail-n.franken.de [193.175.24.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.franken.de", Issuer "Sectigo RSA Domain Validation Secure Server CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CcbkV6F1xz3mmG; Thu, 19 Nov 2020 23:39:46 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from [IPv6:2a02:8109:1140:c3d:bc38:8d5c:8c97:55c4] (unknown [IPv6:2a02:8109:1140:c3d:bc38:8d5c:8c97:55c4]) (Authenticated sender: macmic) by mail-n.franken.de (Postfix) with ESMTPSA id 2FDDB7220B805; Fri, 20 Nov 2020 00:39:43 +0100 (CET) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.20.0.2.21\)) Subject: Re: svn commit: r367530 - in head/sys/netinet: . tcp_stacks From: Michael Tuexen In-Reply-To: <00fb0227-efd3-e1a2-4178-15bdf6f26712@FreeBSD.org> Date: Fri, 20 Nov 2020 00:39:42 +0100 Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: <96A00DDE-2E94-4F6D-AD66-4DCA822F8E7D@freebsd.org> References: <202011092149.0A9Lnfmh069050@repo.freebsd.org> <9fd00098-0ce9-627e-0163-7ede5aa18d6f@FreeBSD.org> <00fb0227-efd3-e1a2-4178-15bdf6f26712@FreeBSD.org> To: John Baldwin X-Mailer: Apple Mail (2.3654.20.0.2.21) X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=disabled version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on mail-n.franken.de X-Rspamd-Queue-Id: 4CcbkV6F1xz3mmG X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Nov 2020 23:39:47 -0000 > On 20. Nov 2020, at 00:13, John Baldwin wrote: >=20 > On 11/19/20 2:55 PM, John Baldwin wrote: >> On 11/9/20 1:49 PM, Michael Tuexen wrote: >>> Author: tuexen >>> Date: Mon Nov 9 21:49:40 2020 >>> New Revision: 367530 >>> URL: https://svnweb.freebsd.org/changeset/base/367530 >>>=20 >>> Log: >>> RFC 7323 specifies that: >>> * TCP segments without timestamps should be dropped when support = for >>> the timestamp option has been negotiated. >>> * TCP segments with timestamps should be processed normally if = support >>> for the timestamp option has not been negotiated. >>> This patch enforces the above. >>>=20 >>> PR: 250499 >>> Reviewed by: gnn, rrs >>> MFC after: 1 week >>> Sponsored by: Netflix, Inc >>> Differential Revision: https://reviews.freebsd.org/D27148 >>>=20 >>> Modified: >>> head/sys/netinet/tcp_input.c >>> head/sys/netinet/tcp_stacks/bbr.c >>> head/sys/netinet/tcp_stacks/rack.c >>> head/sys/netinet/tcp_syncache.c >>> head/sys/netinet/tcp_timewait.c >>>=20 >>> Modified: head/sys/netinet/tcp_timewait.c >>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>> --- head/sys/netinet/tcp_timewait.c Mon Nov 9 21:19:17 2020 = (r367529) >>> +++ head/sys/netinet/tcp_timewait.c Mon Nov 9 21:49:40 2020 = (r367530) >>> @@ -376,7 +376,7 @@ tcp_twstart(struct tcpcb *tp) >>> * looking for a pcb in the listen state. Returns 0 otherwise. >>> */ >>> int >>> -tcp_twcheck(struct inpcb *inp, struct tcpopt *to __unused, struct = tcphdr *th, >>> +tcp_twcheck(struct inpcb *inp, struct tcpopt *to, struct tcphdr = *th, >>> struct mbuf *m, int tlen) >>> { >>> struct tcptw *tw; >>> @@ -410,6 +410,16 @@ tcp_twcheck(struct inpcb *inp, struct tcpopt = *to __unu >>> */ >>> if (thflags & TH_RST) >>> goto drop; >>> + >>> + /* >>> + * If timestamps were negotiated during SYN/ACK and a >>> + * segment without a timestamp is received, silently drop >>> + * the segment. >>> + * See section 3.2 of RFC 7323. >>> + */ >>> + if (((to->to_flags & TOF_TS) =3D=3D 0) && (tw->t_recent !=3D 0)) = { >>> + goto drop; >>> + } >>=20 >> This causes an insta-panic with TOE because toe_4tuple_check() passes = in a NULL >> pointer for 'to'. I'm working on a fix for that, but perhaps wait to = MFC until >> the fix is ready so they can be merged together? >>=20 >> That said, TOE only calls this in the case that it has gotten a new = SYN, so I >> wonder if it makes sense to apply this check on a new SYN. For a new = SYN, >> shouldn't we not care if the new connection is using a different = timestamp >> option from the old connection? The language in RFC 7323 3.2 is all = about >> segments on an existing connection, not segments from a new = connection I think? >>=20 >> That is, I think we should perhaps move this check after the TH_SYN = check so >> that a mismatch doesn't prevent recycling? >=20 > Actually, we move the check below requiring TH_ACK, I think this would = fix the TOE > case and also DTRT for plain SYNs for non-TOE: Let me have a look tomorrow morning... Best regards Michael >=20 > diff --git a/sys/netinet/tcp_timewait.c b/sys/netinet/tcp_timewait.c > index c52eab956303..85f1ccbe40f9 100644 > --- a/sys/netinet/tcp_timewait.c > +++ b/sys/netinet/tcp_timewait.c > @@ -411,16 +411,6 @@ tcp_twcheck(struct inpcb *inp, struct tcpopt *to, = struct tcphdr *th, > if (thflags & TH_RST) > goto drop; >=20 > - /* > - * If timestamps were negotiated during SYN/ACK and a > - * segment without a timestamp is received, silently drop > - * the segment. > - * See section 3.2 of RFC 7323. > - */ > - if (((to->to_flags & TOF_TS) =3D=3D 0) && (tw->t_recent !=3D 0)) = { > - goto drop; > - } > - > #if 0 > /* PAWS not needed at the moment */ > /* > @@ -455,6 +445,16 @@ tcp_twcheck(struct inpcb *inp, struct tcpopt *to, = struct tcphdr *th, > if ((thflags & TH_ACK) =3D=3D 0) > goto drop; >=20 > + /* > + * If timestamps were negotiated during SYN/ACK and a > + * segment without a timestamp is received, silently drop > + * the segment. > + * See section 3.2 of RFC 7323. > + */ > + if (((to->to_flags & TOF_TS) =3D=3D 0) && (tw->t_recent !=3D 0)) = { > + goto drop; > + } > + > /* > * Reset the 2MSL timer if this is a duplicate FIN. > */ >=20 > The commented out PAWS bits would also seem to not be relevant for = SYN-only > packets? However, I'm less sure of if that bit should be moved later = as > well. (Or perhaps it should just be removed. It has been #if 0'd = since the > timewait structure was first added back in 2003 by jlemon@) >=20 > --=20 > John Baldwin