Date: Wed, 13 Jan 2021 23:56:54 +0100 From: Michael Tuexen <tuexen@freebsd.org> To: Kyle Evans <kevans@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: <CCD3B200-A002-495C-AEDF-629CF8D4A8AE@freebsd.org> In-Reply-To: <CACNAnaEqcLLFP7u9Rvm6u6cnSTUCgxbOAhHJdcHtuJ3OLUy%2BkA@mail.gmail.com> 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> <CACNAnaEqcLLFP7u9Rvm6u6cnSTUCgxbOAhHJdcHtuJ3OLUy%2BkA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 13. Jan 2021, at 16:33, Kyle Evans <kevans@FreeBSD.org> wrote: >=20 > On Wed, Jan 13, 2021 at 9:31 AM Michael Tuexen <tuexen@freebsd.org> = wrote: >>=20 >>> On 13. Jan 2021, at 16:16, Kyle Evans <kevans@FreeBSD.org> wrote: >>>=20 >>> On Wed, Jan 6, 2021 at 9:01 AM Kyle Evans <kevans@freebsd.org> = wrote: >>>>=20 >>>> On Mon, Nov 30, 2020 at 3:45 AM Michael Tuexen <tuexen@freebsd.org> = 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, >>=20 >> 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. >>=20 >> 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? >>=20 >=20 > Excellent, thanks! It runs fine on 12.2, but not on an up-to-date > -CURRENT or stable/12 after this MFC. The issue should be fixed by https://reviews.freebsd.org/D28143 With that patch your reproducer terminates immediately, sometimes = reporting tuexen@head:~ % ./tcpr waiting for server attempting to connect got client connected, closing waiting write fail (bad!): 54 and sometimes reporting tuexen@head:~ % ./tcpr waiting for server attempting to connect connected, closing waiting got client pipe gone (good!) but that depends on the timing. Thanks for reporting the issue! Best regards Michael >> 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. >>=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CCD3B200-A002-495C-AEDF-629CF8D4A8AE>