Opened 6 years ago

Closed 6 years ago

#7440 closed defect (implemented)

Path Bias code should count circuit destructions as failures

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

Description

Right now, the path bias code counts any circuit that succeeds as a success. However, this is incorrect. Circuits can still be tagged *after* they succeed, by embedding the tag in a RELAY_BEGIN or RELAY_RESOLVE.

Clients should therefore decrement a guard's success count and increment the failure count for any circuit that is destroyed after successful construction, but hasn't successfully carried any streams.

Child Tickets

Change History (6)

comment:1 Changed 6 years ago by mikeperry

If we're also concerned about timing-based tagging using stall-failures, we might also consider counting circuits that are never able to succeed a stream within the CircuitStreamTimeout value as failures as well.

comment:2 in reply to:  1 Changed 6 years ago by mikeperry

Replying to mikeperry:

If we're also concerned about timing-based tagging using stall-failures, we might also consider counting circuits that are never able to succeed a stream within the CircuitStreamTimeout value as failures as well.

Bleh, a quick look at connection_edge_process_relay_cell makes me think there may even be ways to tag a relay cell such that the exit simply drops it, making what would normally be a destructive tag potentially appear as a CircuitStreamTimeout, so I think we want to do this anyways. :/

comment:3 Changed 6 years ago by mikeperry

My plan for this is to use either circ->isolation_any_streams_attached or circ->timestamp_dirty in combination with a new field to represent stream success (something like circ->got_stream_reply) to do this check.

If a successfully opened circuit has had any stream attempts but none that succeeded (circ->isolation_any_streams_attached && !circ->got_stream_reply) in circuit_mark_for_close(), then we subtract a success and add a failure for that circuit for its guard. We'll want to store the number of circs this happens on in a separate guard PathBias field, for debugging.

Technically, this check should cover any attempts to either choke a circuit or destroy it once streams are attached.

However, in case there are non-destructive tagging mechanisms that allow the circuit to survive construction (such as reply-only tags, perhaps?) but allow it to be destroyed *before* any streams are attempted, we should keep a separate count for circuits closed prematurely due to any remote reason. I think this can also be done in circuit_mark_for_close(). We should also keep a separate tally of this count for informational purposes.

comment:4 Changed 6 years ago by mikeperry

Actual Points: 14
Keywords: MikePerry201212 added; MikePerry201211 removed

comment:5 Changed 6 years ago by mikeperry

Actual Points: 1423
Status: newneeds_review

This is fixed in mikepery/209-path-bias-changes. See #7157 for review info.

comment:6 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

I just merged 209-path-bias-changes; please reopen if there's more to do here.

Note: See TracTickets for help on using tickets.