svn commit: r368181 - in stable/12/sys/netinet: . tcp_stacks
Michael Tuexen
tuexen at freebsd.org
Wed Jan 13 22:57:04 UTC 2021
> On 13. Jan 2021, at 16:33, Kyle Evans <kevans at FreeBSD.org> wrote:
>
> On Wed, Jan 13, 2021 at 9:31 AM Michael Tuexen <tuexen at freebsd.org> wrote:
>>
>>> On 13. Jan 2021, at 16:16, Kyle Evans <kevans at FreeBSD.org> wrote:
>>>
>>> On Wed, Jan 6, 2021 at 9:01 AM Kyle Evans <kevans at freebsd.org> wrote:
>>>>
>>>> On Mon, Nov 30, 2020 at 3:45 AM Michael Tuexen <tuexen at 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 at 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 at 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.
The issue should be fixed by https://reviews.freebsd.org/D28143
With that patch your reproducer terminates immediately, sometimes reporting
tuexen at head:~ % ./tcpr
waiting for server
attempting to connect
got client
connected, closing
waiting
write fail (bad!): 54
and sometimes reporting
tuexen at 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
>>>
>>>>>
>>>>> 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.
>>
More information about the svn-src-all
mailing list