d/freebsd-skq
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

CC Cubic: fix underflow for cubic_cwnd()

Browse Source 
Singed calculations in cubic_cwnd() can result in negative cwnd
value which is then cast to an unsigned value. Values less than
1 mss are generally bad for other parts of the code, also fixed.

Submitted by:	Jason Eggleston <jason@eggnet.com>
Sponsored by:	Limelight Networks
Differential Revision:	https://reviews.freebsd.org/D14141
This commit is contained in:
Sean Bruno 2018-03-26 19:53:36 +00:00
parent 7953f98e23
commit e2041bfa7c
3 changed files with 52 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
sys/netinet/cc/cc.h
View File

@ -102,6 +102,8 @@ struct cc_var {
#define CCF_ACKNOW 0x0008 /* Will this ack be sent now? */ #define CCF_ACKNOW 0x0008 /* Will this ack be sent now? */
#define CCF_IPHDR_CE 0x0010 /* Does this packet set CE bit? */ #define CCF_IPHDR_CE 0x0010 /* Does this packet set CE bit? */
#define CCF_TCPHDR_CWR 0x0020 /* Does this packet set CWR bit? */ #define CCF_TCPHDR_CWR 0x0020 /* Does this packet set CWR bit? */
#define CCF_MAX_CWND 0x0040 /* Have we reached maximum cwnd? */
#define CCF_CHG_MAX_CWND 0x0080 /* Cubic max_cwnd changed, for K */
/* ACK types passed to the ack_received() hook. */ /* ACK types passed to the ack_received() hook. */
#define CC_ACK 0x0001 /* Regular in sequence ACK. */ #define CC_ACK 0x0001 /* Regular in sequence ACK. */

33
sys/netinet/cc/cc_cubic.c
View File

@ -88,6 +88,8 @@ struct cubic {
unsigned long max_cwnd; unsigned long max_cwnd;
/* cwnd at the previous congestion event. */ /* cwnd at the previous congestion event. */
unsigned long prev_max_cwnd; unsigned long prev_max_cwnd;
/* Cached value for t_maxseg when K was computed */
uint32_t k_maxseg;
/* Number of congestion events. */ /* Number of congestion events. */
uint32_t num_cong_events; uint32_t num_cong_events;
/* Minimum observed rtt in ticks. */ /* Minimum observed rtt in ticks. */
@ -124,6 +126,9 @@ cubic_ack_received(struct cc_var *ccv, uint16_t type)
cubic_data = ccv->cc_data; cubic_data = ccv->cc_data;
cubic_record_rtt(ccv); cubic_record_rtt(ccv);
if (ccv->flags & CCF_MAX_CWND)
return;
/* /*
* Regular ACK and we're not in cong/fast recovery and we're cwnd * Regular ACK and we're not in cong/fast recovery and we're cwnd
* limited and we're either not doing ABC or are slow starting or are * limited and we're either not doing ABC or are slow starting or are
@ -151,6 +156,12 @@ cubic_ack_received(struct cc_var *ccv, uint16_t type)
cubic_data->mean_rtt_ticks, cubic_data->max_cwnd, cubic_data->mean_rtt_ticks, cubic_data->max_cwnd,
CCV(ccv, t_maxseg)); CCV(ccv, t_maxseg));
if (ccv->flags & CCF_CHG_MAX_CWND || cubic_data->k_maxseg != CCV(ccv, t_maxseg)) {
cubic_data->K = cubic_k(cubic_data->max_cwnd / CCV(ccv, t_maxseg));
cubic_data->k_maxseg = CCV(ccv, t_maxseg);
ccv->flags &= ~(CCF_MAX_CWND|CCF_CHG_MAX_CWND);
}
w_cubic_next = cubic_cwnd(ticks_since_cong + w_cubic_next = cubic_cwnd(ticks_since_cong +
cubic_data->mean_rtt_ticks, cubic_data->max_cwnd, cubic_data->mean_rtt_ticks, cubic_data->max_cwnd,
CCV(ccv, t_maxseg), cubic_data->K); CCV(ccv, t_maxseg), cubic_data->K);
@ -162,13 +173,18 @@ cubic_ack_received(struct cc_var *ccv, uint16_t type)
* TCP-friendly region, follow tf * TCP-friendly region, follow tf
* cwnd growth. * cwnd growth.
*/ */
CCV(ccv, snd_cwnd) = w_tf; CCV(ccv, snd_cwnd) = ulmin(w_tf, TCP_MAXWIN << CCV(ccv, snd_scale));
else if (CCV(ccv, snd_cwnd) < w_cubic_next) { else if (CCV(ccv, snd_cwnd) < w_cubic_next) {
/* /*
* Concave or convex region, follow CUBIC * Concave or convex region, follow CUBIC
* cwnd growth. * cwnd growth.
*/ */
if (w_cubic_next >= TCP_MAXWIN << CCV(ccv, snd_scale)) {
w_cubic_next = TCP_MAXWIN << CCV(ccv, snd_scale);
ccv->flags |= CCF_MAX_CWND;
}
w_cubic_next = ulmin(w_cubic_next, TCP_MAXWIN << CCV(ccv, snd_scale));
if (V_tcp_do_rfc3465) if (V_tcp_do_rfc3465)
CCV(ccv, snd_cwnd) = w_cubic_next; CCV(ccv, snd_cwnd) = w_cubic_next;
else else
@ -186,8 +202,10 @@ cubic_ack_received(struct cc_var *ccv, uint16_t type)
* max_cwnd. * max_cwnd.
*/ */
if (cubic_data->num_cong_events == 0 && if (cubic_data->num_cong_events == 0 &&
cubic_data->max_cwnd < CCV(ccv, snd_cwnd)) cubic_data->max_cwnd < CCV(ccv, snd_cwnd)) {
cubic_data->max_cwnd = CCV(ccv, snd_cwnd); cubic_data->max_cwnd = CCV(ccv, snd_cwnd);
ccv->flags |= CCF_CHG_MAX_CWND;
}
} }
} }
} }
@ -238,6 +256,7 @@ cubic_cong_signal(struct cc_var *ccv, uint32_t type)
cubic_data->num_cong_events++; cubic_data->num_cong_events++;
cubic_data->prev_max_cwnd = cubic_data->max_cwnd; cubic_data->prev_max_cwnd = cubic_data->max_cwnd;
cubic_data->max_cwnd = CCV(ccv, snd_cwnd); cubic_data->max_cwnd = CCV(ccv, snd_cwnd);
ccv->flags |= CCF_CHG_MAX_CWND;
} }
ENTER_RECOVERY(CCV(ccv, t_flags)); ENTER_RECOVERY(CCV(ccv, t_flags));
} }
@ -250,6 +269,8 @@ cubic_cong_signal(struct cc_var *ccv, uint32_t type)
cubic_data->prev_max_cwnd = cubic_data->max_cwnd; cubic_data->prev_max_cwnd = cubic_data->max_cwnd;
cubic_data->max_cwnd = CCV(ccv, snd_cwnd); cubic_data->max_cwnd = CCV(ccv, snd_cwnd);
cubic_data->t_last_cong = ticks; cubic_data->t_last_cong = ticks;
ccv->flags |= CCF_CHG_MAX_CWND;
ccv->flags &= ~CCF_MAX_CWND;
CCV(ccv, snd_cwnd) = CCV(ccv, snd_ssthresh); CCV(ccv, snd_cwnd) = CCV(ccv, snd_ssthresh);
ENTER_CONGRECOVERY(CCV(ccv, t_flags)); ENTER_CONGRECOVERY(CCV(ccv, t_flags));
} }
@ -266,6 +287,7 @@ cubic_cong_signal(struct cc_var *ccv, uint32_t type)
if (CCV(ccv, t_rxtshift) >= 2) { if (CCV(ccv, t_rxtshift) >= 2) {
cubic_data->num_cong_events++; cubic_data->num_cong_events++;
cubic_data->t_last_cong = ticks; cubic_data->t_last_cong = ticks;
ccv->flags &= ~CCF_MAX_CWND;
} }
break; break;
} }
@ -284,6 +306,7 @@ cubic_conn_init(struct cc_var *ccv)
* get used. * get used.
*/ */
cubic_data->max_cwnd = CCV(ccv, snd_cwnd); cubic_data->max_cwnd = CCV(ccv, snd_cwnd);
ccv->flags |= CCF_CHG_MAX_CWND;
} }
static int static int
@ -308,9 +331,11 @@ cubic_post_recovery(struct cc_var *ccv)
pipe = 0; pipe = 0;
/* Fast convergence heuristic. */ /* Fast convergence heuristic. */
if (cubic_data->max_cwnd < cubic_data->prev_max_cwnd) if (cubic_data->max_cwnd < cubic_data->prev_max_cwnd) {
cubic_data->max_cwnd = (cubic_data->max_cwnd * CUBIC_FC_FACTOR) cubic_data->max_cwnd = (cubic_data->max_cwnd * CUBIC_FC_FACTOR)
>> CUBIC_SHIFT; >> CUBIC_SHIFT;
ccv->flags |= CCF_CHG_MAX_CWND;
}
if (IN_FASTRECOVERY(CCV(ccv, t_flags))) { if (IN_FASTRECOVERY(CCV(ccv, t_flags))) {
/* /*
@ -333,6 +358,7 @@ cubic_post_recovery(struct cc_var *ccv)
cubic_data->max_cwnd) >> CUBIC_SHIFT)); cubic_data->max_cwnd) >> CUBIC_SHIFT));
} }
cubic_data->t_last_cong = ticks; cubic_data->t_last_cong = ticks;
ccv->flags &= ~CCF_MAX_CWND;
/* Calculate the average RTT between congestion epochs. */ /* Calculate the average RTT between congestion epochs. */
if (cubic_data->epoch_ack_count > 0 && if (cubic_data->epoch_ack_count > 0 &&
@ -343,7 +369,6 @@ cubic_post_recovery(struct cc_var *ccv)
cubic_data->epoch_ack_count = 0; cubic_data->epoch_ack_count = 0;
cubic_data->sum_rtt_ticks = 0; cubic_data->sum_rtt_ticks = 0;
cubic_data->K = cubic_k(cubic_data->max_cwnd / CCV(ccv, t_maxseg));
} }
/* /*

24
sys/netinet/cc/cc_cubic.h
View File

@ -41,6 +41,8 @@
#ifndef _NETINET_CC_CUBIC_H_ #ifndef _NETINET_CC_CUBIC_H_
#define _NETINET_CC_CUBIC_H_ #define _NETINET_CC_CUBIC_H_
#include <sys/limits.h>
/* Number of bits of precision for fixed point math calcs. */ /* Number of bits of precision for fixed point math calcs. */
#define CUBIC_SHIFT 8 #define CUBIC_SHIFT 8
@ -162,8 +164,6 @@ cubic_k(unsigned long wmax_pkts)
/* /*
* Compute the new cwnd value using an implementation of eqn 1 from the I-D. * Compute the new cwnd value using an implementation of eqn 1 from the I-D.
* Thanks to Kip Macy for help debugging this function. * Thanks to Kip Macy for help debugging this function.
*
* XXXLAS: Characterise bounds for overflow.
*/ */
static __inline unsigned long static __inline unsigned long
cubic_cwnd(int ticks_since_cong, unsigned long wmax, uint32_t smss, int64_t K) cubic_cwnd(int ticks_since_cong, unsigned long wmax, uint32_t smss, int64_t K)
@ -175,6 +175,15 @@ cubic_cwnd(int ticks_since_cong, unsigned long wmax, uint32_t smss, int64_t K)
/* t - K, with CUBIC_SHIFT worth of precision. */ /* t - K, with CUBIC_SHIFT worth of precision. */
cwnd = ((int64_t)(ticks_since_cong << CUBIC_SHIFT) - (K * hz)) / hz; cwnd = ((int64_t)(ticks_since_cong << CUBIC_SHIFT) - (K * hz)) / hz;
/* moved this calculation up because it cannot overflow or underflow */
cwnd *= CUBIC_C_FACTOR * smss;
if (cwnd > 2097151) /* 2^21 cubed is long max */
return INT_MAX;
if (cwnd < -2097152) /* -2^21 cubed is long min */
return smss;
/* (t - K)^3, with CUBIC_SHIFT^3 worth of precision. */ /* (t - K)^3, with CUBIC_SHIFT^3 worth of precision. */
cwnd *= (cwnd * cwnd); cwnd *= (cwnd * cwnd);
@ -183,8 +192,17 @@ cubic_cwnd(int ticks_since_cong, unsigned long wmax, uint32_t smss, int64_t K)
* The down shift by CUBIC_SHIFT_4 is because cwnd has 4 lots of * The down shift by CUBIC_SHIFT_4 is because cwnd has 4 lots of
* CUBIC_SHIFT included in the value. 3 from the cubing of cwnd above, * CUBIC_SHIFT included in the value. 3 from the cubing of cwnd above,
* and an extra from multiplying through by CUBIC_C_FACTOR. * and an extra from multiplying through by CUBIC_C_FACTOR.
*
* The original formula was this:
* cwnd = ((cwnd * CUBIC_C_FACTOR * smss) >> CUBIC_SHIFT_4) + wmax;
*
* CUBIC_C_FACTOR and smss factors were moved up to an earlier
* calculation to simplify overflow and underflow detection.
*/ */
cwnd = ((cwnd * CUBIC_C_FACTOR * smss) >> CUBIC_SHIFT_4) + wmax; cwnd = (cwnd >> CUBIC_SHIFT_4) + wmax;
if (cwnd < 0)
return 1;
return ((unsigned long)cwnd); return ((unsigned long)cwnd);
} }