From owner-svn-src-stable-12@freebsd.org Wed Jan 13 15:16:52 2021 Return-Path: Delivered-To: svn-src-stable-12@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 BBC534E04AE; Wed, 13 Jan 2021 15:16:52 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DG9yr4k2Zz4p5w; Wed, 13 Jan 2021 15:16:52 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from mail-qk1-f182.google.com (mail-qk1-f182.google.com [209.85.222.182]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) (Authenticated sender: kevans) by smtp.freebsd.org (Postfix) with ESMTPSA id 8ECD0291EC; Wed, 13 Jan 2021 15:16:52 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: by mail-qk1-f182.google.com with SMTP id f26so1991445qka.0; Wed, 13 Jan 2021 07:16:52 -0800 (PST) X-Gm-Message-State: AOAM530srO0pTns83vLgPPv/wb5zY3g1D2jgFqT0s/kCAMxfzMEdlVU7 WM/LoeQPF65wJSPmXpVON1xyXmncjkLWQFR4+Ng= X-Google-Smtp-Source: ABdhPJw9Z0QDLNKgZVLbAIhGWd/PiLpHRwi+4QuFRWCU/RdKIRtx+/cX8MDrT3DkZcGeXrGERMhr4h1R4SYH2VtxH8M= X-Received: by 2002:a05:620a:14a:: with SMTP id e10mr2408744qkn.103.1610551011725; Wed, 13 Jan 2021 07:16:51 -0800 (PST) MIME-Version: 1.0 References: <202011300945.0AU9jilR008960@repo.freebsd.org> In-Reply-To: From: Kyle Evans Date: Wed, 13 Jan 2021 09:16:38 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r368181 - in stable/12/sys/netinet: . tcp_stacks To: Michael Tuexen Cc: src-committers , svn-src-all , svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org, dmgk@freebsd.org Content-Type: text/plain; charset="UTF-8" X-BeenThere: svn-src-stable-12@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: SVN commit messages for only the 12-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Jan 2021 15:16:52 -0000 On Wed, Jan 6, 2021 at 9:01 AM Kyle Evans wrote: > > On Mon, Nov 30, 2020 at 3:45 AM Michael Tuexen 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? > > > > 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.