Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#2559 closed enhancement (fixed)

connection buckets get not decremented in a private tor network

Reported by: cagara Owned by: cagara
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In Line 2112 of connection.c:

When we are handling a read/write operation from a connection, we have to decrement the connetion read/write bucket to track down bandwidth usage and make limiting functionable.

This line calles "connection_is_rate_limited()" to verify if we should decrement the bucket or not.

In Line 1895 of connection.c:
Here we check if we have such unlimited connection.
Unfortunately tor thinks that private nets are unlimited too, which should not be the case: Only linked connection should be free of limitation. Otherwise we would have problems viewing used Bandwidth Rate using Controlsocket or ARM in private tor setups.

Child Tickets

Attachments (2)

my_patch.diff (2.9 KB) - added by cagara 9 years ago.
Patch for solution with extra parameter called "CountPrivateBandwidth"
my_patch.2.diff (2.9 KB) - added by cagara 9 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by nickm

Component: - Select a componentTor Relay

comment:2 Changed 9 years ago by nickm

Note:

The rationale for exempting private networks from connection buckets from rate limiting is that people usually want to use rate limits to limit how much they use over a slow or expensive upstream link.

But of course, as you note, this is only "usually". Under some circumstances (like a testing network) you want to limit internal addresses too.

So the right solution IMO is to have a configuration option to alter the behavior of connection_is_rate_limited(). This could either be a simple flag, or another address policy.

comment:3 Changed 9 years ago by arma

Right, I'm ok with either of these solutions. I think the address policy would provide more flexibility, and could be just as fast a check as now in the case where you didn't set a policy.

I wonder if there are going to be side effects though, e.g. where the Tor client writing to a local socks connection counts those bytes as part of its rate limiting.

I suspect the answer is yes -- meaning each researcher will have a different reason for wanting this feature disabled, and will have different expectations about what the resulting behavior ought to be.

That argues for making it a simple flag rather than a policy. And for putting a warning on its use ("you get to keep both pieces").

comment:4 Changed 9 years ago by cagara

Owner: set to cagara
Status: newaccepted

Changed 9 years ago by cagara

Attachment: my_patch.diff added

Patch for solution with extra parameter called "CountPrivateBandwidth"

comment:5 Changed 9 years ago by cagara

Status: acceptedneeds_review

I have added a patch introducing a new parameter settable via torrc.
"CountPrivateBandwidth" will allow the user to make traffic from private addresses influence on the packaging window.

I would appreciate a code review, and hope that I could give something back to the community.

comment:6 Changed 9 years ago by nickm

Milestone: Tor: 0.2.3.x-final
Type: defectenhancement

I think this is correct. I've put your patch, plus another patch to make some tweaks and cleanups, in a new branch called "bug2559" in my public git repository. Does it look okay to you?

Also, when we credit the patch in Git, it wants a name and an email. Is "cagara" the name you want to use, and should we include an email address, or just use a dummy address like nobody@…?

comment:7 Changed 9 years ago by cagara

Thanks for reviewing! "cagara" is okay, my email address is Daniel.Cagara@…

comment:8 in reply to:  7 Changed 9 years ago by cagara

Replying to cagara:

Thanks for reviewing! "cagara" is okay, my email address is Daniel.Cagara@…

Daniel.Cagara (at) uni-duesseldorf.de

comment:9 Changed 9 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Thanks! Fixing the credits and merging into the master branch; this code will go into 0.2.3.1-alpha.

comment:10 Changed 9 years ago by arma

Resolution: implemented
Status: closedreopened

comment:11 Changed 9 years ago by arma

I hate boolean logic, but isn't the logic here backwards?

If it's linked, return 0: not rate limited

If it's localhost && CountPrivateBandwidth, return 0: not rate limited

If it's localhost && !CountPrivateBandwidth, return 1, rate limited

Line 2 and Line 3 are the reverse of what they ought to be.

comment:12 Changed 9 years ago by Sebastian

Yes, I think you're right.

  else if (options->CountPrivateBandwidth &&

could be replaced by

  else if (!options->CountPrivateBandwidth &&

to fix it.

Changed 9 years ago by cagara

Attachment: my_patch.2.diff added

comment:13 Changed 9 years ago by cagara

Status: reopenedneeds_review

You're absolutely right, Sebastian! Thank you for your commitment.

I have modified the attached patch.

comment:14 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Negation added.

comment:15 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:16 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.