> On 13. Jan 2021, at 16:16, Kyle Evans <kev...@freebsd.org> wrote:
>
> On Wed, Jan 6, 2021 at 9:01 AM Kyle Evans <kev...@freebsd.org> wrote:
>>
>> On Mon, Nov 30, 2020 at 3:45 AM Michael Tuexen <tue...@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?
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.
_______________________________________________
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"
