Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 13 Jan 2021 09:16:38 -0600
From:      Kyle Evans <kevans@freebsd.org>
To:        Michael Tuexen <tuexen@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:  <CACNAnaGomHxXMbLV6OFSH7Mhv%2BDoC7Q-u0Q7r_T5ChjNK5jHXw@mail.gmail.com>
In-Reply-To: <CACNAnaEHH9e47hveitj_X-Zsh%2B_-5REckjA3_ZZbPJKGzgmWRA@mail.gmail.com>
References:  <202011300945.0AU9jilR008960@repo.freebsd.org> <CACNAnaEHH9e47hveitj_X-Zsh%2B_-5REckjA3_ZZbPJKGzgmWRA@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jan 6, 2021 at 9:01 AM Kyle Evans <kevans@freebsd.org> wrote:
>
> On Mon, Nov 30, 2020 at 3:45 AM Michael Tuexen <tuexen@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?

> >
> > 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.



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaGomHxXMbLV6OFSH7Mhv%2BDoC7Q-u0Q7r_T5ChjNK5jHXw>