commit 99703eaca0f575e4739523ca815cf55329d16024 Author: David Goulet dgoulet@torproject.org Date: Tue Jan 26 11:57:58 2021 -0500

dos: Move concurrent count into conn_stats object

No behavior change except for logging. This is so the connection related statistics are in the right object.

Related to #40253

Signed-off-by: David Goulet dgoulet@torproject.org --- src/core/or/dos.c | 61 +++++++++++++++++++++++++++++++++++------------------ src/core/or/dos.h | 8 +++---- src/test/test_dos.c | 4 ++-- 3 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/src/core/or/dos.c b/src/core/or/dos.c index 8b3dccc871..0169a631c2 100644 --- a/src/core/or/dos.c +++ b/src/core/or/dos.c @@ -399,7 +399,7 @@ cc_has_exhausted_circuits(const dos_client_stats_t *stats) { tor_assert(stats); return stats->cc_stats.circuit_bucket == 0 && - stats->concurrent_count >= dos_cc_min_concurrent_conn; + stats->conn_stats.concurrent_count >= dos_cc_min_concurrent_conn; }

/* Mark client address by setting a timestamp in the stats object which tells @@ -491,11 +491,17 @@ conn_consensus_has_changed(const networkstatus_t *ns) /** Called when a new client connection has arrived. The following will update * the client connection statistics. * + * The addr is used for logging purposes only. + * * If the connect counter reaches its limit, it is marked. */ static void -conn_update_on_connect(conn_client_stats_t *stats) +conn_update_on_connect(conn_client_stats_t *stats, const tor_addr_t *addr) { tor_assert(stats); + tor_assert(addr); + + /* Update concurrent count for this new connect. */ + stats->concurrent_count++;

/* Refill connect connection count. */ token_bucket_ctr_refill(&stats->connect_count, (uint32_t) approx_time()); @@ -512,6 +518,31 @@ conn_update_on_connect(conn_client_stats_t *stats) stats->marked_until_ts == 0) { conn_mark_client(stats); } + + log_debug(LD_DOS, "Client address %s has now %u concurrent connections. " + "Remaining %lu/sec connections are allowed.", + fmt_addr(addr), stats->concurrent_count, + token_bucket_ctr_get(&stats->connect_count)); +} + +/** Called when a client connection is closed. The following will update + * the client connection statistics. + * + * The addr is used for logging purposes only. */ +static void +conn_update_on_close(conn_client_stats_t *stats, const tor_addr_t *addr) +{ + /* Extra super duper safety. Going below 0 means an underflow which could + * lead to most likely a false positive. In theory, this should never happen + * but lets be extra safe. */ + if (BUG(stats->concurrent_count == 0)) { + return; + } + + stats->concurrent_count--; + log_debug(LD_DOS, "Client address %s has lost a connection. Concurrent " + "connections are now at %u", + fmt_addr(addr), stats->concurrent_count); }

/* General private API */ @@ -651,7 +682,8 @@ dos_conn_addr_get_defense_type(const tor_addr_t *addr)

/* Need to be above the maximum concurrent connection count to trigger a * defense. */ - if (entry->dos_stats.concurrent_count > dos_conn_max_concurrent_count) { + if (entry->dos_stats.conn_stats.concurrent_count > + dos_conn_max_concurrent_count) { conn_num_addr_rejected++; return dos_conn_defense_type; } @@ -676,7 +708,7 @@ dos_geoip_entry_about_to_free(const clientmap_entry_t *geoip_ent)

/* The count is down to 0 meaning no connections right now, we can safely * clear the geoip entry from the cache. */ - if (geoip_ent->dos_stats.concurrent_count == 0) { + if (geoip_ent->dos_stats.conn_stats.concurrent_count == 0) { goto end; }

@@ -831,13 +863,10 @@ dos_new_client_conn(or_connection_t *or_conn, const char *transport_name) }

/* Update stats from this new connect. */ - conn_update_on_connect(&entry->dos_stats.conn_stats); + conn_update_on_connect(&entry->dos_stats.conn_stats, + &TO_CONN(or_conn)->addr);

- entry->dos_stats.concurrent_count++; or_conn->tracked_for_dos_mitigation = 1; - log_debug(LD_DOS, "Client address %s has now %u concurrent connections.", - fmt_addr(&TO_CONN(or_conn)->addr), - entry->dos_stats.concurrent_count);

end: return; @@ -867,18 +896,8 @@ dos_close_client_conn(const or_connection_t *or_conn) goto end; }

- /* Extra super duper safety. Going below 0 means an underflow which could - * lead to most likely a false positive. In theory, this should never happen - * but lets be extra safe. */ - if (BUG(entry->dos_stats.concurrent_count == 0)) { - goto end; - } - - entry->dos_stats.concurrent_count--; - log_debug(LD_DOS, "Client address %s has lost a connection. Concurrent " - "connections are now at %u", - fmt_addr(&TO_CONN(or_conn)->addr), - entry->dos_stats.concurrent_count); + /* Update stats from this new close. */ + conn_update_on_close(&entry->dos_stats.conn_stats, &TO_CONN(or_conn)->addr);

end: return; diff --git a/src/core/or/dos.h b/src/core/or/dos.h index 9dba63531b..cadabdb2c9 100644 --- a/src/core/or/dos.h +++ b/src/core/or/dos.h @@ -34,6 +34,10 @@ typedef struct cc_client_stats_t {

/* Structure that keeps stats of client connection per-IP. */ typedef struct conn_client_stats_t { + /* Concurrent connection count from the specific address. 2^32 - 1 is most + * likely way too big for the amount of allowed file descriptors. */ + uint32_t concurrent_count; + /* Connect count from the specific address. We use a token bucket here to * track the rate and burst of connections from the same IP address.*/ token_bucket_ctr_t connect_count; @@ -48,10 +52,6 @@ typedef struct conn_client_stats_t { * per-IP client DoS mitigation. Because it is per-IP, it is used in the geoip * clientmap_entry_t object. */ typedef struct dos_client_stats_t { - /* Concurrent connection count from the specific address. 2^32 is most - * likely way too big for the amount of allowed file descriptors. */ - uint32_t concurrent_count; - /* Client connection statistics. */ conn_client_stats_t conn_stats;

diff --git a/src/test/test_dos.c b/src/test/test_dos.c index 850bbef59b..cbebecb030 100644 --- a/src/test/test_dos.c +++ b/src/test/test_dos.c @@ -470,7 +470,7 @@ test_known_relay(void *arg) GEOIP_CLIENT_CONNECT); tt_assert(entry); /* We should have a count of 0. */ - tt_uint_op(entry->dos_stats.concurrent_count, OP_EQ, 0); + tt_uint_op(entry->dos_stats.conn_stats.concurrent_count, OP_EQ, 0);

/* To make sure that his is working properly, make a unknown client * connection and see if we do get it. */ @@ -483,7 +483,7 @@ test_known_relay(void *arg) GEOIP_CLIENT_CONNECT); tt_assert(entry); /* We should have a count of 2. */ - tt_uint_op(entry->dos_stats.concurrent_count, OP_EQ, 2); + tt_uint_op(entry->dos_stats.conn_stats.concurrent_count, OP_EQ, 2);

done: routerstatus_free(rs); routerinfo_free(ri); microdesc_free(md);