Opened 7 years ago

Closed 7 years ago

#7341 closed defect (fixed)

circuit_expire_building applies CBT in twisted ways on hidden service circuits

Reported by: mikeperry Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client, MikePerry201212
Cc: nickm, arma, karsten Actual Points: 25
Parent ID: Points:
Reviewer: Sponsor:

Description

There is something wrong with how circuit_expire_building() applies timeouts to hidden services. There's some rather funky switch statements in there that grant hidden services a pass from timeout at certain stages in their handshaking, but not others. However, the end result seems to be that hidden service timeouts are actually *more* common than they should be for some reason. See my comments in #7157 for slightly more detail (though not much).

Child Tickets

Change History (18)

comment:1 Changed 7 years ago by mikeperry

These circuits appear to be mostly purpose 6 (CIRCUIT_PURPOSE_C_INTRODUCING), with a few purpose 5 (CIRCUIT_PURPOSE_C_GENERAL) and 9 (CIRCUIT_PURPOSE_C_ESTABLISH_REND), and a tiny fraction of measurement timeouts.

comment:2 Changed 7 years ago by mikeperry

Looking deeper into this, those first two purposes (6 and 5) so far all appear to have has_opened=1. This appears to cause circuit_expire_building to select the cannibalized_cutoff for them, which is *half* of the current circuit build timeout. For the record, they also have desired_path_len=4, but since has_opened=1, they don't get assinged the fourhop_cutoff timeout.

Purpose 9 has the same has_opened -> cannibalized problem, but only has a desired_path_len of 3.

I am tempted to simply remove the has_opened -> cannibalized timeout differentiation, and simply use the desired_path_len for cutoff selection.

comment:3 in reply to:  2 ; Changed 7 years ago by mikeperry

Cc: nickm arma karsten added

Replying to mikeperry:

I am tempted to simply remove the has_opened -> cannibalized timeout differentiation, and simply use the desired_path_len for cutoff selection.

Wow, after running this fix for just a few minutes, and I'm now seeing desired_path_len's of 5 for purpose 6. Is that a different bug, or do we also need a fivehop_cutoff for hidden services circuits for some reason?

I'm going to let this run for a few hours and see what the final counts look like.

Meanwhile, if anyone knows why hidden service client circuits with a purpose of CIRCUIT_PURPOSE_C_INTRODUCING would have a desired_path_len of 5 in normal operation, that would be useful info.

comment:4 in reply to:  3 Changed 7 years ago by rransom

Replying to mikeperry:

Replying to mikeperry:

I am tempted to simply remove the has_opened -> cannibalized timeout differentiation, and simply use the desired_path_len for cutoff selection.

Wow, after running this fix for just a few minutes, and I'm now seeing desired_path_len's of 5 for purpose 6. Is that a different bug, or do we also need a fivehop_cutoff for hidden services circuits for some reason?

This means that those client-side intro circuits reached the introduction point, sent an INTRODUCE1 cell, and then decided that they should re-introduce to the HS at some other intro point (I suspect that means the client's _C_REND_READY or _C_REND_READY_INTRO_ACKED circuit timed out, but I haven't looked at that code in a while, and it's a mess).

#1297 might be worth reading if you haven't found it already.

I'm going to let this run for a few hours and see what the final counts look like.

Meanwhile, if anyone knows why hidden service client circuits with a purpose of CIRCUIT_PURPOSE_C_INTRODUCING would have a desired_path_len of 5 in normal operation, that would be useful info.

https://gitweb.torproject.org/tor.git/blob/tor-0.2.4.6-alpha:/src/or/rendclient.c#l91

comment:5 Changed 7 years ago by rransom

A log of the Tor client's STREAM, CIRC, and CIRC_MINOR events would be useful in figuring out why more circuits are timing out.

comment:6 Changed 7 years ago by mikeperry

Some notes of interest from the event logs rransom suggested:

  1. The cannibalization situation is even worse than the inappropriate short-timeout application I noted above. If a circuit is cannibalized, we never update the creation timer. This means that if a circuit lies idle for any amount of time, it will instantly time out when it is cannibalized.
  1. There is a full second local delay between the point where a HS intro circuit completes and the point where we attempt to send INTRODUCE1. Possibly due to a 1/sec libevent loop that drives HS creation? That would be sad.

comment:7 in reply to:  6 ; Changed 7 years ago by rransom

Replying to mikeperry:

Some notes of interest from the event logs rransom suggested:

  1. The cannibalization situation is even worse than the inappropriate short-timeout application I noted above. If a circuit is cannibalized, we never update the creation timer. This means that if a circuit lies idle for any amount of time, it will instantly time out when it is cannibalized.

No.

  1. There is a full second local delay between the point where a HS intro circuit completes and the point where we attempt to send INTRODUCE1. Possibly due to a 1/sec libevent loop that drives HS creation? That would be sad.

Yes.

comment:8 Changed 7 years ago by nickm

Keywords: tor-client added
Milestone: Tor: 0.2.4.x-final

comment:9 in reply to:  7 ; Changed 7 years ago by mikeperry

Replying to rransom:

Replying to mikeperry:

Some notes of interest from the event logs rransom suggested:

  1. The cannibalization situation is even worse than the inappropriate short-timeout application I noted above. If a circuit is cannibalized, we never update the creation timer. This means that if a circuit lies idle for any amount of time, it will instantly time out when it is cannibalized.

No.

What about rend_client_reextend_intro_circuit()? It appears to call circuit_extend_to_new_exit() without a path to circuit_launch_by_extend_info() or a timestamp update.

comment:10 in reply to:  9 Changed 7 years ago by rransom

Replying to mikeperry:

Replying to rransom:

Replying to mikeperry:

Some notes of interest from the event logs rransom suggested:

  1. The cannibalization situation is even worse than the inappropriate short-timeout application I noted above. If a circuit is cannibalized, we never update the creation timer. This means that if a circuit lies idle for any amount of time, it will instantly time out when it is cannibalized.

No.

What about rend_client_reextend_intro_circuit()? It appears to call circuit_extend_to_new_exit() without a path to circuit_launch_by_extend_info() or a timestamp update.

Good point, but the problem there is that rend_client_reextend_intro_circuit is only re-extending the circuit, not cannibalizing it properly. The ‘actually cannibalize circuit foo for purpose bar’ code should be split out of circuit_launch_by_extend_info into a separate function. (Open a separate ticket for this change.)

comment:11 Changed 7 years ago by mikeperry

Ok, I think the cannibalize fixes have managed to cut down the CIRCUIT_PURPOSE_C_GENERAL timeouts to a reasonably small number. I'm still pondering what to do with CIRCUIT_PURPOSE_C_INTRODUCING and CIRCUIT_PURPOSE_C_ESTABLISH_REND. I currently have two ideas:

  1. Flag them with hs_circ_has_timed_out and launch a new circuit in parallel, and then use the first INDTRODUCE_ACK one to get back to us. This seems like it will work especially well for CIRCUIT_PURPOSE_C_INTRODUCING, but I'm not sure it will be great for CIRCUIT_PURPOSE_C_ESTABLISH_REND, given that we'd need to reuse the rend cookie and/or restart the whole introduction handshake too.
  1. Simply declare the circuit purpose(s) exempt from timeouts once the circuit was already built to its full length. It might make sense to apply the CircuitStreamTimeout instead of messing around with CBT for them, since they are more like end-to-end RELAY cells anyway. I think I like this idea for CIRCUIT_PURPOSE_C_ESTABLISH_REND, due to the cookie issues above.

So I think I'm going to try to hack up approach 1 for CIRCUIT_PURPOSE_C_INTRODUCING and approach 2 for CIRCUIT_PURPOSE_C_ESTABLISH_REND. Anyone object?

If anyone knows any potential pitfalls with launching CIRCUIT_PURPOSE_C_INTRODUCING requests in parallel, or wants to help me out by pointing me at the regions of the code I'd need to hack to do this, I'd appreciate that, too.

I also need to task switch to W3C stuff, so you guys have till at least Wednesday to mull it over.

comment:12 Changed 7 years ago by nickm

rransom said:

Good point, but the problem there is that rend_client_reextend_intro_circuit is only re-extending the circuit, not cannibalizing it properly. The ‘actually cannibalize circuit foo for purpose bar’ code should be split out of circuit_launch_by_extend_info into a separate function. (Open a separate ticket for this change.)

Mike, could you do this? I'd do it myself, but I think you've grokked the issues better here.

comment:13 in reply to:  11 Changed 7 years ago by nickm

Replying to mikeperry:

  1. Flag them with hs_circ_has_timed_out and launch a new circuit in parallel, and then use the first INDTRODUCE_ACK one to get back to us. This seems like it will work especially well for CIRCUIT_PURPOSE_C_INTRODUCING, but I'm not sure it will be great for CIRCUIT_PURPOSE_C_ESTABLISH_REND, given that we'd need to reuse the rend cookie and/or restart the whole introduction handshake too.

Yeah, we really shouldn't be reusing rend cookies.

  1. Simply declare the circuit purpose(s) exempt from timeouts once the circuit was already built to its full length. It might make sense to apply the CircuitStreamTimeout instead of messing around with CBT for them, since they are more like end-to-end RELAY cells anyway. I think I like this idea for CIRCUIT_PURPOSE_C_ESTABLISH_REND, due to the cookie issues above.

That seems like an okay idea. It's not perfect, but it's an improvement.

So I think I'm going to try to hack up approach 1 for CIRCUIT_PURPOSE_C_INTRODUCING and approach 2 for CIRCUIT_PURPOSE_C_ESTABLISH_REND. Anyone object?

Seems like it's worth trying; I say go for it and see how it works.

(We definitely should be

If anyone knows any potential pitfalls with launching CIRCUIT_PURPOSE_C_INTRODUCING requests in parallel, or wants to help me out by pointing me at the regions of the code I'd need to hack to do this, I'd appreciate that, too.

What exact behavior are you proposing here? *Always* launch in parallel seems iffy; "Launch in parallel on timeout" seems plausible, but we need to keep an eye on it.

comment:14 Changed 7 years ago by mikeperry

Actual Points: 16
Keywords: MikePerry201212 added

comment:15 Changed 7 years ago by mikeperry

Actual Points: 1625
Status: newneeds_review

This is fixed in the single commit at the tip of mikeperry/bug7341. Note that that branch is based off of mikeperry/209-path-bias-changes, since I tested them in combination.

See the changes file for a summary, but it's basically what we agreed on. I implemented "Launch in parallel on timeout" for the intro case.

Incidentally, Roger had already coded the "launch timed out intro circuits in parallel" change back in 2008, but something prevented his code from exercising. I ripped his code out, and consolidated it in circuit_expire_building() and rend_client_introduction_acked(), which is a much better place for it.

comment:16 Changed 7 years ago by mikeperry

The "refactor the cannibalize code" ticket is #7660.

comment:17 Changed 7 years ago by mikeperry

Nick's review comments for this branch are in https://trac.torproject.org/projects/tor/ticket/7691#comment:5 along with my reply.

comment:18 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay, I merged and closed the branch for #7691. I hope there wasn't any extra code for 7341 that didn't make it into that branch?

Note: See TracTickets for help on using tickets.