aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMatt Macy <mmacy@FreeBSD.org>2018-07-22 05:37:58 +0000
committerMatt Macy <mmacy@FreeBSD.org>2018-07-22 05:37:58 +0000
commit2269988749d2e4b8519d32244eaf06e4314eae42 (patch)
treed3ed46f02f41c98c0bcd819289573495c9705de0
parent8707733f71a14cf104779b32cc0cb3ed817ae836 (diff)
downloadsrc-2269988749d2e4b8519d32244eaf06e4314eae42.tar.gz
src-2269988749d2e4b8519d32244eaf06e4314eae42.zip
NULL out cc_data in pluggable TCP {cc}_cb_destroy
When ABE was added (rS331214) to NewReno and leak fixed (rS333699) , it now has a destructor (newreno_cb_destroy) for per connection state. Other congestion controls may allocate and free cc_data on entry and exit, but the field is never explicitly NULLed if moving back to NewReno which only internally allocates stateful data (no entry contstructor) resulting in a situation where newreno_cb_destory might be called on a junk pointer. - NULL out cc_data in the framework after calling {cc}_cb_destroy - free(9) checks for NULL so there is no need to perform not NULL checks before calling free. - Improve a comment about NewReno in tcp_ccalgounload This is the result of a debugging session from Jason Wolfe, Jason Eggleston, and mmacy@ and very helpful insight from lstewart@. Submitted by: Kevin Bowling Reviewed by: lstewart Sponsored by: Limelight Networks Differential Revision: https://reviews.freebsd.org/D16282
Notes
Notes: svn path=/head/; revision=336596
-rw-r--r--sys/netinet/cc/cc_chd.c3
-rw-r--r--sys/netinet/cc/cc_cubic.c4
-rw-r--r--sys/netinet/cc/cc_dctcp.c3
-rw-r--r--sys/netinet/cc/cc_htcp.c4
-rw-r--r--sys/netinet/cc/cc_newreno.c4
-rw-r--r--sys/netinet/cc/cc_vegas.c4
-rw-r--r--sys/netinet/tcp_subr.c13
-rw-r--r--sys/netinet/tcp_usrreq.c1
8 files changed, 18 insertions, 18 deletions
diff --git a/sys/netinet/cc/cc_chd.c b/sys/netinet/cc/cc_chd.c
index 5233bdd9400d..43e7f7c41d8d 100644
--- a/sys/netinet/cc/cc_chd.c
+++ b/sys/netinet/cc/cc_chd.c
@@ -307,8 +307,7 @@ static void
chd_cb_destroy(struct cc_var *ccv)
{
- if (ccv->cc_data != NULL)
- free(ccv->cc_data, M_CHD);
+ free(ccv->cc_data, M_CHD);
}
static int
diff --git a/sys/netinet/cc/cc_cubic.c b/sys/netinet/cc/cc_cubic.c
index e3454b8599c8..34dc56fa14c8 100644
--- a/sys/netinet/cc/cc_cubic.c
+++ b/sys/netinet/cc/cc_cubic.c
@@ -213,9 +213,7 @@ cubic_ack_received(struct cc_var *ccv, uint16_t type)
static void
cubic_cb_destroy(struct cc_var *ccv)
{
-
- if (ccv->cc_data != NULL)
- free(ccv->cc_data, M_CUBIC);
+ free(ccv->cc_data, M_CUBIC);
}
static int
diff --git a/sys/netinet/cc/cc_dctcp.c b/sys/netinet/cc/cc_dctcp.c
index ce75750fef8b..2ed8aed97640 100644
--- a/sys/netinet/cc/cc_dctcp.c
+++ b/sys/netinet/cc/cc_dctcp.c
@@ -184,8 +184,7 @@ dctcp_after_idle(struct cc_var *ccv)
static void
dctcp_cb_destroy(struct cc_var *ccv)
{
- if (ccv->cc_data != NULL)
- free(ccv->cc_data, M_dctcp);
+ free(ccv->cc_data, M_dctcp);
}
static int
diff --git a/sys/netinet/cc/cc_htcp.c b/sys/netinet/cc/cc_htcp.c
index 9545a30c8c63..bb69bb5ab6d7 100644
--- a/sys/netinet/cc/cc_htcp.c
+++ b/sys/netinet/cc/cc_htcp.c
@@ -238,9 +238,7 @@ htcp_ack_received(struct cc_var *ccv, uint16_t type)
static void
htcp_cb_destroy(struct cc_var *ccv)
{
-
- if (ccv->cc_data != NULL)
- free(ccv->cc_data, M_HTCP);
+ free(ccv->cc_data, M_HTCP);
}
static int
diff --git a/sys/netinet/cc/cc_newreno.c b/sys/netinet/cc/cc_newreno.c
index 249ab4a88c94..0ecbbbe72f28 100644
--- a/sys/netinet/cc/cc_newreno.c
+++ b/sys/netinet/cc/cc_newreno.c
@@ -127,9 +127,7 @@ newreno_malloc(struct cc_var *ccv)
static void
newreno_cb_destroy(struct cc_var *ccv)
{
-
- if (ccv->cc_data != NULL)
- free(ccv->cc_data, M_NEWRENO);
+ free(ccv->cc_data, M_NEWRENO);
}
static void
diff --git a/sys/netinet/cc/cc_vegas.c b/sys/netinet/cc/cc_vegas.c
index 5e4b98a23d02..768c1cb9f143 100644
--- a/sys/netinet/cc/cc_vegas.c
+++ b/sys/netinet/cc/cc_vegas.c
@@ -170,9 +170,7 @@ vegas_ack_received(struct cc_var *ccv, uint16_t ack_type)
static void
vegas_cb_destroy(struct cc_var *ccv)
{
-
- if (ccv->cc_data != NULL)
- free(ccv->cc_data, M_VEGAS);
+ free(ccv->cc_data, M_VEGAS);
}
static int
diff --git a/sys/netinet/tcp_subr.c b/sys/netinet/tcp_subr.c
index 51105a8f0777..812f03eecfe4 100644
--- a/sys/netinet/tcp_subr.c
+++ b/sys/netinet/tcp_subr.c
@@ -1730,10 +1730,18 @@ tcp_ccalgounload(struct cc_algo *unload_algo)
*/
if (CC_ALGO(tp) == unload_algo) {
tmpalgo = CC_ALGO(tp);
- /* NewReno does not require any init. */
- CC_ALGO(tp) = &newreno_cc_algo;
if (tmpalgo->cb_destroy != NULL)
tmpalgo->cb_destroy(tp->ccv);
+ CC_DATA(tp) = NULL;
+ /*
+ * NewReno may allocate memory on
+ * demand for certain stateful
+ * configuration as needed, but is
+ * coded to never fail on memory
+ * allocation failure so it is a safe
+ * fallback.
+ */
+ CC_ALGO(tp) = &newreno_cc_algo;
}
}
INP_WUNLOCK(inp);
@@ -1885,6 +1893,7 @@ tcp_discardcb(struct tcpcb *tp)
/* Allow the CC algorithm to clean up after itself. */
if (CC_ALGO(tp)->cb_destroy != NULL)
CC_ALGO(tp)->cb_destroy(tp->ccv);
+ CC_DATA(tp) = NULL;
#ifdef TCP_HHOOK
khelp_destroy_osd(tp->osd);
diff --git a/sys/netinet/tcp_usrreq.c b/sys/netinet/tcp_usrreq.c
index 75312fcd5847..1b0500c3a397 100644
--- a/sys/netinet/tcp_usrreq.c
+++ b/sys/netinet/tcp_usrreq.c
@@ -1729,6 +1729,7 @@ unlock_and_done:
*/
if (CC_ALGO(tp)->cb_destroy != NULL)
CC_ALGO(tp)->cb_destroy(tp->ccv);
+ CC_DATA(tp) = NULL;
CC_ALGO(tp) = algo;
/*
* If something goes pear shaped initialising the new