[tor-commits] [tor/master] sched: Consider extra_space even if negative in KIST

[nickm at torproject.org]

commit 885ba513ff709eb86a71c7daf7c23aafab4862a8
Author: David Goulet <dgoulet at 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 at 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)) */