From bb9b4c37f8e7f5cf78918f382e90d8b11ff42551 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 12:28:30 -0400 Subject: [PATCH 1/5] Supply better and less frequent warnings on circID exhaustion Fixes the surface behavior of #11553 --- changes/bug11553 | 5 +++++ src/or/channel.h | 2 ++ src/or/circuitbuild.c | 9 ++++++++- 3 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 changes/bug11553 diff --git a/changes/bug11553 b/changes/bug11553 new file mode 100644 index 0000000000..1540f4642f --- /dev/null +++ b/changes/bug11553 @@ -0,0 +1,5 @@ + o Minor features: + - When we run out of usable circuit IDs on a channel, log only one + warning for the whole channel, and include a description of + how many circuits there were on the channel. Fix for part of ticket + #11553. diff --git a/src/or/channel.h b/src/or/channel.h index 2dca81705f..29ba40e326 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -148,6 +148,8 @@ struct channel_s { ENUM_BF(circ_id_type_t) circ_id_type:2; /** DOCDOC*/ unsigned wide_circ_ids:1; + /** Have we logged a warning about circID exhaustion on this channel? */ + unsigned warned_circ_ids_exhausted:1; /* * Which circ_id do we try to use next on this connection? This is * always in the range 0..1<<15-1. diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index e47a2780af..2b4d3c3118 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -127,7 +127,14 @@ get_unique_circ_id_by_chan(channel_t *chan) /* Make sure we don't loop forever if all circ_id's are used. This * matters because it's an external DoS opportunity. */ - log_warn(LD_CIRC,"No unused circ IDs. Failing."); + if (! chan->warned_circ_ids_exhausted) { + chan->warned_circ_ids_exhausted = 1; + log_warn(LD_CIRC,"No unused circIDs found on channel %s wide " + "circID support, with %u inbound and %u outbound circuits. " + "Failing a circuit.", + chan->wide_circ_ids ? "with" : "without", + chan->num_p_circuits, chan->num_n_circuits); + } return 0; } test_circ_id |= high_bit; From 0d75344b0e0eafc89db89a974e87b16564cd8f0a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 12:50:04 -0400 Subject: [PATCH 2/5] Switch to random allocation on circuitIDs. Fixes a possible root cause of 11553 by only making 64 attempts at most to pick a circuitID. Previously, we would test every possible circuit ID until we found one or ran out. This algorithm succeeds probabilistically. As the comment says: This potentially causes us to give up early if our circuit ID space is nearly full. If we have N circuit IDs in use, then we will reject a new circuit with probability (N / max_range) ^ MAX_CIRCID_ATTEMPTS. This means that in practice, a few percent of our circuit ID capacity will go unused. The alternative here, though, is to do a linear search over the whole circuit ID space every time we extend a circuit, which is not so great either. This makes new vs old clients distinguishable, so we should try to batch it with other patches that do that, like 11438. --- changes/bug11553 | 10 ++++++++++ src/or/channel.c | 3 --- src/or/channel.h | 5 ----- src/or/circuitbuild.c | 43 ++++++++++++++++++++++++++++--------------- src/or/connection.c | 2 -- 5 files changed, 38 insertions(+), 25 deletions(-) diff --git a/changes/bug11553 b/changes/bug11553 index 1540f4642f..ed30ce9850 100644 --- a/changes/bug11553 +++ b/changes/bug11553 @@ -3,3 +3,13 @@ warning for the whole channel, and include a description of how many circuits there were on the channel. Fix for part of ticket #11553. + + + o Major features (performance): + - Avoid wasting cycles looking for usable circuit IDs. Previously, + when allocating a new circuit ID, we would in the worst case do a + linear scan over the entire possible range of circuit IDs before + deciding that we had exhausted our possibilities. Now, we + try 64 circuit IDs at random before deciding that we probably + won't succeed. Fix for a possible root cause of ticket + #11553. diff --git a/src/or/channel.c b/src/or/channel.c index 1270eace7d..dfef703b1b 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -731,9 +731,6 @@ channel_init(channel_t *chan) /* Init timestamp */ chan->timestamp_last_added_nonpadding = time(NULL); - /* Init next_circ_id */ - chan->next_circ_id = crypto_rand_int(1 << 15); - /* Initialize queues. */ TOR_SIMPLEQ_INIT(&chan->incoming_queue); TOR_SIMPLEQ_INIT(&chan->outgoing_queue); diff --git a/src/or/channel.h b/src/or/channel.h index 29ba40e326..e63c94961f 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -150,11 +150,6 @@ struct channel_s { unsigned wide_circ_ids:1; /** Have we logged a warning about circID exhaustion on this channel? */ unsigned warned_circ_ids_exhausted:1; - /* - * Which circ_id do we try to use next on this connection? This is - * always in the range 0..1<<15-1. - */ - circid_t next_circ_id; /* For how many circuits are we n_chan? What about p_chan? */ unsigned int num_n_circuits, num_p_circuits; diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 2b4d3c3118..1b3c5991ba 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -92,18 +92,21 @@ channel_connect_for_circuit(const tor_addr_t *addr, uint16_t port, return chan; } -/** Iterate over values of circ_id, starting from conn-\>next_circ_id, - * and with the high bit specified by conn-\>circ_id_type, until we get - * a circ_id that is not in use by any other circuit on that conn. + +/** Search for a value for circ_id that we can use on chan for an + * outbound circuit, until we get a circ_id that is not in use by any other + * circuit on that conn. * * Return it, or 0 if can't get a unique circ_id. */ static circid_t get_unique_circ_id_by_chan(channel_t *chan) { +#define MAX_CIRCID_ATTEMPTS 64 + circid_t test_circ_id; circid_t attempts=0; - circid_t high_bit, max_range; + circid_t high_bit, max_range, mask; tor_assert(chan); @@ -113,19 +116,26 @@ get_unique_circ_id_by_chan(channel_t *chan) "a client with no identity."); return 0; } - max_range = (chan->wide_circ_ids) ? (1u<<31) : (1u<<15); + max_range = (chan->wide_circ_ids) ? (1u<<31) : (1u<<15); + mask = max_range - 1; high_bit = (chan->circ_id_type == CIRC_ID_TYPE_HIGHER) ? max_range : 0; do { - /* Sequentially iterate over test_circ_id=1...max_range until we find a - * circID such that (high_bit|test_circ_id) is not already used. */ - test_circ_id = chan->next_circ_id++; - if (test_circ_id == 0 || test_circ_id >= max_range) { - test_circ_id = 1; - chan->next_circ_id = 2; - } - if (++attempts > max_range) { - /* Make sure we don't loop forever if all circ_id's are used. This - * matters because it's an external DoS opportunity. + if (++attempts > MAX_CIRCID_ATTEMPTS) { + /* Make sure we don't loop forever because all circuit IDs are used. + * + * Once, we would try until we had tried every possible circuit ID. But + * that's quite expensive. Instead, we try MAX_CIRCID_ATTEMPTS random + * circuit IDs, and then give up. + * + * This potentially causes us to give up early if our circuit ID space + * is nearly full. If we have N circuit IDs in use, then we will reject + * a new circuit with probability (N / max_range) ^ MAX_CIRCID_ATTEMPTS. + * This means that in practice, a few percent of our circuit ID capacity + * will go unused. + * + * The alternative here, though, is to do a linear search over the + * whole circuit ID space every time we extend a circuit, which is + * not so great either. */ if (! chan->warned_circ_ids_exhausted) { chan->warned_circ_ids_exhausted = 1; @@ -137,6 +147,9 @@ get_unique_circ_id_by_chan(channel_t *chan) } return 0; } + + crypto_rand((char*) &test_circ_id, sizeof(test_circ_id)); + test_circ_id &= mask; test_circ_id |= high_bit; } while (circuit_id_in_use_on_channel(test_circ_id, chan)); return test_circ_id; diff --git a/src/or/connection.c b/src/or/connection.c index 4f74a1d04b..96e6a471bb 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -251,8 +251,6 @@ dir_connection_new(int socket_family) * * Set timestamp_last_added_nonpadding to now. * - * Assign a pseudorandom next_circ_id between 0 and 2**15. - * * Initialize active_circuit_pqueue. * * Set active_circuit_pqueue_last_recalibrated to current cell_ewma tick. From 47a0c10728712cbfa3683f218b9379299b968636 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 13:04:37 -0400 Subject: [PATCH 3/5] Diagnostic warning to see if it's pending destroys causing 11553 --- src/or/circuitbuild.c | 16 +++++++++++++--- src/or/circuitlist.c | 14 +++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index e1d57ad6eb..e4e811529f 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -88,7 +88,8 @@ static circid_t get_unique_circ_id_by_chan(channel_t *chan) { #define MAX_CIRCID_ATTEMPTS 64 - + int in_use; + unsigned n_with_circ = 0, n_pending_destroy = 0; circid_t test_circ_id; circid_t attempts=0; circid_t high_bit, max_range, mask; @@ -126,9 +127,12 @@ get_unique_circ_id_by_chan(channel_t *chan) chan->warned_circ_ids_exhausted = 1; log_warn(LD_CIRC,"No unused circIDs found on channel %s wide " "circID support, with %u inbound and %u outbound circuits. " + "Found %u circuit IDs in use by circuits, and %u with " + "pending destroy cells." "Failing a circuit.", chan->wide_circ_ids ? "with" : "without", - chan->num_p_circuits, chan->num_n_circuits); + chan->num_p_circuits, chan->num_n_circuits, + n_with_circ, n_pending_destroy); } return 0; } @@ -136,7 +140,13 @@ get_unique_circ_id_by_chan(channel_t *chan) crypto_rand((char*) &test_circ_id, sizeof(test_circ_id)); test_circ_id &= mask; test_circ_id |= high_bit; - } while (circuit_id_in_use_on_channel(test_circ_id, chan)); + + in_use = circuit_id_in_use_on_channel(test_circ_id, chan); + if (in_use == 1) + ++n_with_circ; + else if (in_use == 2) + ++n_pending_destroy; + } while (in_use); return test_circ_id; } diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index b71dc3c13a..8a8fc8b4ed 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1065,13 +1065,21 @@ circuit_get_by_circid_channel_even_if_marked(circid_t circ_id, } /** Return true iff the circuit ID circ_id is currently used by a - * circuit, marked or not, on chan. */ + * circuit, marked or not, on chan, or if the circ ID is reserved until + * a queued destroy cell can be sent. + * + * (Return 1 if the circuit is present, marked or not; Return 2 + * if the circuit ID is pending a destroy.) + **/ int circuit_id_in_use_on_channel(circid_t circ_id, channel_t *chan) { int found = 0; - return circuit_get_by_circid_channel_impl(circ_id, chan, &found) != NULL - || found; + if (circuit_get_by_circid_channel_impl(circ_id, chan, &found) != NULL) + return 1; + if (found) + return 2; + return 0; } /** Return the circuit that a given edge connection is using. */ From 985deaaaf7b7397857e02206e89392e0ee101077 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Apr 2014 13:22:42 -0400 Subject: [PATCH 4/5] Add a rate-limiter for the other circuitID exhaustion warning --- src/or/circuitbuild.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 1b3c5991ba..7b852ff5c6 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -596,7 +596,9 @@ circuit_deliver_create_cell(circuit_t *circ, const create_cell_t *create_cell, id = get_unique_circ_id_by_chan(circ->n_chan); if (!id) { - log_warn(LD_CIRC,"failed to get unique circID."); + static ratelim_t circid_warning_limit = RATELIM_INIT(9600); + log_fn_ratelim(&circid_warning_limit, LOG_WARN, LD_CIRC, + "failed to get unique circID."); return -1; } log_debug(LD_CIRC,"Chosen circID %u.", (unsigned)id); From a770b74501a3faf6483c40735b70adae6fb95187 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 23 Apr 2014 12:39:01 -0400 Subject: [PATCH 5/5] Improvements to #11553 fix based on review Use a per-channel ratelim_t to control the rate at which we report failures for each channel. Explain why I picked N=32. Never return a zero circID. Thanks to Andrea and to cypherpunks. --- src/or/channel.c | 3 +++ src/or/channel.h | 6 ++++-- src/or/circuitbuild.c | 19 +++++++++++++------ 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index dfef703b1b..691da174ba 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -731,6 +731,9 @@ channel_init(channel_t *chan) /* Init timestamp */ chan->timestamp_last_added_nonpadding = time(NULL); + /* Warn about exhausted circuit IDs no more than hourly. */ + chan->last_warned_circ_ids_exhausted.rate = 3600; + /* Initialize queues. */ TOR_SIMPLEQ_INIT(&chan->incoming_queue); TOR_SIMPLEQ_INIT(&chan->outgoing_queue); diff --git a/src/or/channel.h b/src/or/channel.h index e63c94961f..63da0c1c24 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -148,8 +148,6 @@ struct channel_s { ENUM_BF(circ_id_type_t) circ_id_type:2; /** DOCDOC*/ unsigned wide_circ_ids:1; - /** Have we logged a warning about circID exhaustion on this channel? */ - unsigned warned_circ_ids_exhausted:1; /* For how many circuits are we n_chan? What about p_chan? */ unsigned int num_n_circuits, num_p_circuits; @@ -178,6 +176,10 @@ struct channel_s { */ unsigned int is_local:1; + /** Have we logged a warning about circID exhaustion on this channel? + * If so, when? */ + ratelim_t last_warned_circ_ids_exhausted; + /** Channel timestamps for cell channels */ time_t timestamp_client; /* Client used this, according to relay.c */ time_t timestamp_drained; /* Output queue empty */ diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 7b852ff5c6..8d6aad605c 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -102,6 +102,12 @@ channel_connect_for_circuit(const tor_addr_t *addr, uint16_t port, static circid_t get_unique_circ_id_by_chan(channel_t *chan) { +/* This number is chosen somewhat arbitrarily; see comment below for more + * info. When the space is 80% full, it gives a one-in-a-million failure + * chance; when the space is 90% full, it gives a one-in-850 chance; and when + * the space is 95% full, it gives a one-in-26 failure chance. That seems + * okay, though you could make a case IMO for anything between N=32 and + * N=256. */ #define MAX_CIRCID_ATTEMPTS 64 circid_t test_circ_id; @@ -137,19 +143,20 @@ get_unique_circ_id_by_chan(channel_t *chan) * whole circuit ID space every time we extend a circuit, which is * not so great either. */ - if (! chan->warned_circ_ids_exhausted) { - chan->warned_circ_ids_exhausted = 1; - log_warn(LD_CIRC,"No unused circIDs found on channel %s wide " + log_fn_ratelim(&chan->last_warned_circ_ids_exhausted, LOG_WARN, + LD_CIRC,"No unused circIDs found on channel %s wide " "circID support, with %u inbound and %u outbound circuits. " "Failing a circuit.", chan->wide_circ_ids ? "with" : "without", chan->num_p_circuits, chan->num_n_circuits); - } return 0; } - crypto_rand((char*) &test_circ_id, sizeof(test_circ_id)); - test_circ_id &= mask; + do { + crypto_rand((char*) &test_circ_id, sizeof(test_circ_id)); + test_circ_id &= mask; + } while (test_circ_id == 0); + test_circ_id |= high_bit; } while (circuit_id_in_use_on_channel(test_circ_id, chan)); return test_circ_id;