Date: Fri, 20 Nov 2020 00:39:42 +0100 From: Michael Tuexen <tuexen@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r367530 - in head/sys/netinet: . tcp_stacks Message-ID: <96A00DDE-2E94-4F6D-AD66-4DCA822F8E7D@freebsd.org> In-Reply-To: <00fb0227-efd3-e1a2-4178-15bdf6f26712@FreeBSD.org> References: <202011092149.0A9Lnfmh069050@repo.freebsd.org> <9fd00098-0ce9-627e-0163-7ede5aa18d6f@FreeBSD.org> <00fb0227-efd3-e1a2-4178-15bdf6f26712@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 20. Nov 2020, at 00:13, John Baldwin <jhb@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?96A00DDE-2E94-4F6D-AD66-4DCA822F8E7D>