From d7f14a54fbf520ffc56d162426f649ecde12e287 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 23 Oct 2023 21:48:09 +0000 Subject: [PATCH 1/3] Bug 40876: Don't reduce primary list for temporary restrictions --- src/feature/client/entrynodes.c | 49 +++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/src/feature/client/entrynodes.c b/src/feature/client/entrynodes.c index e5c89645f6..45fd3a4d9a 100644 --- a/src/feature/client/entrynodes.c +++ b/src/feature/client/entrynodes.c @@ -1681,6 +1681,17 @@ guard_obeys_md_dirserver_restriction(const entry_guard_t *guard) return 1; } +/** + * Return true if a restriction is reachability related, such that it should + * cause us to consider additional primary guards when selecting one. + */ +static bool +entry_guard_restriction_is_reachability(const entry_guard_restriction_t *rst) +{ + tor_assert(rst); + return (rst->type == RST_OUTDATED_MD_DIRSERVER); +} + /** * Return true iff guard obeys the restrictions defined in rst. * (If rst is NULL, there are no restrictions.) @@ -2127,14 +2138,44 @@ select_primary_guard_for_circuit(guard_selection_t *gs, const int need_descriptor = (usage == GUARD_USAGE_TRAFFIC); entry_guard_t *chosen_guard = NULL; - int num_entry_guards = get_n_primary_guards_to_use(usage); + int num_entry_guards_to_consider = get_n_primary_guards_to_use(usage); smartlist_t *usable_primary_guards = smartlist_new(); + int num_entry_guards_considered = 0; SMARTLIST_FOREACH_BEGIN(gs->primary_entry_guards, entry_guard_t *, guard) { entry_guard_consider_retry(guard); if (!entry_guard_obeys_restriction(guard, rst)) { log_info(LD_GUARD, "Entry guard %s doesn't obey restriction, we test the" " next one", entry_guard_describe(guard)); + if (!entry_guard_restriction_is_reachability(rst)) { + log_info(LD_GUARD, + "Skipping guard %s due to circuit path restriction. " + "Have %d, considered: %d, to consider: %d", + entry_guard_describe(guard), + smartlist_len(usable_primary_guards), + num_entry_guards_considered, + num_entry_guards_to_consider); + /* If the restriction is a circuit path restriction (as opposed to a + * reachability restriction), count this as considered. */ + num_entry_guards_considered++; + + /* If we have considered enough guards, *and* we actually have a guard, + * then proceed to select one from the list. */ + if (num_entry_guards_considered >= num_entry_guards_to_consider) { + /* This should not happen with 2-leg conflux unless there is a + * race between removing a failed leg and a retry, but check + * anyway and log. */ + if (smartlist_len(usable_primary_guards) == 0) { + static ratelim_t guardlog = RATELIM_INIT(60); + log_fn_ratelim(&guardlog, LOG_NOTICE, LD_GUARD, + "All current guards excluded by path restriction " + "type %d; using an additonal guard.", + rst->type); + } else { + break; + } + } + } continue; } if (guard->is_reachable != GUARD_REACHABLE_NO) { @@ -2146,7 +2187,11 @@ select_primary_guard_for_circuit(guard_selection_t *gs, *state_out = GUARD_CIRC_STATE_USABLE_ON_COMPLETION; guard->last_tried_to_connect = approx_time(); smartlist_add(usable_primary_guards, guard); - if (smartlist_len(usable_primary_guards) >= num_entry_guards) + num_entry_guards_considered++; + + /* If we have considered enough guards, then proceed to select + * one from the list. */ + if (num_entry_guards_considered >= num_entry_guards_to_consider) { break; } } SMARTLIST_FOREACH_END(guard); From 6bfadc7a5db2d3d6ca1449a43edc711814934ca9 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 25 Oct 2023 00:55:43 +0000 Subject: [PATCH 2/3] Bug 40876: Extra logging --- src/feature/client/entrynodes.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/feature/client/entrynodes.c b/src/feature/client/entrynodes.c index 45fd3a4d9a..7c469ea84a 100644 --- a/src/feature/client/entrynodes.c +++ b/src/feature/client/entrynodes.c @@ -2193,6 +2193,10 @@ select_primary_guard_for_circuit(guard_selection_t *gs, * one from the list. */ if (num_entry_guards_considered >= num_entry_guards_to_consider) { break; + } + } else { + log_info(LD_GUARD, "Guard %s is not reachable", + entry_guard_describe(guard)); } } SMARTLIST_FOREACH_END(guard); @@ -2202,6 +2206,10 @@ select_primary_guard_for_circuit(guard_selection_t *gs, "Selected primary guard %s for circuit from a list size of %d.", entry_guard_describe(chosen_guard), smartlist_len(usable_primary_guards)); + /* Describe each guard in the list: */ + SMARTLIST_FOREACH_BEGIN(usable_primary_guards, entry_guard_t *, guard) { + log_info(LD_GUARD, " %s", entry_guard_describe(guard)); + } SMARTLIST_FOREACH_END(guard); smartlist_free(usable_primary_guards); } @@ -2307,16 +2315,22 @@ select_entry_guard_for_circuit(guard_selection_t *gs, /* "If any entry in PRIMARY_GUARDS has {is_reachable} status of or , return the first such guard." */ chosen_guard = select_primary_guard_for_circuit(gs, usage, rst, state_out); - if (chosen_guard) + if (chosen_guard) { + log_info(LD_GUARD, "Selected primary guard %s for circuit.", + entry_guard_describe(chosen_guard)); return chosen_guard; + } /* "Otherwise, if the ordered intersection of {CONFIRMED_GUARDS} and {USABLE_FILTERED_GUARDS} is nonempty, return the first entry in that intersection that has {is_pending} set to false." */ chosen_guard = select_confirmed_guard_for_circuit(gs, usage, rst, state_out); - if (chosen_guard) + if (chosen_guard) { + log_info(LD_GUARD, "Selected confirmed guard %s for circuit.", + entry_guard_describe(chosen_guard)); return chosen_guard; + } /* "Otherwise, if there is no such entry, select a member * {USABLE_FILTERED_GUARDS} following the sample ordering" */ @@ -2329,6 +2343,8 @@ select_entry_guard_for_circuit(guard_selection_t *gs, return NULL; } + log_info(LD_GUARD, "Selected filtered guard %s for circuit.", + entry_guard_describe(chosen_guard)); return chosen_guard; } From d4d78f5033ec9e09c6e680987f12cebd5e3a282f Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 25 Oct 2023 18:00:19 +0000 Subject: [PATCH 3/3] Bug 40876 changes file --- changes/bug40876 | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changes/bug40876 diff --git a/changes/bug40876 b/changes/bug40876 new file mode 100644 index 0000000000..a467cf64c1 --- /dev/null +++ b/changes/bug40876 @@ -0,0 +1,8 @@ + o Major bugfixes (guard usage): + - When Tor excluded a guard due to temporary circuit restrictions, + it considered *additional* primary guards for potential usage + by that circuit. This could result in more than the specified number + of guards (currently 2) being used, long-term, by the tor client. + This could happen when a Guard was also selected as an Exit node, + but it was exacerbated by the Conflux guard restrictions. Both + instances have been fixed. Fixes bug 40876; bugfix on 0.3.0.1-alpha.