commit 885ba513ff709eb86a71c7daf7c23aafab4862a8 Author: David Goulet <dgoulet@torproject.org> Date: Tue Dec 19 16:20:36 2017 -0500 sched: Consider extra_space even if negative in KIST With extra_space negative, it means that the "notsent" queue is quite large so we must consider that value with the current computed tcp_space. If we end up to have negative space, we should not add more data to the kernel since the notsent queue is just too filled up. Fixes #24665 Signed-off-by: David Goulet <dgoulet@torproject.org> --- changes/bug24665 | 6 ++++++ src/or/scheduler_kist.c | 22 +++++++++++----------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/changes/bug24665 b/changes/bug24665 new file mode 100644 index 000000000..f950d9dd0 --- /dev/null +++ b/changes/bug24665 @@ -0,0 +1,6 @@ + o Major bugfixes (KIST, scheduler): + - The KIST scheduler did not correctly account for data already enqueued + in each connection's send socket buffer, particularly in cases when the + TCP/IP congestion window was reduced between scheduler calls. This + situation lead to excessive per-connection buffering in the kernel, and + a potential memory DoS. Fixes bug 24665; bugfix on 0.3.2.1-alpha. diff --git a/src/or/scheduler_kist.c b/src/or/scheduler_kist.c index 9acd89b37..a50be345b 100644 --- a/src/or/scheduler_kist.c +++ b/src/or/scheduler_kist.c @@ -266,8 +266,7 @@ update_socket_info_impl, (socket_table_ent_t *ent)) /* These values from the kernel are uint32_t, they will always fit into a * int64_t tcp_space variable but if the congestion window cwnd is smaller - * than the unacked packets, the remaining TCP space is set to 0 so we don't - * write more on this channel. */ + * than the unacked packets, the remaining TCP space is set to 0. */ if (ent->cwnd >= ent->unacked) { tcp_space = (ent->cwnd - ent->unacked) * (int64_t)(ent->mss); } else { @@ -276,20 +275,21 @@ update_socket_info_impl, (socket_table_ent_t *ent)) /* The clamp_double_to_int64 makes sure the first part fits into an int64_t. * In fact, if sock_buf_size_factor is still forced to be >= 0 in config.c, - * then it will be positive for sure. Then we subtract a uint32_t. At worst - * we end up negative, but then we just set extra_space to 0 in the sanity - * check.*/ + * then it will be positive for sure. Then we subtract a uint32_t. Getting a + * negative value is OK, see after how it is being handled. */ extra_space = clamp_double_to_int64( (ent->cwnd * (int64_t)ent->mss) * sock_buf_size_factor) - ent->notsent; - if (extra_space < 0) { - extra_space = 0; + if ((tcp_space + extra_space) < 0) { + /* This means that the "notsent" queue is just too big so we shouldn't put + * more in the kernel for now. */ + ent->limit = 0; + } else { + /* Adding two positive int64_t together will always fit in an uint64_t. + * And we know this will always be positive. */ + ent->limit = (uint64_t)tcp_space + (uint64_t)extra_space; } - - /* Finally we set the limit. Adding two positive int64_t together will always - * fit in an uint64_t. */ - ent->limit = (uint64_t)tcp_space + (uint64_t)extra_space; return; #else /* !(defined(HAVE_KIST_SUPPORT)) */