Merge branch 'ticket25122_029_02' into ticket24902_029_05

changes/ticket25122

@@ -0,0 +1,4 @@
+  o Minor feature (geoip cache):
+    - Make our OOM handler aware of the geoip client history cache so it
+      doesn't fill up the memory which is especially important for IPv6 and
+      our DoS mitigation subsystem. Closes ticket 25122.

src/or/geoip.c

@@ -73,6 +73,38 @@ static smartlist_t *geoip_ipv4_entries = NULL, *geoip_ipv6_entries = NULL;
 static char geoip_digest[DIGEST_LEN];
 static char geoip6_digest[DIGEST_LEN];
 
+/* Total size in bytes of the geoip client history cache. Used by the OOM
+ * handler. */
+static size_t geoip_client_history_cache_size;
+
+/* Increment the geoip client history cache size counter with the given bytes.
+ * This prevents an overflow and set it to its maximum in that case. */
+static inline void
+geoip_increment_client_history_cache_size(size_t bytes)
+{
+  /* This is shockingly high, lets log it so it can be reported. */
+  IF_BUG_ONCE(geoip_client_history_cache_size > (SIZE_MAX - bytes)) {
+    geoip_client_history_cache_size = SIZE_MAX;
+    return;
+  }
+  geoip_client_history_cache_size += bytes;
+}
+
+/* Decrement the geoip client history cache size counter with the given bytes.
+ * This prevents an underflow and set it to 0 in that case. */
+static inline void
+geoip_decrement_client_history_cache_size(size_t bytes)
+{
+  /* Going below 0 means that we either allocated an entry without
+   * incrementing the counter or we have different sizes when allocating and
+   * freeing. It shouldn't happened so log it. */
+  IF_BUG_ONCE(geoip_client_history_cache_size < bytes) {
+    geoip_client_history_cache_size = 0;
+    return;
+  }
+  geoip_client_history_cache_size -= bytes;
+}
+
 /** Return the index of the <b>country</b>'s entry in the GeoIP
  * country list if it is a valid 2-letter country code, otherwise
  * return -1. */
@@ -509,6 +541,15 @@ HT_PROTOTYPE(clientmap, clientmap_entry_t, node, clientmap_entry_hash,
 HT_GENERATE2(clientmap, clientmap_entry_t, node, clientmap_entry_hash,
              clientmap_entries_eq, 0.6, tor_reallocarray_, tor_free_)
 
+/** Return the size of a client map entry. */
+static inline size_t
+clientmap_entry_size(const clientmap_entry_t *ent)
+{
+  tor_assert(ent);
+  return (sizeof(clientmap_entry_t) +
+          (ent->transport_name ? strlen(ent->transport_name) : 0));
+}
+
 /** Free all storage held by <b>ent</b>. */
 static void
 clientmap_entry_free(clientmap_entry_t *ent)
@@ -519,11 +560,37 @@ clientmap_entry_free(clientmap_entry_t *ent)
   /* This entry is about to be freed so pass it to the DoS subsystem to see if
    * any actions can be taken about it. */
   dos_geoip_entry_about_to_free(ent);
+  geoip_decrement_client_history_cache_size(clientmap_entry_size(ent));
 
   tor_free(ent->transport_name);
   tor_free(ent);
 }
 
+/* Return a newly allocated clientmap entry with the given action and address
+ * that are mandatory. The transport_name can be optional. This can't fail. */
+static clientmap_entry_t *
+clientmap_entry_new(geoip_client_action_t action, const tor_addr_t *addr,
+                    const char *transport_name)
+{
+  clientmap_entry_t *entry;
+
+  tor_assert(action == GEOIP_CLIENT_CONNECT ||
+             action == GEOIP_CLIENT_NETWORKSTATUS);
+  tor_assert(addr);
+
+  entry = tor_malloc_zero(sizeof(clientmap_entry_t));
+  entry->action = action;
+  tor_addr_copy(&entry->addr, addr);
+  if (transport_name) {
+    entry->transport_name = tor_strdup(transport_name);
+  }
+
+  /* Allocated and initialized, note down its size for the OOM handler. */
+  geoip_increment_client_history_cache_size(clientmap_entry_size(entry));
+
+  return entry;
+}
+
 /** Clear history of connecting clients used by entry and bridge stats. */
 static void
 client_history_clear(void)
@@ -575,11 +642,7 @@ geoip_note_client_seen(geoip_client_action_t action,
 
   ent = geoip_lookup_client(addr, transport_name, action);
   if (! ent) {
-    ent = tor_malloc_zero(sizeof(clientmap_entry_t));
+    ent = clientmap_entry_new(action, addr, transport_name);
-    tor_addr_copy(&ent->addr, addr);
-    if (transport_name)
-      ent->transport_name = tor_strdup(transport_name);
-    ent->action = (int)action;
     HT_INSERT(clientmap, &client_history, ent);
   }
   if (now / 60 <= (int)MAX_LAST_SEEN_IN_MINUTES && now >= 0)
@@ -640,6 +703,81 @@ geoip_lookup_client(const tor_addr_t *addr, const char *transport_name,
   return HT_FIND(clientmap, &client_history, &lookup);
 }
 
+/* Cleanup client entries older than the cutoff. Used for the OOM. Return the
+ * number of bytes freed. If 0 is returned, nothing was freed. */
+static size_t
+oom_clean_client_entries(time_t cutoff)
+{
+  size_t bytes = 0;
+  clientmap_entry_t **ent, **ent_next;
+
+  for (ent = HT_START(clientmap, &client_history); ent; ent = ent_next) {
+    clientmap_entry_t *entry = *ent;
+    if (entry->last_seen_in_minutes < (cutoff / 60)) {
+      ent_next = HT_NEXT_RMV(clientmap, &client_history, ent);
+      bytes += clientmap_entry_size(entry);
+      clientmap_entry_free(entry);
+    } else {
+      ent_next = HT_NEXT(clientmap, &client_history, ent);
+    }
+  }
+  return bytes;
+}
+
+/* Below this minimum lifetime, the OOM won't cleanup any entries. */
+#define GEOIP_CLIENT_CACHE_OOM_MIN_CUTOFF (4 * 60 * 60)
+/* The OOM moves the cutoff by that much every run. */
+#define GEOIP_CLIENT_CACHE_OOM_STEP (15 * 50)
+
+/* Cleanup the geoip client history cache called from the OOM handler. Return
+ * the amount of bytes removed. This can return a value below or above
+ * min_remove_bytes but will stop as oon as the min_remove_bytes has been
+ * reached. */
+size_t
+geoip_client_cache_handle_oom(time_t now, size_t min_remove_bytes)
+{
+  time_t k;
+  size_t bytes_removed = 0;
+
+  /* Our OOM handler called with 0 bytes to remove is a code flow error. */
+  tor_assert(min_remove_bytes != 0);
+
+  /* Set k to the initial cutoff of an entry. We then going to move it by step
+   * to try to remove as much as we can. */
+  k = WRITE_STATS_INTERVAL;
+
+  do {
+    time_t cutoff;
+
+    /* If k has reached the minimum lifetime, we have to stop else we might
+     * remove every single entries which would be pretty bad for the DoS
+     * mitigation subsystem if by just filling the geoip cache, it was enough
+     * to trigger the OOM and clean every single entries. */
+    if (k <= GEOIP_CLIENT_CACHE_OOM_MIN_CUTOFF) {
+      break;
+    }
+
+    cutoff = now - k;
+    bytes_removed += oom_clean_client_entries(cutoff);
+    k -= GEOIP_CLIENT_CACHE_OOM_STEP;
+  } while (bytes_removed < min_remove_bytes);
+
+  return bytes_removed;
+}
+
+/* Return the total size in bytes of the client history cache. */
+size_t
+geoip_client_cache_total_allocation(void)
+{
+  size_t bytes = 0;
+  clientmap_entry_t **ent;
+
+  HT_FOREACH(ent, clientmap, &client_history) {
+    bytes += clientmap_entry_size(*ent);
+  }
+  return bytes;
+}
+
 /** How many responses are we giving to clients requesting v3 network
  * statuses? */
 static uint32_t ns_v3_responses[GEOIP_NS_RESPONSE_NUM];

src/or/geoip.h

@@ -60,6 +60,8 @@ void geoip_remove_old_clients(time_t cutoff);
 clientmap_entry_t *geoip_lookup_client(const tor_addr_t *addr,
                                        const char *transport_name,
                                        geoip_client_action_t action);
+size_t geoip_client_cache_total_allocation(void);
+size_t geoip_client_cache_handle_oom(time_t now, size_t min_remove_bytes);
 
 void geoip_note_ns_response(geoip_ns_response_t response);
 char *geoip_get_transport_history(void);

src/or/relay.c

@@ -2469,24 +2469,34 @@ static time_t last_time_under_memory_pressure = 0;
 STATIC int
 cell_queues_check_size(void)
 {
+  time_t now = time(NULL);
   size_t alloc = cell_queues_get_total_allocation();
   alloc += buf_get_total_allocation();
   alloc += tor_zlib_get_total_allocation();
   const size_t rend_cache_total = rend_cache_get_total_allocation();
   alloc += rend_cache_total;
+  const size_t geoip_client_cache_total =
+    geoip_client_cache_total_allocation();
+  alloc += geoip_client_cache_total;
   if (alloc >= get_options()->MaxMemInQueues_low_threshold) {
     last_time_under_memory_pressure = approx_time();
     if (alloc >= get_options()->MaxMemInQueues) {
       /* If we're spending over 20% of the memory limit on hidden service
-       * descriptors, free them until we're down to 10%.
+       * descriptors, free them until we're down to 10%. Do the same for geoip
-       */
+       * client cache. */
       if (rend_cache_total > get_options()->MaxMemInQueues / 5) {
         const size_t bytes_to_remove =
           rend_cache_total - (size_t)(get_options()->MaxMemInQueues / 10);
-        rend_cache_clean_v2_descs_as_dir(time(NULL), bytes_to_remove);
+        rend_cache_clean_v2_descs_as_dir(now, bytes_to_remove);
         alloc -= rend_cache_total;
         alloc += rend_cache_get_total_allocation();
       }
+      if (geoip_client_cache_total > get_options()->MaxMemInQueues / 5) {
+        const size_t bytes_to_remove =
+          geoip_client_cache_total -
+          (size_t)(get_options()->MaxMemInQueues / 10);
+        alloc -= geoip_client_cache_handle_oom(now, bytes_to_remove);
+      }
       circuits_handle_oom(alloc);
       return 1;
     }

src/test/test.c

@@ -912,6 +912,24 @@ test_geoip(void *arg)
   tt_str_op(entry_stats_2,OP_EQ, s);
   tor_free(s);
 
+  /* Test the OOM handler. Add a client, run the OOM. */
+  geoip_entry_stats_init(now);
+  SET_TEST_ADDRESS(100);
+  geoip_note_client_seen(GEOIP_CLIENT_CONNECT, &addr, NULL,
+                         now - (12 * 60 * 60));
+  /* We've seen this 12 hours ago. Run the OOM, it should clean the entry
+   * because it is above the minimum cutoff of 4 hours. */
+  size_t bytes_removed = geoip_client_cache_handle_oom(now, 1000);
+  tt_size_op(bytes_removed, OP_GT, 0);
+
+  /* Do it again but this time with an entry with a lower cutoff. */
+  geoip_entry_stats_init(now);
+  SET_TEST_ADDRESS(100);
+  geoip_note_client_seen(GEOIP_CLIENT_CONNECT, &addr, NULL,
+                         now - (3 * 60 * 60));
+  bytes_removed = geoip_client_cache_handle_oom(now, 1000);
+  tt_size_op(bytes_removed, OP_EQ, 0);
+
   /* Stop collecting entry statistics. */
   geoip_entry_stats_term();
   get_options_mutable()->EntryStatistics = 0;