On 20. Nov 2020, at 00:13, John Baldwin <j...@freebsd.org> wrote: > > 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 >>> >>> 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. >>> >>> PR: 250499 >>> Reviewed by: gnn, rrs >>> MFC after: 1 week >>> Sponsored by: Netflix, Inc >>> Differential Revision: https://reviews.freebsd.org/D27148 >>> >>> 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 >>> >>> Modified: head/sys/netinet/tcp_timewait.c >>> ============================================================================== >>> --- 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) == 0) && (tw->t_recent != 0)) { >>> + goto drop; >>> + } >> >> 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? >> >> 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? >> >> That is, I think we should perhaps move this check after the TH_SYN check so >> that a mismatch doesn't prevent recycling? > > 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: Yes, I committed this in https://svnweb.freebsd.org/changeset/base/367891 and also added a comment and a KASSERT. > > 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; > > - /* > - * 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) == 0) && (tw->t_recent != 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) == 0) > 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) == 0) && (tw->t_recent != 0)) { > + goto drop; > + } > + > /* > * Reset the 2MSL timer if this is a duplicate FIN. > */ > > 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@) Yes, I saw that also. Will deal with it in a separate issue...
Thanks for reporting the issue and sorry for breaking the code. I didn't know about this use of tcp_twcheck() and have no hardware to test this... Best regards Michael > > -- > John Baldwin _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
