From owner-svn-src-stable@freebsd.org Wed Jan 13 15:31:00 2021 Return-Path: Delivered-To: svn-src-stable@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 0E3E64E06B1; Wed, 13 Jan 2021 15:31:00 +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 4DGBH75xdQz4pQg; Wed, 13 Jan 2021 15:30:59 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from [IPv6:2a02:8109:1140:c3d:7988:47cb:edef:6bb9] (unknown [IPv6:2a02:8109:1140:c3d:7988:47cb:edef:6bb9]) (Authenticated sender: macmic) by mail-n.franken.de (Postfix) with ESMTPSA id 9F035721BE01D; Wed, 13 Jan 2021 16:30:55 +0100 (CET) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.40.0.2.32\)) Subject: Re: svn commit: r368181 - in stable/12/sys/netinet: . tcp_stacks From: Michael Tuexen In-Reply-To: Date: Wed, 13 Jan 2021 16:30:53 +0100 Cc: src-committers , svn-src-all , svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org, dmgk@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <202011300945.0AU9jilR008960@repo.freebsd.org> To: Kyle Evans X-Mailer: Apple Mail (2.3654.40.0.2.32) 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: 4DGBH75xdQz4pQg 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-stable@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: SVN commit messages for all the -stable branches of the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Jan 2021 15:31:00 -0000 > On 13. Jan 2021, at 16:16, Kyle Evans wrote: >=20 > On Wed, Jan 6, 2021 at 9:01 AM Kyle Evans wrote: >>=20 >> On Mon, Nov 30, 2020 at 3:45 AM Michael Tuexen = wrote: >>>=20 >>> Author: tuexen >>> Date: Mon Nov 30 09:45:44 2020 >>> New Revision: 368181 >>> URL: https://svnweb.freebsd.org/changeset/base/368181 >>>=20 >>> 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. >>>=20 >>> MFC 367891: >>> Fix an issue I introuced in r367530: tcp_twcheck() can be called >>> with to =3D=3D NULL for SYN segments. So don't assume tp !=3D NULL. >>> Thanks to jhb@ for reporting and suggesting a fix. >>>=20 >>> MFC r367946: >>> Fix two occurences of a typo in a comment introduced in r367530. >>> Thanks to lstewart@ for reporting them. >>>=20 >>=20 >> Hi Michael, >>=20 >> 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). >>=20 >> 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 >>=20 >> 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# >>=20 >=20 > 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? Best regards Michael >=20 >>>=20 >>> 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) >>>=20 >>> Modified: stable/12/sys/netinet/tcp_input.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 >>> --- 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); >>>=20 >>> - 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 >>> } >>>=20 >>> /* >>> - * 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 =3D 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 =3D 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); >>> } >>> } >>>=20 >>> Modified: stable/12/sys/netinet/tcp_stacks/rack.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 >>> --- 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 =3D 5; >>> + retval =3D 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); >>>=20 >>> /* >>> * 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 =3D 2; >>> } >>> + done_with_input: >>> rack_log_doseg_done(rack, cts, nxt_pkt, did_out, = way_out); >>> if (did_out) >>> rack->r_wanted_output =3D 0; >>>=20 >>> Modified: stable/12/sys/netinet/tcp_syncache.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 >>> --- 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 >>> } >>>=20 >>> /* >>> + * 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 =3D 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 =3D 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 =3D 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 !=3D 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 =3D 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 =3D tcp_log_addrs(inc, th, NULL, NULL))) { >>> - log(LOG_DEBUG, "%s; %s: Timestamp missing, " >>> - "no action\n", s, __func__); >>> - free(s, M_TCPLOG); >>> - s =3D NULL; >>> - } >>> } >>>=20 >>> *lsop =3D syncache_socket(sc, *lsop, m); >>>=20 >>> Modified: stable/12/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 >>> --- 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 =3D=3D 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; >>>=20 >>> thflags =3D th->th_flags; >>> + KASSERT(to !=3D NULL || (thflags & (TH_SYN | TH_ACK)) =3D=3D = TH_SYN, >>> + ("tcp_twcheck: called without options on a non-SYN = segment")); >>>=20 >>> /* >>> * 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) =3D=3D 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) =3D=3D 0) && (tw->t_recent !=3D = 0)) { >>> + goto drop; >>> + } >>>=20 >>> /* >>> * Reset the 2MSL timer if this is a duplicate FIN.