Fix some NewReno handlers in case of being called from other CC modules

[tuexen]

Summary

Some NewReno functions use the internal data, but are also called from other CC modules. Ensure that in this case, the internal data is not accessed.

This should fix:

• panic: ASan: Invalid access, NUM-byte read in newreno_ack_received
• panic: ASan: Invalid access, NUM-byte read in newreno_cong_signal

[rrs]

Overall I think this is a good fix for an ugly problem. I.e. that other
CC algo's use newreno. I think we should re-look at this and have the
"reused" functions put into the kernel as just non-static functions that
can be used by CC modules. Pass in beta etc...

sys/netinet/cc/cc_newreno.c

405

nit double ;; on end

sys/netinet/cc/cc_newreno.c

if (tp->t_logstate != TCP_LOG_STATE_OFF) {
                 TCP_LOG_EVENTP(tp, NULL,
                     &tp->t_inpcb->inp_socket->so_rcv,
                     &tp->t_inpcb->inp_socket->so_snd,
                     TCP_HYSTART, 0,
                     0, &log, false, &tv);
         }
 }
 
-static  int
+static int
 newreno_cb_init(struct cc_var *ccv)
 {
         struct newreno *nreno;
 
-        ccv->cc_data = NULL;
         ccv->cc_data = malloc(sizeof(struct newreno), M_NEWRENO, M_NOWAIT);
         if (ccv->cc_data == NULL)
                 return (ENOMEM);
         nreno = (struct newreno *)ccv->cc_data;
         /* NB: nreno is not zeroed, so initialise all fields. */
         nreno->beta = V_newreno_beta;
         nreno->beta_ecn = V_newreno_beta_ecn;
         /*
newreno_cb_destroy(struct cc_var *ccv)
         free(ccv->cc_data, M_NEWRENO);
 }
 
 static void
 newreno_ack_received(struct cc_var *ccv, uint16_t type)
 {
         struct newreno *nreno;
 
-        nreno = (struct newreno *)ccv->cc_data;
+        /*
+         * Other TCP congestion controls use newreno_ack_received(), but
+         * with their own private cc_data. Make sure the cc_data is used
+         * correctly.
+         */
+        nreno = (CC_ALGO(ccv->ccvc.tcp) == &newreno_cc_algo) ? ccv->cc_data : NULL;
+
         if (type == CC_ACK && !IN_RECOVERY(CCV(ccv, t_flags)) &&
             (ccv->flags & CCF_CWND_LIMITED)) {
                 u_int cw = CCV(ccv, snd_cwnd);
                 u_int incr = CCV(ccv, t_maxseg);
 
                 /*
                  * Regular in-order ACK, open the congestion window.
                  * Method depends on which congestion control state we're
if (type == CC_ACK && !IN_RECOVERY(CCV(ccv, t_flags)) &&
                  *
                  * cong avoid without ABC (RFC 5681):
                  *   Grow cwnd linearly by approximately maxseg per RTT using
                  *   maxseg^2 / cwnd per ACK as the increment.
                  *   If cwnd > maxseg^2, fix the cwnd increment at 1 byte to
                  *   avoid capping cwnd.
                  */
                 if (cw > CCV(ccv, snd_ssthresh)) {
-                        if (nreno->newreno_flags & CC_NEWRENO_HYSTART_IN_CSS) {
+                        if ((nreno != NULL) &&
+                            (nreno->newreno_flags & CC_NEWRENO_HYSTART_IN_CSS)) {
                                 /*
                                  * We have slipped into CA with
                                  * CSS active. Deactivate all.
                                  */
                                 /* Turn off the CSS flag */
                                 nreno->newreno_flags &= ~CC_NEWRENO_HYSTART_IN_CSS;
                                 /* Disable use of CSS in the future except long idle  */
                                 nreno->newreno_flags &= ~CC_NEWRENO_HYSTART_ENABLED;
if (cw > CCV(ccv, snd_ssthresh)) {
                          * doesn't rely on tcpcb vars.
                          */
                         uint16_t abc_val;
 
                         if (ccv->flags & CCF_USE_LOCAL_ABC)
                                 abc_val = ccv->labc;
                         else
                                 abc_val = V_tcp_abc_l_var;
-                        if ((nreno->newreno_flags & CC_NEWRENO_HYSTART_ALLOWED) &&
+                        if ((nreno != NULL) &&
+                            (nreno->newreno_flags & CC_NEWRENO_HYSTART_ALLOWED) &&
                             (nreno->newreno_flags & CC_NEWRENO_HYSTART_ENABLED) &&
                             ((nreno->newreno_flags & CC_NEWRENO_HYSTART_IN_CSS) == 0)) {
                                 /*
                                  * Hystart is allowed and still enabled and we are not yet
                                  * in CSS. Lets check to see if we can make a decision on
                                  * if we need to go into CSS.
                                  */
                                 if ((nreno->css_rttsample_count >= hystart_n_rttsamples) &&
if (cw > CCV(ccv, snd_ssthresh)) {
                         if (CCV(ccv, snd_nxt) == CCV(ccv, snd_max))
                                 incr = min(ccv->bytes_this_ack,
                                     ccv->nsegs * abc_val *
                                     CCV(ccv, t_maxseg));
                         else
                                 incr = min(ccv->bytes_this_ack, CCV(ccv, t_maxseg));
 
                         /* Only if Hystart is enabled will the flag get set */
-                        if (nreno->newreno_flags & CC_NEWRENO_HYSTART_IN_CSS) {
+                        if ((nreno != NULL) &&
+                            (nreno->newreno_flags & CC_NEWRENO_HYSTART_IN_CSS)) {
                                 incr /= hystart_css_growth_div;
                                 newreno_log_hystart_event(ccv, nreno, 3, incr);
                         }
                 }
                 /* ABC is on by default, so incr equals 0 frequently. */
                 if (incr > 0)
                         CCV(ccv, snd_cwnd) = min(cw + incr,
                             TCP_MAXWIN << CCV(ccv, snd_scale));
         }
 }
 
 static void
 newreno_after_idle(struct cc_var *ccv)
 {
         struct newreno *nreno;
         uint32_t rw;
 
-        nreno = (struct newreno *)ccv->cc_data;
         /*
+         * Other TCP congestion controls use newreno_after_idle(), but
+         * with their own private cc_data. Make sure the cc_data is used
+         * correctly.
+         */
+        nreno = (CC_ALGO(ccv->ccvc.tcp) == &newreno_cc_algo) ? ccv->cc_data : NULL;
+        /*
          * If we've been idle for more than one retransmit timeout the old
          * congestion window is no longer current and we have to reduce it to
          * the restart window before we can transmit again.
          *
          * The restart window is the initial window or the last CWND, whichever
          * is smaller.
          *
          * This is done to prevent us from flooding the path with a full CWND at
          * wirespeed, overloading router and switch buffers along the way.
          *
          * See RFC5681 Section 4.1. "Restarting Idle Connections".
          *
          * In addition, per RFC2861 Section 2, the ssthresh is set to the
          * maximum of the former ssthresh or 3/4 of the old cwnd, to
          * not exit slow-start prematurely.
          */
         rw = tcp_compute_initwnd(tcp_maxseg(ccv->ccvc.tcp));
 
         CCV(ccv, snd_ssthresh) = max(CCV(ccv, snd_ssthresh),
             CCV(ccv, snd_cwnd)-(CCV(ccv, snd_cwnd)>>2));
 
         CCV(ccv, snd_cwnd) = min(rw, CCV(ccv, snd_cwnd));
-        if ((nreno->newreno_flags & CC_NEWRENO_HYSTART_ENABLED) == 0) {
+        if ((nreno != NULL) &&
+            (nreno->newreno_flags & CC_NEWRENO_HYSTART_ENABLED) == 0) {
                 if (CCV(ccv, snd_cwnd) <= (hystart_lowcwnd * tcp_fixed_maxseg(ccv->ccvc.tcp))) {
                         /*
                          * Re-enable hystart if our cwnd has fallen below
                          * the hystart lowcwnd point.
                          */
                         nreno->newreno_flags &= ~CC_NEWRENO_HYSTART_IN_CSS;
                         nreno->newreno_flags |= CC_NEWRENO_HYSTART_ENABLED;
                 }
         }
 }
 
 /*
  * Perform any necessary tasks before we enter congestion recovery.
  */
 static void
 newreno_cong_signal(struct cc_var *ccv, uint32_t type)
 {
         struct newreno *nreno;
         uint32_t beta, beta_ecn, cwin, factor;
         u_int mss;
 
         cwin = CCV(ccv, snd_cwnd);
         mss = tcp_fixed_maxseg(ccv->ccvc.tcp);
-        nreno = (struct newreno *) ccv->cc_data;
-        beta = nreno->beta;
-        beta_ecn = nreno->beta_ecn;
         /*
+         * Other TCP congestion controls use newreno_cong_signal(), but
+         * with their own private cc_data. Make sure the cc_data is used
+         * correctly.
+         */
+        nreno = (CC_ALGO(ccv->ccvc.tcp) == &newreno_cc_algo) ? ccv->cc_data : NULL;
+        beta = (nreno == NULL) ? V_newreno_beta : nreno->beta;;
+        beta_ecn = (nreno == NULL) ? V_newreno_beta_ecn : nreno->beta_ecn;
+        /*
          * Note that we only change the backoff for ECN if the
          * global sysctl V_cc_do_abe is set <or> the stack itself
          * has set a flag in our newreno_flags (due to pacing) telling
          * us to use the lower valued back-off.
          */
         if ((type == CC_ECN) &&
             (V_cc_do_abe ||
             ((nreno != NULL) && (nreno->newreno_flags & CC_NEWRENO_BETA_ECN_ENABLED))))
                 factor = beta_ecn;
         else
                 factor = beta;
 
         /* Catch algos which mistakenly leak private signal types. */
         KASSERT((type & CC_SIGPRIVMASK) == 0,
             ("%s: congestion signal type 0x%08x is private\n", __func__, type));
 
         cwin = max(((uint64_t)cwin * (uint64_t)factor) / (100ULL * (uint64_t)mss),
             2) * mss;
 
         switch (type) {
         case CC_NDUPACK:
-                if (nreno->newreno_flags & CC_NEWRENO_HYSTART_ENABLED) {
+                if ((nreno != NULL) &&
+                    (nreno->newreno_flags & CC_NEWRENO_HYSTART_ENABLED)) {
                         /* Make sure the flags are all off we had a loss */
                         nreno->newreno_flags &= ~CC_NEWRENO_HYSTART_ENABLED;
                         nreno->newreno_flags &= ~CC_NEWRENO_HYSTART_IN_CSS;
                 }
                 if (!IN_FASTRECOVERY(CCV(ccv, t_flags))) {
                         if (IN_CONGRECOVERY(CCV(ccv, t_flags) &&
                             V_cc_do_abe && V_cc_abe_frlossreduce)) {
                                 CCV(ccv, snd_ssthresh) =
                                     ((uint64_t)CCV(ccv, snd_ssthresh) *
                                      (uint64_t)beta) / (uint64_t)beta_ecn;
                         }
                         if (!IN_CONGRECOVERY(CCV(ccv, t_flags)))
                                 CCV(ccv, snd_ssthresh) = cwin;
                         ENTER_RECOVERY(CCV(ccv, t_flags));
                 }
                 break;
         case CC_ECN:
-                if (nreno->newreno_flags & CC_NEWRENO_HYSTART_ENABLED) {
+                if ((nreno != NULL) &&
+                    (nreno->newreno_flags & CC_NEWRENO_HYSTART_ENABLED)) {
                         /* Make sure the flags are all off we had a loss */
                         nreno->newreno_flags &= ~CC_NEWRENO_HYSTART_ENABLED;
                         nreno->newreno_flags &= ~CC_NEWRENO_HYSTART_IN_CSS;
                 }
                 if (!IN_CONGRECOVERY(CCV(ccv, t_flags))) {
                         CCV(ccv, snd_ssthresh) = cwin;
                         CCV(ccv, snd_cwnd) = cwin;
                         ENTER_CONGRECOVERY(CCV(ccv, t_flags));