From owner-dev-commits-src-all@freebsd.org Thu Sep 23 15:44:54 2021 Return-Path: Delivered-To: dev-commits-src-all@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 C1E316A8B6C; Thu, 23 Sep 2021 15:44:54 +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 4HFfcQ4jNfz4vnw; Thu, 23 Sep 2021 15:44:54 +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 813C32DB00; Thu, 23 Sep 2021 15:44:54 +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 18NFissU043837; Thu, 23 Sep 2021 15:44:54 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 18NFis4A043836; Thu, 23 Sep 2021 15:44:54 GMT (envelope-from git) Date: Thu, 23 Sep 2021 15:44:54 GMT Message-Id: <202109231544.18NFis4A043836@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: 1ca931a540cb - main - tcp: Rack compressed ack path updates the recv window too easily 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: 1ca931a540cbb1891f535832ee6ef40ae3ed3910 Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Sep 2021 15:44:54 -0000 The branch main has been updated by rrs: URL: https://cgit.FreeBSD.org/src/commit/?id=1ca931a540cbb1891f535832ee6ef40ae3ed3910 commit 1ca931a540cbb1891f535832ee6ef40ae3ed3910 Author: Randall Stewart AuthorDate: 2021-09-23 15:43:29 +0000 Commit: Randall Stewart CommitDate: 2021-09-23 15:43:29 +0000 tcp: Rack compressed ack path updates the recv window too easily The compressed ack path of rack is not following proper procedures in updating the peers window. It should be checking the seq and ack values before updating and instead it is blindly updating the values. This could in theory get the wrong window in the connection for some length of time. Reviewed by: tuexen Sponsored by: Netflix Inc. Differential Revision: https://reviews.freebsd.org/D32082 --- sys/netinet/tcp_stacks/rack.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/sys/netinet/tcp_stacks/rack.c b/sys/netinet/tcp_stacks/rack.c index 23b1ff1fc584..2369a1c368bf 100644 --- a/sys/netinet/tcp_stacks/rack.c +++ b/sys/netinet/tcp_stacks/rack.c @@ -13116,10 +13116,28 @@ rack_timer_audit(struct tcpcb *tp, struct tcp_rack *rack, struct sockbuf *sb) static void rack_do_win_updates(struct tcpcb *tp, struct tcp_rack *rack, uint32_t tiwin, uint32_t seq, uint32_t ack, uint32_t cts, uint32_t high_seq) { - tp->snd_wnd = tiwin; - rack_validate_fo_sendwin_up(tp, rack); - tp->snd_wl1 = seq; - tp->snd_wl2 = ack; + if ((SEQ_LT(tp->snd_wl1, seq) || + (tp->snd_wl1 == seq && (SEQ_LT(tp->snd_wl2, ack) || + (tp->snd_wl2 == ack && tiwin > tp->snd_wnd))))) { + /* keep track of pure window updates */ + if ((tp->snd_wl2 == ack) && (tiwin > tp->snd_wnd)) + KMOD_TCPSTAT_INC(tcps_rcvwinupd); + tp->snd_wnd = tiwin; + rack_validate_fo_sendwin_up(tp, rack); + tp->snd_wl1 = seq; + tp->snd_wl2 = ack; + if (tp->snd_wnd > tp->max_sndwnd) + tp->max_sndwnd = tp->snd_wnd; + rack->r_wanted_output = 1; + } else if ((tp->snd_wl2 == ack) && (tiwin < tp->snd_wnd)) { + tp->snd_wnd = tiwin; + rack_validate_fo_sendwin_up(tp, rack); + tp->snd_wl1 = seq; + tp->snd_wl2 = ack; + } else { + /* Not a valid win update */ + return; + } if (tp->snd_wnd > tp->max_sndwnd) tp->max_sndwnd = tp->snd_wnd; if (tp->snd_wnd < (tp->snd_max - high_seq)) { @@ -13324,8 +13342,8 @@ rack_do_compressed_ack_processing(struct tcpcb *tp, struct socket *so, struct mb struct tcp_ackent *ae; uint32_t tiwin, ms_cts, cts, acked, acked_amount, high_seq, win_seq, the_win, win_upd_ack; int cnt, i, did_out, ourfinisacked = 0; - int win_up_req = 0; struct tcpopt to_holder, *to = NULL; + int win_up_req = 0; int nsegs = 0; int under_pacing = 1; int recovery = 0; @@ -13519,11 +13537,11 @@ rack_do_compressed_ack_processing(struct tcpcb *tp, struct socket *so, struct mb rack_strike_dupack(rack); } else if (ae->ack_val_set == ACK_RWND) { /* Case C */ - win_up_req = 1; win_upd_ack = ae->ack; win_seq = ae->seq; the_win = tiwin; + rack_do_win_updates(tp, rack, the_win, win_seq, win_upd_ack, cts, high_seq); } else { /* Case A */ if (SEQ_GT(ae->ack, tp->snd_max)) { @@ -13540,10 +13558,10 @@ rack_do_compressed_ack_processing(struct tcpcb *tp, struct socket *so, struct mb nsegs++; /* If the window changed setup to update */ if (tiwin != tp->snd_wnd) { - win_up_req = 1; win_upd_ack = ae->ack; win_seq = ae->seq; the_win = tiwin; + rack_do_win_updates(tp, rack, the_win, win_seq, win_upd_ack, cts, high_seq); } #ifdef TCP_ACCOUNTING /* Account for the acks */ @@ -13592,9 +13610,6 @@ rack_do_compressed_ack_processing(struct tcpcb *tp, struct socket *so, struct mb ts_val = get_cyclecount(); #endif acked_amount = acked = (high_seq - tp->snd_una); - if (win_up_req) { - rack_do_win_updates(tp, rack, the_win, win_seq, win_upd_ack, cts, high_seq); - } if (acked) { if (rack->sack_attack_disable == 0) rack_do_decay(rack);