Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#2434 closed defect (fixed)

Don't disable dirport when accounting limit can't be reached due to bwrate

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

Description

A lot of people set accounting options just for the statistics, not to actually enforce a bandwidth cap (they do that via bandwidthrate). We shouldn't disable their dirports when they do that.

(Of course, the real bug is that you don't get the statistics without setting accounting options, but that's unrelated IMO.)

Child Tickets

Change History (13)

comment:1 Changed 8 years ago by Sebastian

(I should point people at bug #871 here, too. A few things are discussed there. I think this proposal is a bit different tho)

comment:2 Changed 8 years ago by atagar

Is this still relevant with the implementation of #2345? The accounting is sometimes set to provide the traffic usage, which that option provides. Cheers! -Damian

comment:3 Changed 8 years ago by Sebastian

Milestone: Tor: 0.2.3.x-final
Status: newneeds_review

I think it might still be relevant, not sure what other features accountingmax provides, especially since you need a controller for #2345.

Implementation of this in branch bug2434 in my repository.

comment:4 Changed 8 years ago by arma

Looks reasonable.

My first minor suggestion would be to put a comment in the code as to why we have that extra case, and a short comment in the changes file saying what that lets you do.

Second minor suggestion would be to switch the accounting_set_wakeup_time() function to use your new accounting_get_interval_length() abstraction rather than its time_in_interval variable.

The broader suggestion would be, if we want an alternate more explicit way to do this (rather than relying on a hack), to have another torrc option (e.g. AccountingTrack) which explicitly says to do all the accounting stuff but not for hibernating. I don't think this change is a big enough deal to matter either way though.

comment:5 Changed 8 years ago by nickm

Looks okay to me too; I concur with roger's first two recommendations, and suggest that we defer the third "till later".

I am not totally comfortable with the new calculation in decide_to_advertise_dirport: get_effective_bwrate() returns uint32_t, and accounting_get_interval_length returns an int. On platforms where int is 32-bit, their product can easily overflow the result type.

What are our options here? First option is to use a 64-bit type. If the longest interval we support is a month, then we'll be okay until somebody has get_effective_bwrate() returning more than about 6.8 terabytes per second. Second option is to use division, and check whether get_effective_bwrate(options) >= options->AccountingMax / accounting_get_interval_length().

comment:6 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

I'm calling this 0.2.4.x, unless you think the revisions will happen by Friday. It's worth doing, but it can certainly wait.

comment:7 Changed 8 years ago by Sebastian

I rebased the branch onto master because it didn't merge cleanly anymore, and added a fixup commit to change the identified problems. I don't think a new torrc option is warranted, especially because we have way too many already, and I think this feature only means that a few people will not be confused where they'd otherwise have been confused.

comment:8 Changed 8 years ago by Sebastian

Milestone: Tor: 0.2.4.x-finalTor: 0.2.3.x-final

Oh, changing the milestone also. Please change it back if you'd rather wait with the merge.

comment:9 Changed 8 years ago by nickm

Keywords: small-feature added

I'm a little worried about a case where accounting_get_interval_time() returns 0 and we divide by it. Can we add a defensive programming check for that? Also, let's maybe log the values here at log_info severity, so we can track down any infelicities in the equation.

comment:10 Changed 8 years ago by Sebastian

added another fixup commit. Did you mean something like that?

comment:11 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Yes, just so. Squashing & merging & closing.

comment:12 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:13 Changed 7 years ago by nickm

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