From owner-dev-commits-src-main@freebsd.org Fri Jun 4 09:29:09 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 01C5564B423; Fri, 4 Jun 2021 09:29:09 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4FxHX46Z1Xz4hSP; Fri, 4 Jun 2021 09:29:08 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id C93251A872; Fri, 4 Jun 2021 09:29:08 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 1549T81S013291; Fri, 4 Jun 2021 09:29:08 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 1549T8ad013290; Fri, 4 Jun 2021 09:29:08 GMT (envelope-from git) Date: Fri, 4 Jun 2021 09:29:08 GMT Message-Id: <202106040929.1549T8ad013290@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Randall Stewart Subject: git: 4747500deaaa - main - tcp: A better fix for the previously attempted fix of the ack-war issue with tcp. MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: rrs X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 4747500deaaa7765ba1c0413197c23ddba4faf49 Auto-Submitted: auto-generated 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 09:29:09 -0000 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. */