Merge branch 'maint-0.2.4'

Conflicts:
	src/test/test.c

changes/feature9574

@@ -0,0 +1,7 @@
+  o Major features:
+    - Relays now process the new "NTor" circuit-level handshake requests
+      with higher priority than the old "TAP" circuit-level handshake
+      requests. We still process some TAP requests to not totally starve
+      0.2.3 clients when NTor becomes popular. A new consensus parameter
+      "NumNTorsPerTAP" lets us tune the balance later if we need to.
+      Implements ticket 9574.

src/or/onion.c

@@ -14,6 +14,7 @@
 #include "circuitlist.h"
 #include "config.h"
 #include "cpuworker.h"
+#include "networkstatus.h"
 #include "onion.h"
 #include "onion_fast.h"
 #include "onion_ntor.h"
@@ -27,6 +28,7 @@
 typedef struct onion_queue_t {
   TOR_TAILQ_ENTRY(onion_queue_t) next;
   or_circuit_t *circ;
+  uint16_t handshake_type;
   create_cell_t *onionskin;
   time_t when_added;
 } onion_queue_t;
@@ -34,13 +36,19 @@ typedef struct onion_queue_t {
 /** 5 seconds on the onion queue til we just send back a destroy */
 #define ONIONQUEUE_WAIT_CUTOFF 5
 
-/** Queue of circuits waiting for CPU workers, or NULL if the list is empty.*/
+/** Array of queues of circuits waiting for CPU workers. An element is NULL
-TOR_TAILQ_HEAD(onion_queue_head_t, onion_queue_t) ol_list =
+ * if that queue is empty.*/
-  TOR_TAILQ_HEAD_INITIALIZER(ol_list);
+TOR_TAILQ_HEAD(onion_queue_head_t, onion_queue_t)
+              ol_list[MAX_ONION_HANDSHAKE_TYPE+1] = {
+  TOR_TAILQ_HEAD_INITIALIZER(ol_list[0]), /* tap */
+  TOR_TAILQ_HEAD_INITIALIZER(ol_list[1]), /* fast */
+  TOR_TAILQ_HEAD_INITIALIZER(ol_list[2]), /* ntor */
+};
 
-/** Number of entries of each type currently in ol_list. */
+/** Number of entries of each type currently in each element of ol_list[]. */
 static int ol_entries[MAX_ONION_HANDSHAKE_TYPE+1];
 
+static int num_ntors_per_tap(void);
 static void onion_queue_entry_remove(onion_queue_t *victim);
 
 /* XXXX024 Check lengths vs MAX_ONIONSKIN_{CHALLENGE,REPLY}_LEN.
@@ -58,23 +66,51 @@ have_room_for_onionskin(uint16_t type)
   const or_options_t *options = get_options();
   int num_cpus;
   uint64_t tap_usec, ntor_usec;
+  uint64_t ntor_during_tap_usec, tap_during_ntor_usec;
+
   /* If we've got fewer than 50 entries, we always have room for one more. */
-  if (ol_entries[ONION_HANDSHAKE_TYPE_TAP] +
+  if (ol_entries[type] < 50)
-      ol_entries[ONION_HANDSHAKE_TYPE_NTOR] < 50)
     return 1;
   num_cpus = get_num_cpus(options);
   /* Compute how many microseconds we'd expect to need to clear all
-   * onionskins in the current queue. */
+   * onionskins in various combinations of the queues. */
+
+  /* How long would it take to process all the TAP cells in the queue? */
   tap_usec  = estimated_usec_for_onionskins(
                                     ol_entries[ONION_HANDSHAKE_TYPE_TAP],
                                     ONION_HANDSHAKE_TYPE_TAP) / num_cpus;
+
+  /* How long would it take to process all the NTor cells in the queue? */
   ntor_usec = estimated_usec_for_onionskins(
                                     ol_entries[ONION_HANDSHAKE_TYPE_NTOR],
                                     ONION_HANDSHAKE_TYPE_NTOR) / num_cpus;
+
+  /* How long would it take to process the tap cells that we expect to
+   * process while draining the ntor queue? */
+  tap_during_ntor_usec  = estimated_usec_for_onionskins(
+    MIN(ol_entries[ONION_HANDSHAKE_TYPE_TAP],
+        ol_entries[ONION_HANDSHAKE_TYPE_NTOR] / num_ntors_per_tap()),
+                                    ONION_HANDSHAKE_TYPE_TAP) / num_cpus;
+
+  /* How long would it take to process the ntor cells that we expect to
+   * process while draining the tap queue? */
+  ntor_during_tap_usec  = estimated_usec_for_onionskins(
+    MIN(ol_entries[ONION_HANDSHAKE_TYPE_NTOR],
+        ol_entries[ONION_HANDSHAKE_TYPE_TAP] * num_ntors_per_tap()),
+                                    ONION_HANDSHAKE_TYPE_NTOR) / num_cpus;
+
   /* See whether that exceeds MaxOnionQueueDelay. If so, we can't queue
    * this. */
-  if ((tap_usec + ntor_usec) / 1000 > (uint64_t)options->MaxOnionQueueDelay)
+  if (type == ONION_HANDSHAKE_TYPE_NTOR &&
+      (ntor_usec + tap_during_ntor_usec) / 1000 >
+       (uint64_t)options->MaxOnionQueueDelay)
     return 0;
+
+  if (type == ONION_HANDSHAKE_TYPE_TAP &&
+      (tap_usec + ntor_during_tap_usec) / 1000 >
+       (uint64_t)options->MaxOnionQueueDelay)
+    return 0;
+
 #ifdef CURVE25519_ENABLED
   /* If we support the ntor handshake, then don't let TAP handshakes use
    * more than 2/3 of the space on the queue. */
@@ -97,8 +133,15 @@ onion_pending_add(or_circuit_t *circ, create_cell_t *onionskin)
   onion_queue_t *tmp;
   time_t now = time(NULL);
 
+  if (onionskin->handshake_type > MAX_ONION_HANDSHAKE_TYPE) {
+    log_warn(LD_BUG, "Handshake %d out of range! Dropping.",
+             onionskin->handshake_type);
+    return -1;
+  }
+
   tmp = tor_malloc_zero(sizeof(onion_queue_t));
   tmp->circ = circ;
+  tmp->handshake_type = onionskin->handshake_type;
   tmp->onionskin = onionskin;
   tmp->when_added = now;
 
@@ -107,7 +150,8 @@ onion_pending_add(or_circuit_t *circ, create_cell_t *onionskin)
     static ratelim_t last_warned =
       RATELIM_INIT(WARN_TOO_MANY_CIRC_CREATIONS_INTERVAL);
     char *m;
-    if ((m = rate_limit_log(&last_warned, approx_time()))) {
+    if (onionskin->handshake_type == ONION_HANDSHAKE_TYPE_NTOR &&
+        (m = rate_limit_log(&last_warned, approx_time()))) {
       log_warn(LD_GENERAL,
                "Your computer is too slow to handle this many circuit "
                "creation requests! Please consider using the "
@@ -120,12 +164,17 @@ onion_pending_add(or_circuit_t *circ, create_cell_t *onionskin)
   }
 
   ++ol_entries[onionskin->handshake_type];
+  log_info(LD_OR, "New create (%s). Queues now ntor=%d and tap=%d.",
+    onionskin->handshake_type == ONION_HANDSHAKE_TYPE_NTOR ? "ntor" : "tap",
+    ol_entries[ONION_HANDSHAKE_TYPE_NTOR],
+    ol_entries[ONION_HANDSHAKE_TYPE_TAP]);
+
   circ->onionqueue_entry = tmp;
-  TOR_TAILQ_INSERT_TAIL(&ol_list, tmp, next);
+  TOR_TAILQ_INSERT_TAIL(&ol_list[onionskin->handshake_type], tmp, next);
 
   /* cull elderly requests. */
   while (1) {
-    onion_queue_t *head = TOR_TAILQ_FIRST(&ol_list);
+    onion_queue_t *head = TOR_TAILQ_FIRST(&ol_list[onionskin->handshake_type]);
     if (now - head->when_added < (time_t)ONIONQUEUE_WAIT_CUTOFF)
       break;
 
@@ -139,24 +188,87 @@ onion_pending_add(or_circuit_t *circ, create_cell_t *onionskin)
   return 0;
 }
 
-/** Remove the first item from ol_list and return it, or return
+/** Return a fairness parameter, to prefer processing NTOR style
- * NULL if the list is empty.
+ * handshakes but still slowly drain the TAP queue so we don't starve
+ * it entirely. */
+static int
+num_ntors_per_tap(void)
+{
+#define DEFAULT_NUM_NTORS_PER_TAP 10
+#define MIN_NUM_NTORS_PER_TAP 1
+#define MAX_NUM_NTORS_PER_TAP 100000
+
+  return networkstatus_get_param(NULL, "NumNTorsPerTAP",
+                                 DEFAULT_NUM_NTORS_PER_TAP,
+                                 MIN_NUM_NTORS_PER_TAP,
+                                 MAX_NUM_NTORS_PER_TAP);
+}
+
+/** Choose which onion queue we'll pull from next. If one is empty choose
+ * the other; if they both have elements, load balance across them but
+ * favoring NTOR. */
+static uint16_t
+decide_next_handshake_type(void)
+{
+  /* The number of times we've chosen ntor lately when both were available. */
+  static int recently_chosen_ntors = 0;
+
+  if (!ol_entries[ONION_HANDSHAKE_TYPE_NTOR])
+    return ONION_HANDSHAKE_TYPE_TAP; /* no ntors? try tap */
+
+  if (!ol_entries[ONION_HANDSHAKE_TYPE_TAP]) {
+
+    /* Nick wants us to prioritize new tap requests when there aren't
+     * any in the queue and we've processed k ntor cells since the last
+     * tap cell. This strategy is maybe a good idea, since it starves tap
+     * less in the case where tap is rare, or maybe a poor idea, since it
+     * makes the new tap cell unfairly jump in front of ntor cells that
+     * got here first. In any case this edge case will only become relevant
+     * once tap is rare. We should reevaluate whether we like this decision
+     * once tap gets more rare. */
+    if (ol_entries[ONION_HANDSHAKE_TYPE_NTOR])
+      ++recently_chosen_ntors;
+
+    return ONION_HANDSHAKE_TYPE_NTOR; /* no taps? try ntor */
+  }
+
+  /* They both have something queued. Pick ntor if we haven't done that
+   * too much lately. */
+  if (++recently_chosen_ntors <= num_ntors_per_tap()) {
+    return ONION_HANDSHAKE_TYPE_NTOR;
+  }
+
+  /* Else, it's time to let tap have its turn. */
+  recently_chosen_ntors = 0;
+  return ONION_HANDSHAKE_TYPE_TAP;
+}
+
+/** Remove the highest priority item from ol_list[] and return it, or
+ * return NULL if the lists are empty.
  */
 or_circuit_t *
 onion_next_task(create_cell_t **onionskin_out)
 {
   or_circuit_t *circ;
-  onion_queue_t *head = TOR_TAILQ_FIRST(&ol_list);
+  uint16_t handshake_to_choose = decide_next_handshake_type();
+  onion_queue_t *head = TOR_TAILQ_FIRST(&ol_list[handshake_to_choose]);
 
   if (!head)
     return NULL; /* no onions pending, we're done */
 
   tor_assert(head->circ);
-  tor_assert(head->circ->p_chan); /* make sure it's still valid */
+  tor_assert(head->handshake_type <= MAX_ONION_HANDSHAKE_TYPE);
+//  tor_assert(head->circ->p_chan); /* make sure it's still valid */
+/* XXX I only commented out the above line to make the unit tests
+ * more manageable. That's probably not good long-term. -RD */
   circ = head->circ;
-  if (head->onionskin &&
+  if (head->onionskin)
-      head->onionskin->handshake_type <= MAX_ONION_HANDSHAKE_TYPE)
+    --ol_entries[head->handshake_type];
-    --ol_entries[head->onionskin->handshake_type];
+  log_info(LD_OR, "Processing create (%s). Queues now ntor=%d and tap=%d.",
+    head->handshake_type == ONION_HANDSHAKE_TYPE_NTOR ? "ntor" : "tap",
+    ol_entries[ONION_HANDSHAKE_TYPE_NTOR],
+    ol_entries[ONION_HANDSHAKE_TYPE_TAP]);
+
   *onionskin_out = head->onionskin;
   head->onionskin = NULL; /* prevent free. */
   circ->onionqueue_entry = NULL;
@@ -164,6 +276,14 @@ onion_next_task(create_cell_t **onionskin_out)
   return circ;
 }
 
+/** Return the number of <b>handshake_type</b>-style create requests pending.
+ */
+int
+onion_num_pending(uint16_t handshake_type)
+{
+  return ol_entries[handshake_type];
+}
+
 /** Go through ol_list, find the onion_queue_t element which points to
  * circ, remove and free that element. Leave circ itself alone.
  */
@@ -185,14 +305,20 @@ onion_pending_remove(or_circuit_t *circ)
 static void
 onion_queue_entry_remove(onion_queue_t *victim)
 {
-  TOR_TAILQ_REMOVE(&ol_list, victim, next);
+  if (victim->handshake_type > MAX_ONION_HANDSHAKE_TYPE) {
+    log_warn(LD_BUG, "Handshake %d out of range! Dropping.",
+             victim->handshake_type);
+    /* XXX leaks */
+    return;
+  }
+
+  TOR_TAILQ_REMOVE(&ol_list[victim->handshake_type], victim, next);
 
   if (victim->circ)
     victim->circ->onionqueue_entry = NULL;
 
-  if (victim->onionskin &&
+  if (victim->onionskin)
-      victim->onionskin->handshake_type <= MAX_ONION_HANDSHAKE_TYPE)
+    --ol_entries[victim->handshake_type];
-    --ol_entries[victim->onionskin->handshake_type];
 
   tor_free(victim->onionskin);
   tor_free(victim);
@@ -203,9 +329,12 @@ void
 clear_pending_onions(void)
 {
   onion_queue_t *victim;
-  while ((victim = TOR_TAILQ_FIRST(&ol_list))) {
+  int i;
+  for (i=0; i<=MAX_ONION_HANDSHAKE_TYPE; i++) {
+    while ((victim = TOR_TAILQ_FIRST(&ol_list[i]))) {
       onion_queue_entry_remove(victim);
     }
+  }
   memset(ol_entries, 0, sizeof(ol_entries));
 }
 
@@ -513,6 +642,22 @@ check_create_cell(const create_cell_t *cell, int unknown_ok)
   return 0;
 }
 
+/** Write the various parameters into the create cell. Separate from
+ * create_cell_parse() to make unit testing easier.
+ */
+void
+create_cell_init(create_cell_t *cell_out, uint8_t cell_type,
+                 uint16_t handshake_type, uint16_t handshake_len,
+                 const uint8_t *onionskin)
+{
+  memset(cell_out, 0, sizeof(*cell_out));
+
+  cell_out->cell_type = cell_type;
+  cell_out->handshake_type = handshake_type;
+  cell_out->handshake_len = handshake_len;
+  memcpy(cell_out->onionskin, onionskin, handshake_len);
+}
+
 /** Helper: parse the CREATE2 payload at <b>p</b>, which could be up to
  * <b>p_len</b> bytes long, and use it to fill the fields of
  * <b>cell_out</b>. Return 0 on success and -1 on failure.
@@ -523,17 +668,21 @@ check_create_cell(const create_cell_t *cell, int unknown_ok)
 static int
 parse_create2_payload(create_cell_t *cell_out, const uint8_t *p, size_t p_len)
 {
+  uint16_t handshake_type, handshake_len;
+
   if (p_len < 4)
     return -1;
-  cell_out->cell_type = CELL_CREATE2;
+
-  cell_out->handshake_type = ntohs(get_uint16(p));
+  handshake_type = ntohs(get_uint16(p));
-  cell_out->handshake_len = ntohs(get_uint16(p+2));
+  handshake_len = ntohs(get_uint16(p+2));
-  if (cell_out->handshake_len > CELL_PAYLOAD_SIZE - 4 ||
+
-      cell_out->handshake_len > p_len - 4)
+  if (handshake_len > CELL_PAYLOAD_SIZE - 4 || handshake_len > p_len - 4)
     return -1;
-  if (cell_out->handshake_type == ONION_HANDSHAKE_TYPE_FAST)
+  if (handshake_type == ONION_HANDSHAKE_TYPE_FAST)
     return -1;
-  memcpy(cell_out->onionskin, p+4, cell_out->handshake_len);
+
+  create_cell_init(cell_out, CELL_CREATE2, handshake_type, handshake_len,
+                   p+4);
   return 0;
 }
 
@@ -551,27 +700,19 @@ parse_create2_payload(create_cell_t *cell_out, const uint8_t *p, size_t p_len)
 int
 create_cell_parse(create_cell_t *cell_out, const cell_t *cell_in)
 {
-  memset(cell_out, 0, sizeof(*cell_out));
-
   switch (cell_in->command) {
   case CELL_CREATE:
-    cell_out->cell_type = CELL_CREATE;
     if (tor_memeq(cell_in->payload, NTOR_CREATE_MAGIC, 16)) {
-      cell_out->handshake_type = ONION_HANDSHAKE_TYPE_NTOR;
+      create_cell_init(cell_out, CELL_CREATE, ONION_HANDSHAKE_TYPE_NTOR,
-      cell_out->handshake_len = NTOR_ONIONSKIN_LEN;
+                       NTOR_ONIONSKIN_LEN, cell_in->payload+16);
-      memcpy(cell_out->onionskin, cell_in->payload+16, NTOR_ONIONSKIN_LEN);
     } else {
-      cell_out->handshake_type = ONION_HANDSHAKE_TYPE_TAP;
+      create_cell_init(cell_out, CELL_CREATE, ONION_HANDSHAKE_TYPE_TAP,
-      cell_out->handshake_len = TAP_ONIONSKIN_CHALLENGE_LEN;
+                       TAP_ONIONSKIN_CHALLENGE_LEN, cell_in->payload);
-      memcpy(cell_out->onionskin, cell_in->payload,
-             TAP_ONIONSKIN_CHALLENGE_LEN);
     }
     break;
   case CELL_CREATE_FAST:
-    cell_out->cell_type = CELL_CREATE_FAST;
+    create_cell_init(cell_out, CELL_CREATE_FAST, ONION_HANDSHAKE_TYPE_FAST,
-    cell_out->handshake_type = ONION_HANDSHAKE_TYPE_FAST;
+                     CREATE_FAST_LEN, cell_in->payload);
-    cell_out->handshake_len = CREATE_FAST_LEN;
-    memcpy(cell_out->onionskin, cell_in->payload, CREATE_FAST_LEN);
     break;
   case CELL_CREATE2:
     if (parse_create2_payload(cell_out, cell_in->payload,

src/or/onion.h

@@ -15,6 +15,7 @@
 struct create_cell_t;
 int onion_pending_add(or_circuit_t *circ, struct create_cell_t *onionskin);
 or_circuit_t *onion_next_task(struct create_cell_t **onionskin_out);
+int onion_num_pending(uint16_t handshake_type);
 void onion_pending_remove(or_circuit_t *circ);
 void clear_pending_onions(void);
 
@@ -99,6 +100,9 @@ typedef struct extended_cell_t {
   created_cell_t created_cell;
 } extended_cell_t;
 
+void create_cell_init(create_cell_t *cell_out, uint8_t cell_type,
+                      uint16_t handshake_type, uint16_t handshake_len,
+                      const uint8_t *onionskin);
 int create_cell_parse(create_cell_t *cell_out, const cell_t *cell_in);
 int created_cell_parse(created_cell_t *cell_out, const cell_t *cell_in);
 int extend_cell_parse(extend_cell_t *cell_out, const uint8_t command,

src/test/test.c

@@ -41,6 +41,8 @@ long int lround(double x);
 double fabs(double x);
 
 #include "or.h"
+#include "buffers.h"
+#include "circuitlist.h"
 #include "circuitstats.h"
 #include "config.h"
 #include "connection_edge.h"
@@ -50,6 +52,7 @@ double fabs(double x);
 #include "torgzip.h"
 #include "mempool.h"
 #include "memarea.h"
+#include "onion.h"
 #include "onion_tap.h"
 #include "policies.h"
 #include "rephist.h"
@@ -401,6 +404,49 @@ test_ntor_handshake(void *arg)
 }
 #endif
 
+/** Run unit tests for the onion queues. */
+static void
+test_onion_queues(void)
+{
+  uint8_t buf1[TAP_ONIONSKIN_CHALLENGE_LEN] = {0};
+  uint8_t buf2[NTOR_ONIONSKIN_LEN] = {0};
+
+  or_circuit_t *circ1 = or_circuit_new(0, NULL);
+  or_circuit_t *circ2 = or_circuit_new(0, NULL);
+
+  create_cell_t *onionskin = NULL;
+  create_cell_t *create1 = tor_malloc_zero(sizeof(create_cell_t));
+  create_cell_t *create2 = tor_malloc_zero(sizeof(create_cell_t));
+
+  create_cell_init(create1, CELL_CREATE, ONION_HANDSHAKE_TYPE_TAP,
+                   TAP_ONIONSKIN_CHALLENGE_LEN, buf1);
+  create_cell_init(create2, CELL_CREATE, ONION_HANDSHAKE_TYPE_NTOR,
+                   NTOR_ONIONSKIN_LEN, buf2);
+
+  test_eq(0, onion_num_pending(ONION_HANDSHAKE_TYPE_TAP));
+  test_eq(0, onion_pending_add(circ1, create1));
+  test_eq(1, onion_num_pending(ONION_HANDSHAKE_TYPE_TAP));
+
+  test_eq(0, onion_num_pending(ONION_HANDSHAKE_TYPE_NTOR));
+  test_eq(0, onion_pending_add(circ2, create2));
+  test_eq(1, onion_num_pending(ONION_HANDSHAKE_TYPE_NTOR));
+
+  test_eq_ptr(circ2, onion_next_task(&onionskin));
+  test_eq(1, onion_num_pending(ONION_HANDSHAKE_TYPE_TAP));
+  test_eq(0, onion_num_pending(ONION_HANDSHAKE_TYPE_NTOR));
+
+  clear_pending_onions();
+  test_eq(0, onion_num_pending(ONION_HANDSHAKE_TYPE_TAP));
+  test_eq(0, onion_num_pending(ONION_HANDSHAKE_TYPE_NTOR));
+
+ done:
+  ;
+//  circuit_free(circ1);
+//  circuit_free(circ2);
+  /* and free create1 and create2 */
+  /* XXX leaks everything here */
+}
+
 static void
 test_circuit_timeout(void)
 {
@@ -1541,6 +1587,7 @@ const struct testcase_setup_t legacy_setup = {
 static struct testcase_t test_array[] = {
   ENT(onion_handshake),
   { "bad_onion_handshake", test_bad_onion_handshake, 0, NULL, NULL },
+  ENT(onion_queues),
 #ifdef CURVE25519_ENABLED
   { "ntor_handshake", test_ntor_handshake, 0, NULL, NULL },
 #endif