Date: Wed, 13 Jan 2021 09:33:02 -0600 From: Kyle Evans <kevans@freebsd.org> To: Michael Tuexen <tuexen@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org, dmgk@freebsd.org Subject: Re: svn commit: r368181 - in stable/12/sys/netinet: . tcp_stacks Message-ID: <CACNAnaEqcLLFP7u9Rvm6u6cnSTUCgxbOAhHJdcHtuJ3OLUy%2BkA@mail.gmail.com> In-Reply-To: <D5012151-5379-4249-AB3A-825C1F4E2850@freebsd.org> References: <202011300945.0AU9jilR008960@repo.freebsd.org> <CACNAnaEHH9e47hveitj_X-Zsh%2B_-5REckjA3_ZZbPJKGzgmWRA@mail.gmail.com> <CACNAnaGomHxXMbLV6OFSH7Mhv%2BDoC7Q-u0Q7r_T5ChjNK5jHXw@mail.gmail.com> <D5012151-5379-4249-AB3A-825C1F4E2850@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jan 13, 2021 at 9:31 AM Michael Tuexen <tuexen@freebsd.org> wrote: > > > On 13. Jan 2021, at 16:16, Kyle Evans <kevans@FreeBSD.org> wrote: > > > > On Wed, Jan 6, 2021 at 9:01 AM Kyle Evans <kevans@freebsd.org> wrote: > >> > >> On Mon, Nov 30, 2020 at 3:45 AM Michael Tuexen <tuexen@freebsd.org> wrote: > >>> > >>> Author: tuexen > >>> Date: Mon Nov 30 09:45:44 2020 > >>> New Revision: 368181 > >>> URL: https://svnweb.freebsd.org/changeset/base/368181 > >>> > >>> Log: > >>> MFC r367530: > >>> 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. > >>> Manually resolved merge conflicts. > >>> > >>> MFC 367891: > >>> Fix an issue I introuced in r367530: tcp_twcheck() can be called > >>> with to == NULL for SYN segments. So don't assume tp != NULL. > >>> Thanks to jhb@ for reporting and suggesting a fix. > >>> > >>> MFC r367946: > >>> Fix two occurences of a typo in a comment introduced in r367530. > >>> Thanks to lstewart@ for reporting them. > >>> > >> > >> Hi Michael, > >> > >> Dmitri (CC'd) spotted a regression in the golang test suite along > >> stable/12 and bisected it back to this MFC (reported via > >> efnet#bsdports). The test puts up a local HTTP server and attempts to > >> close the read-side while the write-side is still going, hopefully > >> observing a write failure on the write-side in the process (but it > >> never does). > >> > >> I minimized it to this (rough) reproducer, which shows the write side > >> hanging around in CLOSE_WAIT and successfully writing the msg > >> repeatedly on recent -CURRENT while 12.2 observes an EPIPE almost > >> immediately: https://people.freebsd.org/~kevans/tcpr.c > >> > >> root@viper:~/grep# sockstat -s | grep 8993 > >> root a.out 80831 4 tcp4 127.0.0.1:8993 *:* > >> LISTEN > >> root a.out 80831 5 tcp4 127.0.0.1:8993 > >> 127.0.0.1:40319 CLOSE_WAIT > >> root@viper:~/grep# > >> > > > > Ping? > Hi Kyle, > > thanks for pinging. I missed your original mail (not sure why it did not end up in the > correct mailbox). Will look into it later today/tomorrow. > > Thanks for providing a reproducer. Just to get it crystal clear: You say that the > programs runs fine on CURRENT but not on stable/12. Is that correct? > Excellent, thanks! It runs fine on 12.2, but not on an up-to-date -CURRENT or stable/12 after this MFC. > Best regards > Michael > > > >>> > >>> Modified: > >>> stable/12/sys/netinet/tcp_input.c > >>> stable/12/sys/netinet/tcp_stacks/rack.c > >>> stable/12/sys/netinet/tcp_syncache.c > >>> stable/12/sys/netinet/tcp_timewait.c > >>> Directory Properties: > >>> stable/12/ (props changed) > >>> > >>> Modified: stable/12/sys/netinet/tcp_input.c > >>> ============================================================================== > >>> --- stable/12/sys/netinet/tcp_input.c Mon Nov 30 09:22:33 2020 (r368180) > >>> +++ stable/12/sys/netinet/tcp_input.c Mon Nov 30 09:45:44 2020 (r368181) > >>> @@ -975,8 +975,8 @@ findpcb: > >>> } > >>> INP_INFO_RLOCK_ASSERT(&V_tcbinfo); > >>> > >>> - if (thflags & TH_SYN) > >>> - tcp_dooptions(&to, optp, optlen, TO_SYN); > >>> + tcp_dooptions(&to, optp, optlen, > >>> + (thflags & TH_SYN) ? TO_SYN : 0); > >>> /* > >>> * NB: tcp_twcheck unlocks the INP and frees the mbuf. > >>> */ > >>> @@ -1706,20 +1706,29 @@ tcp_do_segment(struct mbuf *m, struct tcphdr *th, stru > >>> } > >>> > >>> /* > >>> - * If timestamps were negotiated during SYN/ACK they should > >>> - * appear on every segment during this session and vice versa. > >>> + * 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 ((tp->t_flags & TF_RCVD_TSTMP) && !(to.to_flags & TOF_TS)) { > >>> if ((s = tcp_log_addrs(inc, th, NULL, NULL))) { > >>> log(LOG_DEBUG, "%s; %s: Timestamp missing, " > >>> - "no action\n", s, __func__); > >>> + "segment silently dropped\n", s, __func__); > >>> free(s, M_TCPLOG); > >>> } > >>> + goto drop; > >>> } > >>> + /* > >>> + * If timestamps were not negotiated during SYN/ACK and a > >>> + * segment with a timestamp is received, ignore the > >>> + * timestamp and process the packet normally. > >>> + * See section 3.2 of RFC 7323. > >>> + */ > >>> if (!(tp->t_flags & TF_RCVD_TSTMP) && (to.to_flags & TOF_TS)) { > >>> if ((s = tcp_log_addrs(inc, th, NULL, NULL))) { > >>> log(LOG_DEBUG, "%s; %s: Timestamp not expected, " > >>> - "no action\n", s, __func__); > >>> + "segment processed normally\n", s, __func__); > >>> free(s, M_TCPLOG); > >>> } > >>> } > >>> > >>> Modified: stable/12/sys/netinet/tcp_stacks/rack.c > >>> ============================================================================== > >>> --- stable/12/sys/netinet/tcp_stacks/rack.c Mon Nov 30 09:22:33 2020 (r368180) > >>> +++ stable/12/sys/netinet/tcp_stacks/rack.c Mon Nov 30 09:45:44 2020 (r368181) > >>> @@ -6708,7 +6708,27 @@ rack_hpts_do_segment(struct mbuf *m, struct tcphdr *th > >>> TCP_LOG_EVENT(tp, th, &so->so_rcv, &so->so_snd, TCP_LOG_IN, 0, > >>> tlen, &log, true); > >>> } > >>> + > >>> /* > >>> + * Parse options on any incoming segment. > >>> + */ > >>> + tcp_dooptions(&to, (u_char *)(th + 1), > >>> + (th->th_off << 2) - sizeof(struct tcphdr), > >>> + (thflags & TH_SYN) ? TO_SYN : 0); > >>> + > >>> + /* > >>> + * 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 ((tp->t_flags & TF_RCVD_TSTMP) && !(to.to_flags & TOF_TS)) { > >>> + way_out = 5; > >>> + retval = 0; > >>> + goto done_with_input; > >>> + } > >>> + > >>> + /* > >>> * Segment received on connection. Reset idle time and keep-alive > >>> * timer. XXX: This should be done after segment validation to > >>> * ignore broken/spoofed segs. > >>> @@ -6761,12 +6781,6 @@ rack_hpts_do_segment(struct mbuf *m, struct tcphdr *th > >>> rack_cong_signal(tp, th, CC_ECN); > >>> } > >>> } > >>> - /* > >>> - * Parse options on any incoming segment. > >>> - */ > >>> - tcp_dooptions(&to, (u_char *)(th + 1), > >>> - (th->th_off << 2) - sizeof(struct tcphdr), > >>> - (thflags & TH_SYN) ? TO_SYN : 0); > >>> > >>> /* > >>> * If echoed timestamp is later than the current time, fall back to > >>> @@ -6898,6 +6912,7 @@ rack_hpts_do_segment(struct mbuf *m, struct tcphdr *th > >>> rack_timer_audit(tp, rack, &so->so_snd); > >>> way_out = 2; > >>> } > >>> + done_with_input: > >>> rack_log_doseg_done(rack, cts, nxt_pkt, did_out, way_out); > >>> if (did_out) > >>> rack->r_wanted_output = 0; > >>> > >>> Modified: stable/12/sys/netinet/tcp_syncache.c > >>> ============================================================================== > >>> --- stable/12/sys/netinet/tcp_syncache.c Mon Nov 30 09:22:33 2020 (r368180) > >>> +++ stable/12/sys/netinet/tcp_syncache.c Mon Nov 30 09:45:44 2020 (r368181) > >>> @@ -1142,6 +1142,40 @@ syncache_expand(struct in_conninfo *inc, struct tcpopt > >>> } > >>> > >>> /* > >>> + * If timestamps were not negotiated during SYN/ACK and a > >>> + * segment with a timestamp is received, ignore the > >>> + * timestamp and process the packet normally. > >>> + * See section 3.2 of RFC 7323. > >>> + */ > >>> + 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 processed normally\n", > >>> + s, __func__); > >>> + free(s, M_TCPLOG); > >>> + s = NULL; > >>> + } > >>> + } > >>> + > >>> + /* > >>> + * 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 ((sc->sc_flags & SCF_TIMESTAMP) && > >>> + !(to->to_flags & TOF_TS)) { > >>> + SCH_UNLOCK(sch); > >>> + if ((s = tcp_log_addrs(inc, th, NULL, NULL))) { > >>> + log(LOG_DEBUG, "%s; %s: Timestamp missing, " > >>> + "segment silently dropped\n", s, __func__); > >>> + free(s, M_TCPLOG); > >>> + } > >>> + return (-1); /* Do not send RST */ > >>> + } > >>> + > >>> + /* > >>> * Pull out the entry to unlock the bucket row. > >>> * > >>> * NOTE: We must decrease TCPS_SYN_RECEIVED count here, not > >>> @@ -1184,32 +1218,6 @@ syncache_expand(struct in_conninfo *inc, struct tcpopt > >>> log(LOG_DEBUG, "%s; %s: SEQ %u != IRS+1 %u, segment " > >>> "rejected\n", s, __func__, th->th_seq, sc->sc_irs); > >>> goto failed; > >>> - } > >>> - > >>> - /* > >>> - * If timestamps were not negotiated during SYN/ACK they > >>> - * must not appear on any segment during this session. > >>> - */ > >>> - 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 during SYN/ACK they should > >>> - * appear on every segment during this session. > >>> - * XXXAO: This is only informal as there have been unverified > >>> - * reports of non-compliants stacks. > >>> - */ > >>> - 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, " > >>> - "no action\n", s, __func__); > >>> - free(s, M_TCPLOG); > >>> - s = NULL; > >>> - } > >>> } > >>> > >>> *lsop = syncache_socket(sc, *lsop, m); > >>> > >>> Modified: stable/12/sys/netinet/tcp_timewait.c > >>> ============================================================================== > >>> --- stable/12/sys/netinet/tcp_timewait.c Mon Nov 30 09:22:33 2020 (r368180) > >>> +++ stable/12/sys/netinet/tcp_timewait.c Mon Nov 30 09:45:44 2020 (r368181) > >>> @@ -373,9 +373,10 @@ tcp_twstart(struct tcpcb *tp) > >>> /* > >>> * Returns 1 if the TIME_WAIT state was killed and we should start over, > >>> * looking for a pcb in the listen state. Returns 0 otherwise. > >>> + * It be called with to == NULL only for pure SYN-segments. > >>> */ > >>> 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; > >>> @@ -396,6 +397,8 @@ tcp_twcheck(struct inpcb *inp, struct tcpopt *to __unu > >>> goto drop; > >>> > >>> thflags = th->th_flags; > >>> + KASSERT(to != NULL || (thflags & (TH_SYN | TH_ACK)) == TH_SYN, > >>> + ("tcp_twcheck: called without options on a non-SYN segment")); > >>> > >>> /* > >>> * NOTE: for FIN_WAIT_2 (to be added later), > >>> @@ -443,6 +446,16 @@ tcp_twcheck(struct inpcb *inp, struct tcpopt *to __unu > >>> */ > >>> 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. >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaEqcLLFP7u9Rvm6u6cnSTUCgxbOAhHJdcHtuJ3OLUy%2BkA>