Opened 15 months ago

Last modified 4 months ago

#24797 needs_revision enhancement

Add an option that makes Tor use fewer connections

Reported by: teor Owned by: neel
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay, tor-dos, 034-triage-20180328, 034-removed-20180328, 035-removed-20180711
Cc: neel@…, starlight@… Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor:

Description

Tor is willing all the file descriptors the OS tells it are available for the current user.
So to enforce a connection limit, operators need to drop the user file descriptor limit.
But if the OS is wrong, or there are multiple tor instances under the user, tor can easily exceed this limit.
Or machine hits a kernel, RAM, or CPU limit first, then the user might want to artificially limit connections.

Right now, we have ConnLimit, which looks like it limits connections, but it's actually a minimum.
And we have the out of socket check, but DisableOOSCheck is the default.

Does enforcing a connection limit require us to set DisableOOSCheck 0?
Then we should move this ticket to 0.3.4, and open one to improve the out of socket check.

Child Tickets

TicketTypeStatusOwnerSummary
#28367enhancementclosedRFE additional DOS mitigations for exits

Attachments (1)

b24797-001.patch (4.3 KB) - added by neel 11 months ago.
Patch (Revision 1)

Download all attachments as: .zip

Change History (24)

comment:1 Changed 14 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final
Type: defectenhancement

Label a bunch of (arguable and definite) enhancements as enhancements for 0.3.4.

comment:2 Changed 13 months ago by dgoulet

Keywords: tor-dos added; DDoS-resistance removed

comment:3 Changed 12 months ago by nickm

Keywords: 034-triage-20180328 added

comment:4 Changed 12 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:5 Changed 11 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:6 Changed 11 months ago by neel

Cc: neel@… added
Owner: set to neel
Status: newassigned

comment:7 Changed 11 months ago by neel

So, should I implement an option which sets the maximum number of connections (meaning Tor can't go over this)?

comment:8 Changed 11 months ago by arma

This one is tricky, because if we have such an option, and a Tor relay sets it and hits the limit, then the relay will start behaving badly.

We really need our relays to never hit their limit. Anything else is bad.

comment:9 in reply to:  8 Changed 11 months ago by neel

Replying to arma:

This one is tricky, because if we have such an option, and a Tor relay sets it and hits the limit, then the relay will start behaving badly.

We really need our relays to never hit their limit. Anything else is bad.

Thank You.

I noticed this function in src/common/compat.c:

int set_max_file_descriptors(rlim_t limit, int *max_out)

Should I be working with the function above? I am thinking about an argument to this function with the total maximum number of connections Tor will take (and using that value as an alternative to the maximum number of file descriptors) which will in turn be used to set max_sockets in src/common/compat.c and the ConnLimit_ option from the call to set_max_file_descriptors() in config.c. The argument will be based on an option I will add (if I work on this patch). If this option is 0 or left unset, it will default to using the number of file descriptors like Tor does right now.

comment:10 Changed 11 months ago by arma

Careful here! That function is about trying to move us to the highest maximum we can get from the system. It is not about having Tor limit the number that it will use. I think.

comment:11 Changed 11 months ago by teor

Don't change set_max_file_descriptors(), it works well, and does exactly what arma says: gets the most file descriptors from the system.

Instead, look at the out of socket check code. It is enabled by "DisableOOSCheck 0". It uses the socket limit that set_max_file_descriptors() discovers from the system. But you could add a torrc option that allows the user to set a lower limit for the out of socket check. It would only work if the out of socket check was enabled.

Then the tor process could have the maximum number of sockets available, but choose not to use them. (For example, during the extra client load, I would have set the socket limit to half the number of file descriptors, so that the connection closing logic ran less frequently.)

Since I opened this ticket, we also added DoSConnectionMaxConcurrentCount. Maybe that makes this ticket less important?
But it would still be good for operators who run all their tor instances under the same user.

comment:12 Changed 11 months ago by neel

I am still interested in this ticket.

I noticed the following options in connection_check_oos():

get_options()->ConnLimit_high_thresh
get_options()->ConnLimit_
get_options()->ConnLimit_low_thresh

It seems that the threshold is determined from these values. Should I modify these values directly should my proposed option (connection limit) be set, and calculates a threshold based on the set value rather than the number of file descriptors?

Or should I add new values to the or_options_t structure, fill them if my proposed option is set, and make connection_check_oos() use the new options instead if the proposed option is set.

I prefer the former as it is far simpler, since the latter adds some complexity but the latter is also less destructive.

Which approach do you prefer?

comment:13 in reply to:  12 Changed 11 months ago by teor

Replying to neel:

I am still interested in this ticket.

I noticed the following options in connection_check_oos():

get_options()->ConnLimit_high_thresh
get_options()->ConnLimit_
get_options()->ConnLimit_low_thresh

It seems that the threshold is determined from these values. Should I modify these values directly should my proposed option (connection limit) be set, and calculates a threshold based on the set value rather than the number of file descriptors?

If you add another torrc option MaxSockets, then use min(MaxSockets, ConnLimit_) instead of ConnLimit_, all the rest of the code should just work.

Using min(MaxSockets, ConnLimit_) allows us to override the OS when its limit is too high, and it makes sure we don't ever go over the OS limit.

comment:14 Changed 11 months ago by teor

You should replace ConnLimit_ before the thresholds are calculated.

Changed 11 months ago by neel

Attachment: b24797-001.patch added

Patch (Revision 1)

comment:15 Changed 11 months ago by neel

I have a patch for this. The filename is b24797-001.patch.

comment:16 Changed 11 months ago by teor

Status: assignedneeds_review

Hi,

The MaxSockets default in the man page is incorrect:

(Default: 1000)

It should say "Default: unlimited" or something similar.

I don't think we should fail if ConnLimit_ is lower than MaxSockets: why not just log a notice message, and use ConnLimit_?
MaxSockets is a maximum, not a minimum requirement.

Please update the options unit tests to test the new option, and its interaction with DisableOOSCheck.

comment:17 Changed 11 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.4.x-final
Status: needs_reviewneeds_revision

comment:18 in reply to:  16 ; Changed 11 months ago by arma

Replying to teor:

I don't think we should fail if ConnLimit_ is lower than MaxSockets: why not just log a notice message, and use ConnLimit_?

Use ConnLimit for what? As the min or as the max?

I'm still thinking we shouldn't add this feature. We periodically have people who want this sort of thing, and if they set it and it gets triggered, they are being a bad relay. Why are we offering them a way to be a bad relay?

comment:19 in reply to:  18 Changed 11 months ago by neel

Replying to arma:

I'm still thinking we shouldn't add this feature. We periodically have people who want this sort of thing, and if they set it and it gets triggered, they are being a bad relay. Why are we offering them a way to be a bad relay?

What do you mean by being a 'bad relay'? I am talking about a maximum number of sockets should a relay operator desire it (I am sorry if my patch does not make it too clear, it's my ADHD).

I am thinking that for people who run Tor relays on servers with other applications (web, email, etc.), or run multiple Tor instances, this could help with resource limitation without requiring virtualization (VMWare, Xen, etc.) or containerization (Docker, FreeBSD Jails, etc.). But I could understand your concern with this option if you could give an explanation.

comment:20 Changed 10 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

Move most needs_revision tickets from 0.3.4 to 0.3.5: we're about to hit the feature freeze.

If you really need to get this into 0.3.4, please drop me a line. :)

comment:21 Changed 8 months ago by nickm

Keywords: 035-removed-20180711 added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

Removing needs_revision tickets from 0.3.5 that seem to be stalled. Please move back if they are under active revision or discussion.

comment:22 in reply to:  18 Changed 4 months ago by teor

Replying to teor:

Hi,

The MaxSockets default in the man page is incorrect:

(Default: 1000)

It should say "Default: unlimited" or something similar.

I don't think we should fail if ConnLimit_ is lower than MaxSockets: why not just log a notice message, and use ConnLimit_?
MaxSockets is a maximum, not a minimum requirement.

Please update the options unit tests to test the new option, and its interaction with DisableOOSCheck.

Replying to arma:

Replying to teor:

I don't think we should fail if ConnLimit_ is lower than MaxSockets: why not just log a notice message, and use ConnLimit_?

Use ConnLimit for what? As the min or as the max?

ConnLimit_ (underscore) is the maximum number of file descriptors discovered from the OS. Tor doesn't go above this limit: it is a maximum.

ConnLimit (no underscore) is the minimum required value of ConnLimit_ for tor to start.

So I suggest replacing ConnLimit_ with min(MaxSockets, ConnLimit_):

If you add another torrc option MaxSockets, then use min(MaxSockets, ConnLimit_) instead of ConnLimit_, all the rest of the code should just work.

Using min(MaxSockets, ConnLimit_) allows us to override the OS when its limit is too high, and it makes sure we don't ever go over the OS limit.

I'm still thinking we shouldn't add this feature. We periodically have people who want this sort of thing, and if they set it and it gets triggered, they are being a bad relay. Why are we offering them a way to be a bad relay?

Because they're about to hit their limit anyway, and they want to fail slightly less awfully using DisableOOSCheck 0.

Some people want to be able to access their relays after tor hits its socket limit (#28367). And as neel said, others can't set ulimit on their machines.

comment:23 Changed 4 months ago by starlight

Cc: starlight@… added
Note: See TracTickets for help on using tickets.