From 880c71304b6592fccd812315d8c27d234b694221 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 20 Aug 2012 19:03:00 -0700 Subject: [PATCH 1/4] Disable path bias accounting if we have no guards. This should eliminate a lot of notices for Directory Authorities and other situations where circuits built without using guard nodes. --- src/or/circuitbuild.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 39a223b2f4..35a32d8641 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -2638,6 +2638,14 @@ pathbias_count_first_hop(origin_circuit_t *circ) RATELIM_INIT(FIRST_HOP_NOTICE_INTERVAL); char *rate_msg = NULL; + /* We can't do path bias accounting without entry guards. + * Testing and controller circuits also have no guards. */ + if (get_options()->UseEntryGuards == 0 || + circ->_base.purpose == CIRCUIT_PURPOSE_TESTING || + circ->_base.purpose == CIRCUIT_PURPOSE_CONTROLLER) { + return 0; + } + /* Completely ignore one hop circuits */ if (circ->build_state->onehop_tunnel) { tor_assert(circ->build_state->desired_path_len == 1); @@ -2732,6 +2740,14 @@ pathbias_count_success(origin_circuit_t *circ) RATELIM_INIT(SUCCESS_NOTICE_INTERVAL); char *rate_msg = NULL; + /* We can't do path bias accounting without entry guards. + * Testing and controller circuits also have no guards. */ + if (get_options()->UseEntryGuards == 0 || + circ->_base.purpose == CIRCUIT_PURPOSE_TESTING || + circ->_base.purpose == CIRCUIT_PURPOSE_CONTROLLER) { + return; + } + /* Ignore one hop circuits */ if (circ->build_state->onehop_tunnel) { tor_assert(circ->build_state->desired_path_len == 1); @@ -2770,7 +2786,10 @@ pathbias_count_success(origin_circuit_t *circ) guard->circuit_successes, guard->first_hops, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); } - } else { + /* In rare cases, CIRCUIT_PURPOSE_TESTING can get converted to + * CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT and have no guards here. + * No need to log that case. */ + } else if (circ->_base.purpose != CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) { if ((rate_msg = rate_limit_log(&success_notice_limit, approx_time()))) { log_notice(LD_BUG, From 4950618b135073f23f250520b01a80dd36e5a43f Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 21 Aug 2012 18:37:08 -0700 Subject: [PATCH 2/4] Bug 6475: Demote pathbias log messages for 0.2.3.x Also make a couple of them less scary. We'll do a separate, additional commit on 0.2.4.x to bump them back up again. --- src/or/circuitbuild.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 35a32d8641..8ad6d1744b 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -2657,7 +2657,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) if (circ->has_opened && circ->path_state != PATH_STATE_DID_FIRST_HOP) { if ((rate_msg = rate_limit_log(&first_hop_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Opened circuit is in strange path state %s. " "Circuit is a %s currently %s. %s", pathbias_state_to_string(circ->path_state), @@ -2684,7 +2684,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) } else { if ((rate_msg = rate_limit_log(&first_hop_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Unopened circuit has strange path state %s. " "Circuit is a %s currently %s. %s", pathbias_state_to_string(circ->path_state), @@ -2696,7 +2696,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) } else { if ((rate_msg = rate_limit_log(&first_hop_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Unopened circuit has no known guard. " "Circuit is a %s currently %s. %s", circuit_purpose_to_string(circ->_base.purpose), @@ -2710,7 +2710,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) if (circ->path_state == PATH_STATE_NEW_CIRC) { if ((rate_msg = rate_limit_log(&first_hop_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "A %s circuit is in cpath state %d (opened: %d). " "Circuit is a %s currently %s. %s", pathbias_state_to_string(circ->path_state), @@ -2770,7 +2770,7 @@ pathbias_count_success(origin_circuit_t *circ) } else { if ((rate_msg = rate_limit_log(&success_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Succeeded circuit is in strange path state %s. " "Circuit is a %s currently %s. %s", pathbias_state_to_string(circ->path_state), @@ -2781,7 +2781,7 @@ pathbias_count_success(origin_circuit_t *circ) } if (guard->first_hops < guard->circuit_successes) { - log_warn(LD_BUG, "Unexpectedly high circuit_successes (%u/%u) " + log_notice(LD_BUG, "Unexpectedly high circuit_successes (%u/%u) " "for guard %s=%s", guard->circuit_successes, guard->first_hops, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); @@ -2792,7 +2792,7 @@ pathbias_count_success(origin_circuit_t *circ) } else if (circ->_base.purpose != CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) { if ((rate_msg = rate_limit_log(&success_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Completed circuit has no known guard. " "Circuit is a %s currently %s. %s", circuit_purpose_to_string(circ->_base.purpose), @@ -2804,7 +2804,7 @@ pathbias_count_success(origin_circuit_t *circ) if (circ->path_state != PATH_STATE_SUCCEEDED) { if ((rate_msg = rate_limit_log(&success_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Opened circuit is in strange path state %s. " "Circuit is a %s currently %s. %s", pathbias_state_to_string(circ->path_state), @@ -2834,9 +2834,11 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) if (guard->circuit_successes/((double)guard->first_hops) < pathbias_get_disable_rate(options)) { + /* This message is currently disabled by default. */ log_warn(LD_PROTOCOL, "Extremely low circuit success rate %u/%u for guard %s=%s. " - "This might indicate an attack, or a bug.", + "This indicates either an overloaded guard, an attack, or " + "a bug.", guard->circuit_successes, guard->first_hops, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); @@ -2857,6 +2859,11 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) /* If we get a ton of circuits, just scale everything down */ if (guard->first_hops > (unsigned)pathbias_get_scale_threshold(options)) { const int scale_factor = pathbias_get_scale_factor(options); + log_info(LD_PROTOCOL, + "Scaling pathbias counts to (%u/%u)/%d for guard %s=%s", + guard->circuit_successes, guard->first_hops, + scale_factor, guard->nickname, hex_str(guard->identity, + DIGEST_LEN)); guard->first_hops /= scale_factor; guard->circuit_successes /= scale_factor; } From e13abda470c347f95cb2bc9fc499bf93d1f85a68 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 23 Aug 2012 20:27:41 -0700 Subject: [PATCH 3/4] Bug 6647: Use correct scale constant and prevent rounding error We were effectively resetting our counts, and the rounding error leads to incorrect log messages. --- src/or/circuitbuild.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 8ad6d1744b..fc4d4ae13d 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -2601,12 +2601,12 @@ pathbias_get_scale_threshold(const or_options_t *options) static int pathbias_get_scale_factor(const or_options_t *options) { -#define DFLT_PATH_BIAS_SCALE_FACTOR 4 +#define DFLT_PATH_BIAS_SCALE_FACTOR 2 if (options->PathBiasScaleFactor >= 1) return options->PathBiasScaleFactor; else return networkstatus_get_param(NULL, "pb_scalefactor", - DFLT_PATH_BIAS_SCALE_THRESHOLD, 1, INT32_MAX); + DFLT_PATH_BIAS_SCALE_FACTOR, 1, INT32_MAX); } static const char * @@ -2859,13 +2859,18 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) /* If we get a ton of circuits, just scale everything down */ if (guard->first_hops > (unsigned)pathbias_get_scale_threshold(options)) { const int scale_factor = pathbias_get_scale_factor(options); - log_info(LD_PROTOCOL, - "Scaling pathbias counts to (%u/%u)/%d for guard %s=%s", - guard->circuit_successes, guard->first_hops, - scale_factor, guard->nickname, hex_str(guard->identity, - DIGEST_LEN)); - guard->first_hops /= scale_factor; - guard->circuit_successes /= scale_factor; + /* For now, only scale if there will be no rounding error... + * XXX024: We want to switch to a real moving average for 0.2.4. */ + if ((guard->first_hops % scale_factor) == 0 && + (guard->circuit_successes % scale_factor) == 0) { + log_info(LD_PROTOCOL, + "Scaling pathbias counts to (%u/%u)/%d for guard %s=%s", + guard->circuit_successes, guard->first_hops, + scale_factor, guard->nickname, hex_str(guard->identity, + DIGEST_LEN)); + guard->first_hops /= scale_factor; + guard->circuit_successes /= scale_factor; + } } guard->first_hops++; log_info(LD_PROTOCOL, "Got success count %u/%u for guard %s=%s", From 0c13547ea9204975a04ba53daff25020594fbd47 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 27 Aug 2012 11:15:35 -0700 Subject: [PATCH 4/4] Bug 6647: Add changes file --- changes/bug6647 | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changes/bug6647 diff --git a/changes/bug6647 b/changes/bug6647 new file mode 100644 index 0000000000..ef29ae3309 --- /dev/null +++ b/changes/bug6647 @@ -0,0 +1,7 @@ + o Minor bugfixes: + - Prevent rounding error in path bias counts when scaling + them down, and use the correct scale factor default. + Bugfix against 0.2.3.17-beta. + - Demote some path bias related log messages down a level + and make others less scary sounding. + Bugfix against 0.2.3.17-beta.