diff --git a/changes/ticket40694 b/changes/ticket40694 new file mode 100644 index 0000000000..f17639cc27 --- /dev/null +++ b/changes/ticket40694 @@ -0,0 +1,5 @@ + o Major bugfixes (onion service): + - Set a much higher circuit build timeout for opened client rendezvous + circuit. Before this, tor would time them out very quickly leading to many + unnecessary retries and thus more load on the network. Fixes bug 40694; + bugfix on 0.3.5.1-alpha. diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index a259957d37..dbeea10821 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -133,11 +133,6 @@ circuit_is_acceptable(const origin_circuit_t *origin_circ, return 0; } - /* If this is a timed-out hidden service circuit, skip it. */ - if (origin_circ->hs_circ_has_timed_out) { - return 0; - } - if (purpose == CIRCUIT_PURPOSE_C_GENERAL || purpose == CIRCUIT_PURPOSE_C_HSDIR_GET || purpose == CIRCUIT_PURPOSE_S_HSDIR_POST || @@ -334,7 +329,6 @@ circuit_get_best(const entry_connection_t *conn, { origin_circuit_t *best=NULL; struct timeval now; - int intro_going_on_but_too_old = 0; tor_assert(conn); @@ -353,15 +347,6 @@ circuit_get_best(const entry_connection_t *conn, continue; origin_circ = TO_ORIGIN_CIRCUIT(circ); - /* Log an info message if we're going to launch a new intro circ in - * parallel */ - if (purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT && - !must_be_open && origin_circ->hs_circ_has_timed_out && - !circ->marked_for_close) { - intro_going_on_but_too_old = 1; - continue; - } - if (!circuit_is_acceptable(origin_circ,conn,must_be_open,purpose, need_uptime,need_internal, (time_t)now.tv_sec)) continue; @@ -374,11 +359,6 @@ circuit_get_best(const entry_connection_t *conn, } SMARTLIST_FOREACH_END(circ); - if (!best && intro_going_on_but_too_old) - log_info(LD_REND|LD_CIRC, "There is an intro circuit being created " - "right now, but it has already taken quite a while. Starting " - "one in parallel."); - return best; } @@ -450,8 +430,9 @@ circuit_expire_building(void) * circuit_build_times_get_initial_timeout() if we haven't computed * custom timeouts yet */ struct timeval general_cutoff, begindir_cutoff, fourhop_cutoff, - close_cutoff, extremely_old_cutoff, hs_extremely_old_cutoff, - cannibalized_cutoff, c_intro_cutoff, s_intro_cutoff, stream_cutoff; + close_cutoff, extremely_old_cutoff, + cannibalized_cutoff, c_intro_cutoff, s_intro_cutoff, stream_cutoff, + c_rend_ready_cutoff; const or_options_t *options = get_options(); struct timeval now; cpath_build_state_t *build_state; @@ -531,13 +512,19 @@ circuit_expire_building(void) /* Server intro circs have an extra round trip */ SET_CUTOFF(s_intro_cutoff, get_circuit_build_timeout_ms() * (9/6.0) + 1000); + /* For circuit purpose set to: CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED. + * + * The cutoff of such circuit is very high because we end up in this state if + * once the INTRODUCE_ACK is received which could be before the service + * receives the INTRODUCE2 cell. The worst case is a full 3-hop latency + * (intro -> service), 4-hop circuit creation latency (service -> RP), and + * finally a 7-hop latency for the RENDEZVOUS2 cell to arrive (service -> + * client). */ + SET_CUTOFF(c_rend_ready_cutoff, get_circuit_build_timeout_ms() * 3 + 1000); + SET_CUTOFF(close_cutoff, get_circuit_build_close_time_ms()); SET_CUTOFF(extremely_old_cutoff, get_circuit_build_close_time_ms()*2 + 1000); - SET_CUTOFF(hs_extremely_old_cutoff, - MAX(get_circuit_build_close_time_ms()*2 + 1000, - options->SocksTimeout * 1000)); - bool fixed_time = circuit_build_times_disabled(get_options()); SMARTLIST_FOREACH_BEGIN(circuit_get_global_list(), circuit_t *,victim) { @@ -568,8 +555,12 @@ circuit_expire_building(void) cutoff = c_intro_cutoff; else if (victim->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO) cutoff = s_intro_cutoff; - else if (victim->purpose == CIRCUIT_PURPOSE_C_ESTABLISH_REND) - cutoff = stream_cutoff; + else if (victim->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND) { + /* Service connecting to a rendezvous point is a four hop circuit. We set + * it explicitly here because this function is a clusterf***. */ + cutoff = fourhop_cutoff; + } else if (victim->purpose == CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED) + cutoff = c_rend_ready_cutoff; else if (victim->purpose == CIRCUIT_PURPOSE_PATH_BIAS_TESTING) cutoff = close_cutoff; else if (TO_ORIGIN_CIRCUIT(victim)->has_opened && @@ -580,9 +571,6 @@ circuit_expire_building(void) else cutoff = general_cutoff; - if (TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out) - cutoff = hs_extremely_old_cutoff; - if (timercmp(&victim->timestamp_began, &cutoff, OP_GT)) continue; /* it's still young, leave it alone */ @@ -754,51 +742,29 @@ circuit_expire_building(void) } } - /* If this is a hidden service client circuit which is far enough along in - * connecting to its destination, and we haven't already flagged it as - * 'timed out', flag it so we'll launch another intro or rend circ, but - * don't mark it for close yet. - * - * (Circs flagged as 'timed out' are given a much longer timeout - * period above, so we won't close them in the next call to - * circuit_expire_building.) */ - if (!(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out)) { - switch (victim->purpose) { - case CIRCUIT_PURPOSE_C_REND_READY: - /* We only want to spare a rend circ iff it has been specified in an - * INTRODUCE1 cell sent to a hidden service. */ - if (!hs_circ_is_rend_sent_in_intro1(CONST_TO_ORIGIN_CIRCUIT(victim))) { - break; - } - FALLTHROUGH; - case CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT: - case CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED: - /* If we have reached this line, we want to spare the circ for now. */ - log_info(LD_CIRC,"Marking circ %u (state %d:%s, purpose %d) " - "as timed-out HS circ", - (unsigned)victim->n_circ_id, - victim->state, circuit_state_to_string(victim->state), - victim->purpose); - TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out = 1; + /* Special checks for onion service circuits. */ + switch (victim->purpose) { + case CIRCUIT_PURPOSE_C_REND_READY: + /* We only want to spare a rend circ iff it has been specified in an + * INTRODUCE1 cell sent to a hidden service. */ + if (hs_circ_is_rend_sent_in_intro1(CONST_TO_ORIGIN_CIRCUIT(victim))) { continue; - default: - break; } - } - - /* If this is a service-side rendezvous circuit which is far - * enough along in connecting to its destination, consider sparing - * it. */ - if (!(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out) && - victim->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND) { - log_info(LD_CIRC,"Marking circ %u (state %d:%s, purpose %d) " - "as timed-out HS circ; relaunching rendezvous attempt.", + break; + case CIRCUIT_PURPOSE_S_CONNECT_REND: + /* The connection to the rendezvous point has timed out, close it and + * retry if possible. */ + log_info(LD_CIRC,"Rendezvous circ %u (state %d:%s, purpose %d) " + "as timed-out, closing it. Relaunching rendezvous attempt.", (unsigned)victim->n_circ_id, victim->state, circuit_state_to_string(victim->state), victim->purpose); - TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out = 1; - hs_circ_retry_service_rendezvous_point(TO_ORIGIN_CIRCUIT(victim)); - continue; + /* We'll close as a timeout the victim circuit. The rendezvous point + * won't keep both circuits, it only keeps the newest (for the same + * cookie). */ + break; + default: + break; } if (victim->n_chan) diff --git a/src/core/or/origin_circuit_st.h b/src/core/or/origin_circuit_st.h index 6c86a56000..73b971f72d 100644 --- a/src/core/or/origin_circuit_st.h +++ b/src/core/or/origin_circuit_st.h @@ -205,34 +205,10 @@ struct origin_circuit_t { * (in host byte order) for response comparison. */ uint32_t pathbias_probe_nonce; - /** Set iff this is a hidden-service circuit which has timed out - * according to our current circuit-build timeout, but which has - * been kept around because it might still succeed in connecting to - * its destination, and which is not a fully-connected rendezvous - * circuit. - * - * (We clear this flag for client-side rendezvous circuits when they - * are 'joined' to the other side's rendezvous circuit, so that - * connection_ap_handshake_attach_circuit can put client streams on - * the circuit. We also clear this flag for service-side rendezvous - * circuits when they are 'joined' to a client's rend circ, but only - * for symmetry with the client case. Client-side introduction - * circuits are closed when we get a joined rend circ, and - * service-side introduction circuits never have this flag set.) */ - unsigned int hs_circ_has_timed_out : 1; - /** Set iff this circuit has been given a relaxed timeout because * no circuits have opened. Used to prevent spamming logs. */ unsigned int relaxed_timeout : 1; - /** Set iff this is a service-side rendezvous circuit for which a - * new connection attempt has been launched. We consider launching - * a new service-side rend circ to a client when the previous one - * fails; now that we don't necessarily close a service-side rend - * circ when we launch a new one to the same client, this flag keeps - * us from launching two retries for the same failed rend circ. */ - unsigned int hs_service_side_rend_circ_has_been_relaunched : 1; - /** What commands were sent over this circuit that decremented the * RELAY_EARLY counter? This is for debugging task 878. */ uint8_t relay_early_commands[MAX_RELAY_EARLY_CELLS_PER_CIRCUIT]; diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index edb5e692e7..53855d40a9 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -126,11 +126,6 @@ finalize_rend_circuit(origin_circuit_t *circ, crypt_path_t *hop, hop->package_window = circuit_initial_package_window(); hop->deliver_window = CIRCWINDOW_START; - /* Now that this circuit has finished connecting to its destination, - * make sure circuit_get_open_circ_or_launch is willing to return it - * so we can actually use it. */ - circ->hs_circ_has_timed_out = 0; - /* If congestion control, transfer ccontrol onto the cpath. */ if (TO_CIRCUIT(circ)->ccontrol) { hop->ccontrol = TO_CIRCUIT(circ)->ccontrol; @@ -438,16 +433,6 @@ can_relaunch_service_rendezvous_point(const origin_circuit_t *circ) /* XXX: Retrying under certain condition. This is related to #22455. */ - /* Avoid to relaunch twice a circuit to the same rendezvous point at the - * same time. */ - if (circ->hs_service_side_rend_circ_has_been_relaunched) { - log_info(LD_REND, "Rendezvous circuit to %s has already been retried. " - "Skipping retry.", - safe_str_client( - extend_info_describe(circ->build_state->chosen_exit))); - goto disallow; - } - /* We check failure_count >= hs_get_service_max_rend_failures()-1 below, and * the -1 is because we increment the failure count for our current failure * *after* this clause. */ @@ -689,7 +674,7 @@ hs_circ_service_get_established_intro_circ(const hs_service_intro_point_t *ip) * - We've already retried this specific rendezvous circuit. */ void -hs_circ_retry_service_rendezvous_point(origin_circuit_t *circ) +hs_circ_retry_service_rendezvous_point(const origin_circuit_t *circ) { tor_assert(circ); tor_assert(TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND); @@ -699,10 +684,6 @@ hs_circ_retry_service_rendezvous_point(origin_circuit_t *circ) goto done; } - /* Flag the circuit that we are relaunching, to avoid to relaunch twice a - * circuit to the same rendezvous point at the same time. */ - circ->hs_service_side_rend_circ_has_been_relaunched = 1; - /* Legacy services don't have a hidden service ident. */ if (circ->hs_ident) { retry_service_rendezvous_point(circ); diff --git a/src/feature/hs/hs_circuit.h b/src/feature/hs/hs_circuit.h index 808e648951..afbff7b894 100644 --- a/src/feature/hs/hs_circuit.h +++ b/src/feature/hs/hs_circuit.h @@ -33,7 +33,7 @@ int hs_circ_launch_intro_point(hs_service_t *service, int hs_circ_launch_rendezvous_point(const hs_service_t *service, const curve25519_public_key_t *onion_key, const uint8_t *rendezvous_cookie); -void hs_circ_retry_service_rendezvous_point(origin_circuit_t *circ); +void hs_circ_retry_service_rendezvous_point(const origin_circuit_t *circ); origin_circuit_t *hs_circ_service_get_intro_circ( const hs_service_intro_point_t *ip); diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 716386408a..4e971233af 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -3675,6 +3675,9 @@ hs_service_circuit_cleanup_on_close(const circuit_t *circ) hs_metrics_close_established_rdv( &CONST_TO_ORIGIN_CIRCUIT(circ)->hs_ident->identity_pk); break; + case CIRCUIT_PURPOSE_S_CONNECT_REND: + hs_circ_retry_service_rendezvous_point(CONST_TO_ORIGIN_CIRCUIT(circ)); + break; default: break; }