git: bc7ee8e5bc55 - main - Address panic with PRR due to missed initialization of recover_fs

[Richard Scheffenegger]

The branch main has been updated by rscheff:

URL: https://cgit.FreeBSD.org/src/commit/?id=bc7ee8e5bc555c246bad8bbb9cdf964fa0a08f41

commit bc7ee8e5bc555c246bad8bbb9cdf964fa0a08f41
Author:     Richard Scheffenegger <srichard at netapp.com>
AuthorDate: 2021-01-20 11:06:34 +0000
Commit:     Richard Scheffenegger <rscheff at FreeBSD.org>
CommitDate: 2021-01-20 11:06:34 +0000

    Address panic with PRR due to missed initialization of recover_fs
    
    Summary:
    When using the base stack in conjunction with RACK, it appears that
    infrequently, ++tp->t_dupacks is instantly larger than tcprexmtthresh.
    
    This leaves the recover flightsize (sackhint.recover_fs) uninitialized,
    leading to a div/0 panic.
    
    Address this by properly initializing the variable just prior to first
    use, if it is not properly initialized.
    
    In order to prevent stale information from a prior recovery to
    negatively impact the PRR calculations in this event, also clear
    recover_fs once loss recovery is finished.
    
    Finally, improve the readability of the initialization of recover_fs
    when t_dupacks == tcprexmtthresh by adjusting the indentation and
    using the max(1, snd_nxt - snd_una) macro.
    
    Reviewers: rrs, kbowling, tuexen, jtl, #transport, gnn!, jmg, manu, #manpages
    
    Reviewed By: rrs, kbowling, #transport
    
    Subscribers: bdrewery, andrew, rpokala, ae, emaste, bz, bcran, #linuxkpi, imp, melifaro
    
    Differential Revision: https://reviews.freebsd.org/D28114
---
 sys/netinet/tcp_input.c | 20 ++++++++++++--------
 sys/netinet/tcp_subr.c  | 10 ++++++++++
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/sys/netinet/tcp_input.c b/sys/netinet/tcp_input.c
index 283d42b594bd..75718352da00 100644
--- a/sys/netinet/tcp_input.c
+++ b/sys/netinet/tcp_input.c
@@ -510,6 +510,7 @@ cc_post_recovery(struct tcpcb *tp, struct tcphdr *th)
 	}
 	/* XXXLAS: EXIT_RECOVERY ? */
 	tp->t_bytes_acked = 0;
+	tp->sackhint.recover_fs = 0;
 }
 
 /*
@@ -2590,6 +2591,9 @@ tcp_do_segment(struct mbuf *m, struct tcphdr *th, struct socket *so,
 							tp->sackhint.sack_bytes_rexmit;
 						tp->sackhint.prr_delivered += del_data;
 						if (pipe > tp->snd_ssthresh) {
+							if (tp->sackhint.recover_fs == 0)
+								tp->sackhint.recover_fs =
+								    max(1, tp->snd_nxt - tp->snd_una);
 							snd_cnt = (tp->sackhint.prr_delivered *
 							    tp->snd_ssthresh /
 							    tp->sackhint.recover_fs) +
@@ -2677,14 +2681,14 @@ tcp_do_segment(struct mbuf *m, struct tcphdr *th, struct socket *so,
 					tcp_timer_activate(tp, TT_REXMT, 0);
 					tp->t_rtttime = 0;
 					if (V_tcp_do_prr) {
-					    /*
-					     * snd_ssthresh is already updated by
-					     * cc_cong_signal.
-					     */
-					    tp->sackhint.prr_delivered = 0;
-					    tp->sackhint.sack_bytes_rexmit = 0;
-					    if (!(tp->sackhint.recover_fs = tp->snd_nxt - tp->snd_una))
-						tp->sackhint.recover_fs = 1;
+						/*
+						 * snd_ssthresh is already updated by
+						 * cc_cong_signal.
+						 */
+						tp->sackhint.prr_delivered = 0;
+						tp->sackhint.sack_bytes_rexmit = 0;
+						tp->sackhint.recover_fs = max(1,
+						    tp->snd_nxt - tp->snd_una);
 					}
 					if (tp->t_flags & TF_SACK_PERMIT) {
 						TCPSTAT_INC(
diff --git a/sys/netinet/tcp_subr.c b/sys/netinet/tcp_subr.c
index b39d2b43d3a2..c49ff680d201 100644
--- a/sys/netinet/tcp_subr.c
+++ b/sys/netinet/tcp_subr.c
@@ -742,6 +742,16 @@ tcp_default_fb_init(struct tcpcb *tp)
 		    TCPS_HAVEESTABLISHED(tp->t_state) ? TP_KEEPIDLE(tp) :
 		    TP_KEEPINIT(tp));
 
+	/*
+	 * Make sure critical variables are initialized
+	 * if transitioning while in Recovery.
+	 */
+	if IN_FASTRECOVERY(tp->t_flags) {
+		if (tp->sackhint.recover_fs == 0)
+			tp->sackhint.recover_fs = max(1,
+			    tp->snd_nxt - tp->snd_una);
+	}
+
 	return (0);
 }