svn commit: r368181 - in stable/12/sys/netinet: . tcp_stacks

Michael Tuexen tuexen at freebsd.org
Thu Jan 14 21:19:14 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.
Fixed in https://cgit.FreeBSD.org/src/commit/?id=cc3c34859eab1b317d0f38731355b53f7d978c97

Thanks again for the report!

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