From owner-dev-commits-src-main@freebsd.org Fri Jun 4 17:25:53 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id CDB176517A4 for ; Fri, 4 Jun 2021 17:25:53 +0000 (UTC) (envelope-from kevin.bowling@kev009.com) Received: from mail-yb1-xb35.google.com (mail-yb1-xb35.google.com [IPv6:2607:f8b0:4864:20::b35]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4FxV694xngz4V3K for ; Fri, 4 Jun 2021 17:25:53 +0000 (UTC) (envelope-from kevin.bowling@kev009.com) Received: by mail-yb1-xb35.google.com with SMTP id i4so14762991ybe.2 for ; Fri, 04 Jun 2021 10:25:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kev009.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=e/alwdZCkY2h5t/0QE76NPYO6PUg57z/n+2NyR8ug9w=; b=luRNktFMITVdnk1wc0aGPBXS0VMJY5/IvNiXdAtkS0rXph5R1c2EvdMek+ZwAt2Wlk 9WTGzZjpKOUpD9yfK3PC++glYbOI7jFEJcg+uec8o04Ws99WXFtW4DNeEYjb20gSy+nE 9kPFe98+NCIva1nFoYliXhrxcqGa5bv1EKSYI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=e/alwdZCkY2h5t/0QE76NPYO6PUg57z/n+2NyR8ug9w=; b=Oa/LPWGf+e+kg2mROdNxtCqR7oJY7Ei/Px8c2VLy8S3H5kspHfub/B4g0qTg4ud/2v pG+J50WiOIjoSX9P/3YOJFibkPNYSKR90VlHWQIyAnwEnSep+2OjmQHctgNPzKRe5a/x 2Ecbm0O+v6BJZLsazSMmbx+0xKpbyWwUCiYL3UTBVdB9AXdz9DD+xQoUgqSOelK1Y3tn WqLE3jSGXEinZFmviL5i0lDBiE/SfmsWwBRGj4F0EMc7BXQWuOvadF5YJyYAh/vFr4dE duzBjij3WO8xp6zqbDaremoxOXkSB3R7E8PM9oDS+nLzUgFqOtSFBUB4WY0TiIvppkWA jBcA== X-Gm-Message-State: AOAM530Vb4kXJ7eucrr3iezsYNRbSTU+czoN+U1Cl3LsXMJJT627U+0Y odYlTUcPXqAJenMbE2ueDOeQuH+renhH6zzbHNZzMw== X-Google-Smtp-Source: ABdhPJwuBp19ApiYm2EHZOPjz4xJ8aWSeH7bgpN0Q48wV1m44dJsA3xc2qtOjbNeDCoydNm4HaVfSgAL78npbJ0vteI= X-Received: by 2002:a25:ba10:: with SMTP id t16mr6657726ybg.87.1622827552472; Fri, 04 Jun 2021 10:25:52 -0700 (PDT) MIME-Version: 1.0 References: <202106040929.1549T8ad013290@gitrepo.freebsd.org> In-Reply-To: <202106040929.1549T8ad013290@gitrepo.freebsd.org> From: Kevin Bowling Date: Fri, 4 Jun 2021 10:25:40 -0700 Message-ID: Subject: Re: git: 4747500deaaa - main - tcp: A better fix for the previously attempted fix of the ack-war issue with tcp. To: Randall Stewart Cc: src-committers , "" , "dev-commits-src-main@FreeBSD.org" Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4FxV694xngz4V3K X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Jun 2021 17:25:53 -0000 Will you MFC this? On Fri, Jun 4, 2021 at 2:29 AM Randall Stewart wrote: > > The branch main has been updated by rrs: > > URL: https://cgit.FreeBSD.org/src/commit/?id=4747500deaaa7765ba1c0413197c23ddba4faf49 > > commit 4747500deaaa7765ba1c0413197c23ddba4faf49 > Author: Randall Stewart > AuthorDate: 2021-06-04 09:26:43 +0000 > Commit: Randall Stewart > CommitDate: 2021-06-04 09:26:43 +0000 > > tcp: A better fix for the previously attempted fix of the ack-war issue with tcp. > > So it turns out that my fix before was not correct. It ended with us failing > some of the "improved" SYN tests, since we are not in the correct states. > With more digging I have figured out the root of the problem is that when > we receive a SYN|FIN the reassembly code made it so we create a segq entry > to hold the FIN. In the established state where we were not in order this > would be correct i.e. a 0 len with a FIN would need to be accepted. But > if you are in a front state we need to strip the FIN so we correctly handle > the ACK but ignore the FIN. This gets us into the proper states > and avoids the previous ack war. > > I back out some of the previous changes but then add a new change > here in tcp_reass() that fixes the root cause of the issue. We still > leave the rack panic fixes in place however. > > Reviewed by: mtuexen > Sponsored by: Netflix Inc > Differential Revision: https://reviews.freebsd.org/D30627 > --- > sys/netinet/tcp_input.c | 12 +++--------- > sys/netinet/tcp_reass.c | 14 ++++++++++++++ > sys/netinet/tcp_stacks/bbr.c | 12 +++--------- > sys/netinet/tcp_stacks/rack.c | 13 ++++--------- > sys/netinet/tcp_usrreq.c | 16 ---------------- > 5 files changed, 24 insertions(+), 43 deletions(-) > > diff --git a/sys/netinet/tcp_input.c b/sys/netinet/tcp_input.c > index 1dab2e511d95..e71a11bdef05 100644 > --- a/sys/netinet/tcp_input.c > +++ b/sys/netinet/tcp_input.c > @@ -3191,15 +3191,9 @@ dodata: /* XXX */ > * when trimming from the head. > */ > tcp_seq temp = save_start; > - if (tlen || (th->th_seq != tp->rcv_nxt)) { > - /* > - * We add the th_seq != rcv_nxt to > - * catch the case of a stand alone out > - * of order FIN. > - */ > - thflags = tcp_reass(tp, th, &temp, &tlen, m); > - tp->t_flags |= TF_ACKNOW; > - } > + > + thflags = tcp_reass(tp, th, &temp, &tlen, m); > + tp->t_flags |= TF_ACKNOW; > } > if ((tp->t_flags & TF_SACK_PERMIT) && > (save_tlen > 0) && > diff --git a/sys/netinet/tcp_reass.c b/sys/netinet/tcp_reass.c > index 8e24e7412473..5b9255da3acf 100644 > --- a/sys/netinet/tcp_reass.c > +++ b/sys/netinet/tcp_reass.c > @@ -571,6 +571,7 @@ tcp_reass(struct tcpcb *tp, struct tcphdr *th, tcp_seq *seq_start, > * the rcv_nxt <-> rcv_wnd but thats > * already done for us by the caller. > */ > +strip_fin: > #ifdef TCP_REASS_COUNTERS > counter_u64_add(tcp_zero_input, 1); > #endif > @@ -579,6 +580,19 @@ tcp_reass(struct tcpcb *tp, struct tcphdr *th, tcp_seq *seq_start, > tcp_reass_log_dump(tp); > #endif > return (0); > + } else if ((*tlenp == 0) && > + (th->th_flags & TH_FIN) && > + !TCPS_HAVEESTABLISHED(tp->t_state)) { > + /* > + * We have not established, and we > + * have a FIN and no data. Lets treat > + * this as the same as if the FIN were > + * not present. We don't want to save > + * the FIN bit in a reassembly buffer > + * we want to get established first before > + * we do that (the peer will retransmit). > + */ > + goto strip_fin; > } > /* > * Will it fit? > diff --git a/sys/netinet/tcp_stacks/bbr.c b/sys/netinet/tcp_stacks/bbr.c > index 5b97c3d7800f..f6388c39cad3 100644 > --- a/sys/netinet/tcp_stacks/bbr.c > +++ b/sys/netinet/tcp_stacks/bbr.c > @@ -8320,15 +8320,9 @@ bbr_process_data(struct mbuf *m, struct tcphdr *th, struct socket *so, > * trimming from the head. > */ > tcp_seq temp = save_start; > - if (tlen || (th->th_seq != tp->rcv_nxt)) { > - /* > - * We add the th_seq != rcv_nxt to > - * catch the case of a stand alone out > - * of order FIN. > - */ > - thflags = tcp_reass(tp, th, &temp, &tlen, m); > - tp->t_flags |= TF_ACKNOW; > - } > + > + thflags = tcp_reass(tp, th, &temp, &tlen, m); > + tp->t_flags |= TF_ACKNOW; > } > if ((tp->t_flags & TF_SACK_PERMIT) && > (save_tlen > 0) && > diff --git a/sys/netinet/tcp_stacks/rack.c b/sys/netinet/tcp_stacks/rack.c > index 1db09e30d968..98b8ff836ca5 100644 > --- a/sys/netinet/tcp_stacks/rack.c > +++ b/sys/netinet/tcp_stacks/rack.c > @@ -10235,15 +10235,10 @@ rack_process_data(struct mbuf *m, struct tcphdr *th, struct socket *so, > * trimming from the head. > */ > tcp_seq temp = save_start; > - if (tlen || (th->th_seq != tp->rcv_nxt)) { > - /* > - * We add the th_seq != rcv_nxt to > - * catch the case of a stand alone out > - * of order FIN. > - */ > - thflags = tcp_reass(tp, th, &temp, &tlen, m); > - tp->t_flags |= TF_ACKNOW; > - } > + > + thflags = tcp_reass(tp, th, &temp, &tlen, m); > + tp->t_flags |= TF_ACKNOW; > + > } > if ((tp->t_flags & TF_SACK_PERMIT) && > (save_tlen > 0) && > diff --git a/sys/netinet/tcp_usrreq.c b/sys/netinet/tcp_usrreq.c > index 7f1b698408e5..b1fc584f93b2 100644 > --- a/sys/netinet/tcp_usrreq.c > +++ b/sys/netinet/tcp_usrreq.c > @@ -2647,22 +2647,6 @@ tcp_usrclosed(struct tcpcb *tp) > tcp_state_change(tp, TCPS_LAST_ACK); > break; > } > - if ((tp->t_state == TCPS_LAST_ACK) && > - (tp->t_flags & TF_SENTFIN)) { > - /* > - * If we have reached LAST_ACK, and > - * we sent a FIN (e.g. via MSG_EOR), then > - * we really should move to either FIN_WAIT_1 > - * or FIN_WAIT_2 depending on snd_max/snd_una. > - */ > - if (tp->snd_una == tp->snd_max) { > - /* The FIN is acked */ > - tcp_state_change(tp, TCPS_FIN_WAIT_2); > - } else { > - /* The FIN is still outstanding */ > - tcp_state_change(tp, TCPS_FIN_WAIT_1); > - } > - } > if (tp->t_state >= TCPS_FIN_WAIT_2) { > soisdisconnected(tp->t_inpcb->inp_socket); > /* Prevent the connection hanging in FIN_WAIT_2 forever. */ > _______________________________________________ > dev-commits-src-main@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/dev-commits-src-main > To unsubscribe, send any mail to "dev-commits-src-main-unsubscribe@freebsd.org"