Merge remote-tracking branch 'tor-gitlab/mr/638'

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.

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,
+    close_cutoff, extremely_old_cutoff,
-    cannibalized_cutoff, c_intro_cutoff, s_intro_cutoff, stream_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)
+    else if (victim->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND) {
-      cutoff = stream_cutoff;
+      /* 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
+    /* Special checks for onion service circuits. */
-     * connecting to its destination, and we haven't already flagged it as
+    switch (victim->purpose) {
-     * 'timed out', flag it so we'll launch another intro or rend circ, but
+    case CIRCUIT_PURPOSE_C_REND_READY:
-     * don't mark it for close yet.
+      /* We only want to spare a rend circ iff it has been specified in an
-     *
+       * INTRODUCE1 cell sent to a hidden service. */
-     * (Circs flagged as 'timed out' are given a much longer timeout
+      if (hs_circ_is_rend_sent_in_intro1(CONST_TO_ORIGIN_CIRCUIT(victim))) {
-     * 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;
         continue;
-      default:
-        break;
       }
-    }
+      break;
-
+    case CIRCUIT_PURPOSE_S_CONNECT_REND:
-    /* If this is a service-side rendezvous circuit which is far
+      /* The connection to the rendezvous point has timed out, close it and
-     * enough along in connecting to its destination, consider sparing
+       * retry if possible. */
-     * it. */
+      log_info(LD_CIRC,"Rendezvous circ %u (state %d:%s, purpose %d) "
-    if (!(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out) &&
+               "as timed-out, closing it. Relaunching rendezvous attempt.",
-        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.",
                (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;
+      /* We'll close as a timeout the victim circuit. The rendezvous point
-      hs_circ_retry_service_rendezvous_point(TO_ORIGIN_CIRCUIT(victim));
+       * won't keep both circuits, it only keeps the newest (for the same
-      continue;
+       * cookie). */
+      break;
+    default:
+      break;
     }
 
     if (victim->n_chan)

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];

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);

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);

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;
   }