Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#7802 closed enhancement (implemented)

Remove PathBias use of timestamp_dirty and record usage accounting

Reported by: mikeperry Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: MikePerry201301
Cc: nickm Actual Points: 15
Parent ID: #7755 Points:
Reviewer: Sponsor:

Description

Instead of using timestamp_dirty as a signal for "circuit use" in the pathbias code, we should create a dedicated path_state_t state (PATH_STATE_USE_ATTEMPTED).

It's also debatable as to if we should *remove* the additional timestamp_dirty markings I added in #7157. The most controversial addition was marking freshly cannibalized circuits as dirty. While this seems to make sense to me (do we really want to use cannibalized circuits for any random stream?), it may have subtle impacts on the proper use of GENERAL purpose circuits that get cannibalized during predictive circuit building or other cases involving hidden services.

Child Tickets

Change History (10)

comment:1 Changed 6 years ago by mikeperry

Parent ID: #7755

comment:2 Changed 6 years ago by mikeperry

Summary: Remove PathBias use of timestamp_dirtyRemove PathBias use of timestamp_dirty and record usage accounting

When I do this, I think I should also record circuit usage attempt counts in the state file. This means adding another field to the EntryGuardPathBias state file line to say how many circuit usage attempts there were for each guard..

Further down the line, depending on what sort of results we see in the field, we then can set thresholds on these values.

The reason to do this is because the adversary might get a bonus by allowing circuits to build that go unused while only failing the usage attempts. Since I suspect most normal clients build way more circuits than they actually try to use, this difference might be substantial.

comment:3 Changed 6 years ago by mikeperry

Keywords: MikePerry201301 added; MikePerry201212 removed

comment:4 Changed 6 years ago by arma

How much of this ticket is "fix path selection (or stream allocation) behavior", and how much is "make our path bias detection stuff cleaner"?

Assuming PathBiasDisableRate (if I have my names correct, #7982) is disabled in 0.2.4, it seems like the latter isn't as critical as the former, yes? (Since Nick is getting stressed about fitting everything in)

comment:5 Changed 6 years ago by mikeperry

Actual Points: 12
Status: newneeds_review

arma: mikeperry/bug7802 contains both.

I refactored the stream accounting stuff to record separate stream usage statistics; improved the path state machine and added some safeguards to state transitions and record keeping; eliminated reliance on timestamp_dirty; and fixed a couple of edge cases I found during testing that also apply to the previous code.

I've again put the branch through several thousand hidden service client and server streams, wget fetches, DNS resolves, and unreachable host connect attempts during the course of this development, and am still stress testing it now.

comment:6 Changed 6 years ago by mikeperry

Actual Points: 1215

If you've pulled this branch already, I may have committed the mortal sin of rebasing it since your pull. I accidentally pushed a commit that should have been an auto-squash, which I corrected by rebasing. I won't do that again.

The commit hash you're looking for is b810d322bfc55d202dbbd2e8ebe4529cf0778c8b, but I may toss some more autosquash commits on top of that if I notice any more issues.

comment:7 Changed 6 years ago by nickm

Is there any subset of these that could be done for a minimal version? This is a 1300 line diff and a 1700 line patch series. It's a lot of changed code.

If not, what is the most rigorous, specific explanation of what this code is supposed to do?

comment:8 in reply to:  7 Changed 6 years ago by mikeperry

Replying to nickm:

Is there any subset of these that could be done for a minimal version? This is a 1300 line diff and a 1700 line patch series. It's a lot of changed code.

Most of the changes are refactoring that should both improve readability and future-proof the state machine against bitrot.

If not, what is the most rigorous, specific explanation of what this code is supposed to do?

The only substantial functional change is to provide separate tunable limits for the acceptable rate of circuit use failure before we emit log messages. A circuit use failure is defined as a built circuit for which any streams detach and that we are also unable to probe successfully.

The separate use failure accounting is actually limited to changes to just a couple of functions: pathbias_check_use_rate(), and writing two counter variables to the state file as EntryGuardPathUseBias.

All of the other circuit use state transitions and probing code were already there. I just cleaned things up to remove dependence upon timestamp_dirty and improve/guard the state machine transitions, and refactor some common code into helper functions.

#7804 is my ticket to document all of the path bias stuff in path-spec.txt.

comment:9 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Just when you're not looking, git can rear up and bite you.

This branch got merged, squash! commits and all, when andrea was trying to merge the #8024 fix alone. So we had to decide whether to revert or whether to call it "good enough for now".

We just did a littleW pretty thorough emergency post-merge review, and stuff looks basically okay. Closing this and #8024; Andrea is opening a new ticket with the issues we found during review.

comment:10 in reply to:  9 Changed 6 years ago by arma

Replying to nickm:

Andrea is opening a new ticket with the issues we found during review.

For the record, this was #8081.

Note: See TracTickets for help on using tickets.