commit cbc495453cb522db1584525a06d26386f5819e05 Author: David Goulet dgoulet@torproject.org Date: Thu Oct 31 08:06:16 2019 -0400

hs-v3: Give a cleanup type to hs_circ_cleanup()

By centralizing the circuit cleanup type that is: on close, free and repurpose, some actions on the circuit can not happen for a certain cleanup type or for all types.

This passes a cleanup type so the HS subsystem (v2 and v3) can take actions based on the type of cleanup.

For instance, there is slow code that we do not run on a circuit close but rather only on free.

Part of #32020

Signed-off-by: David Goulet dgoulet@torproject.org --- src/core/or/circuitlist.c | 4 +- src/core/or/circuituse.c | 2 +- src/feature/hs/hs_circuit.c | 94 ++++++++++++++++++++++++++++++++++--------- src/feature/hs/hs_circuit.h | 6 ++- src/feature/hs/hs_client.c | 2 +- src/feature/hs/hs_client.h | 4 +- src/feature/rend/rendclient.c | 2 +- src/feature/rend/rendclient.h | 3 +- 8 files changed, 88 insertions(+), 29 deletions(-)

diff --git a/src/core/or/circuitlist.c b/src/core/or/circuitlist.c index 3ae482a5b..49a63c50a 100644 --- a/src/core/or/circuitlist.c +++ b/src/core/or/circuitlist.c @@ -1137,7 +1137,7 @@ circuit_free_(circuit_t *circ) * circuit is closed. This is to avoid any code path that free registered * circuits without closing them before. This needs to be done before the * hs identifier is freed. */ - hs_circ_cleanup(circ); + hs_circ_cleanup_on_free(circ);

if (CIRCUIT_IS_ORIGIN(circ)) { origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); @@ -2261,7 +2261,7 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line, }

/* Notify the HS subsystem that this circuit is closing. */ - hs_circ_cleanup(circ); + hs_circ_cleanup_on_close(circ);

if (circuits_pending_close == NULL) circuits_pending_close = smartlist_new(); diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index 8a667024d..ad9841cc3 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -3124,7 +3124,7 @@ circuit_change_purpose(circuit_t *circ, uint8_t new_purpose) /* Take specific actions if we are repurposing a hidden service circuit. */ if (circuit_purpose_is_hidden_service(circ->purpose) && !circuit_purpose_is_hidden_service(new_purpose)) { - hs_circ_cleanup(circ); + hs_circ_cleanup_on_repurpose(circ); } }

diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index 8640129f3..a43e7f0e2 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -621,16 +621,16 @@ setup_introduce1_data(const hs_desc_intro_point_t *ip, }

/** Helper: cleanup function for client circuit. This is for every HS version. - * It is called from hs_circ_cleanup() entry point. */ + * It is called from hs_circ_cleanup_on_free() entry point. */ static void -cleanup_client_circ(circuit_t *circ) +cleanup_on_free_client_circ(circuit_t *circ) { tor_assert(circ);

if (circuit_is_hs_v2(circ)) { - rend_client_circuit_cleanup(circ); + rend_client_circuit_cleanup_on_free(circ); } else if (circuit_is_hs_v3(circ)) { - hs_client_circuit_cleanup(circ); + hs_client_circuit_cleanup_on_free(circ); } /* It is possible the circuit has an HS purpose but no identifier (rend_data * or hs_ident). Thus possible that this passess through. */ @@ -1215,29 +1215,83 @@ hs_circ_send_establish_rendezvous(origin_circuit_t *circ) return -1; }

-/** We are about to close or free this <b>circ</b>. Clean it up from any - * related HS data structures. This function can be called multiple times - * safely for the same circuit. */ +/** Circuit cleanup strategy: + * + * What follows is a series of functions that notifies the HS subsystem of 3 + * different circuit cleanup phase: close, free and repurpose. + * + * Tor can call any of those in any orders so they have to be safe between + * each other. In other words, the free should never depend on close to be + * called before. + * + * The "on_close()" is called from circuit_mark_for_close() which is + * considered the tor fast path and thus as little work as possible should + * done in that function. Currently, we only remove the circuit from the HS + * circuit map and move on. + * + * The "on_free()" is called from circuit circuit_free_() and it is very + * important that at the end of the function, no state or objects related to + * this circuit remains alive. + * + * The "on_repurpose()" is called from circuit_change_purpose() for which we + * simply remove it from the HS circuit map. We do not have other cleanup + * requirements after that. + * + * NOTE: The onion service code, specifically the service code, cleans up + * lingering objects or state if any of its circuit disappear which is why + * our cleanup strategy doesn't involve any service specific actions. As long + * as the circuit is removed from the HS circuit map, it won't be used. + */ + +/** We are about to close this <b>circ</b>. Clean it up from any related HS + * data structures. This function can be called multiple times safely for the + * same circuit. */ void -hs_circ_cleanup(circuit_t *circ) +hs_circ_cleanup_on_close(circuit_t *circ) { tor_assert(circ);

+ /* On close, we simply remove it from the circuit map. It can not be used + * anymore. We keep this code path fast and lean. */ + + if (circ->hs_token) { + hs_circuitmap_remove_circuit(circ); + } +} + +/** We are about to free this <b>circ</b>. Clean it up from any related HS + * data structures. This function can be called multiple times safely for the + * same circuit. */ +void +hs_circ_cleanup_on_free(circuit_t *circ) +{ + tor_assert(circ); + + /* NOTE: Bulk of the work of cleaning up a circuit is done here. */ + if (circuit_purpose_is_hs_client(circ->purpose)) { - cleanup_client_circ(circ); + cleanup_on_free_client_circ(circ); + } + + /* We have no assurance that the given HS circuit has been closed before and + * thus removed from the HS map. This actually happens in unit tests. */ + if (circ->hs_token) { + hs_circuitmap_remove_circuit(circ); } +} + +/** We are about to repurpose this <b>circ</b>. Clean it up from any related + * HS data structures. This function can be called multiple times safely for + * the same circuit. */ +void +hs_circ_cleanup_on_repurpose(circuit_t *circ) +{ + tor_assert(circ); + + /* On repurpose, we simply remove it from the circuit map but we do not do + * the on_free actions since we don't treat a repurpose as something we need + * to report in the client cache failure. */

- /* Actions that MUST happen for every circuits regardless of what was done - * on it before. */ - - /* Clear HS circuitmap token for this circ (if any). Very important to be - * done after the HS subsystem has been notified of the close else the - * circuit will not be found. - * - * We do this at the close if possible because from that point on, the - * circuit is good as dead. We can't rely on removing it in the circuit - * free() function because we open a race window between the close and free - * where we can't register a new circuit for the same intro point. */ if (circ->hs_token) { hs_circuitmap_remove_circuit(circ); } diff --git a/src/feature/hs/hs_circuit.h b/src/feature/hs/hs_circuit.h index c817f3e37..42e5ca134 100644 --- a/src/feature/hs/hs_circuit.h +++ b/src/feature/hs/hs_circuit.h @@ -14,8 +14,10 @@

#include "feature/hs/hs_service.h"

-/* Cleanup function when the circuit is closed or/and freed. */ -void hs_circ_cleanup(circuit_t *circ); +/* Cleanup function when the circuit is closed or freed. */ +void hs_circ_cleanup_on_close(circuit_t *circ); +void hs_circ_cleanup_on_free(circuit_t *circ); +void hs_circ_cleanup_on_repurpose(circuit_t *circ);

/* Circuit API. */ int hs_circ_service_intro_has_opened(hs_service_t *service, diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index 716b6a26a..a2a47c1da 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -1525,7 +1525,7 @@ get_hs_client_auths_map(void) /** Called when a circuit was just cleaned up. This is done right before the * circuit is freed. */ void -hs_client_circuit_cleanup(const circuit_t *circ) +hs_client_circuit_cleanup_on_free(const circuit_t *circ) { bool has_timed_out; rend_intro_point_failure_t failure = INTRO_POINT_FAILURE_GENERIC; diff --git a/src/feature/hs/hs_client.h b/src/feature/hs/hs_client.h index a0f1c7758..23effc06b 100644 --- a/src/feature/hs/hs_client.h +++ b/src/feature/hs/hs_client.h @@ -10,6 +10,8 @@ #define TOR_HS_CLIENT_H

#include "lib/crypt_ops/crypto_ed25519.h" + +#include "feature/hs/hs_circuit.h" #include "feature/hs/hs_descriptor.h" #include "feature/hs/hs_ident.h"

@@ -112,7 +114,7 @@ int hs_client_send_introduce1(origin_circuit_t *intro_circ, origin_circuit_t *rend_circ);

void hs_client_circuit_has_opened(origin_circuit_t *circ); -void hs_client_circuit_cleanup(const circuit_t *circ); +void hs_client_circuit_cleanup_on_free(const circuit_t *circ);

int hs_client_receive_rendezvous_acked(origin_circuit_t *circ, const uint8_t *payload, diff --git a/src/feature/rend/rendclient.c b/src/feature/rend/rendclient.c index 632b00c85..14484f1ce 100644 --- a/src/feature/rend/rendclient.c +++ b/src/feature/rend/rendclient.c @@ -1254,7 +1254,7 @@ rend_parse_service_authorization(const or_options_t *options, /** The given circuit is being freed. Take appropriate action if it is of * interest to the client subsystem. */ void -rend_client_circuit_cleanup(const circuit_t *circ) +rend_client_circuit_cleanup_on_free(const circuit_t *circ) { int reason, orig_reason; bool has_timed_out, ip_is_redundant; diff --git a/src/feature/rend/rendclient.h b/src/feature/rend/rendclient.h index da6f4646d..63191737c 100644 --- a/src/feature/rend/rendclient.h +++ b/src/feature/rend/rendclient.h @@ -12,6 +12,7 @@ #ifndef TOR_RENDCLIENT_H #define TOR_RENDCLIENT_H

+#include "feature/hs/hs_circuit.h" #include "feature/rend/rendcache.h"

void rend_client_purge_state(void); @@ -47,7 +48,7 @@ rend_service_authorization_t *rend_client_lookup_service_authorization( const char *onion_address); void rend_service_authorization_free_all(void);

-void rend_client_circuit_cleanup(const circuit_t *circ); +void rend_client_circuit_cleanup_on_free(const circuit_t *circ);

#endif /* !defined(TOR_RENDCLIENT_H) */