Teach the OOM handler about the DNS cache

changed milestone to %Tor: 0.3.5.x-final in legacy/trac

Trac:
Type: defect to enhancement
Component: - Select a component to Tor

Trac:
Owner: N/A to nickm
Status: new to accepted

Trac:
Keywords: N/A deleted, tor-dos added

Trac:
Points: small to 1

Trac:
Parent: N/A to legacy/trac#17293 (moved)

by no means sure that I can get to these.

Trac:
Status: accepted to assigned
Owner: nickm to N/A

Put all unowned "assigned" tickets back into "new".

Trac:
Status: assigned to new

Trac:
Milestone: Tor: 0.2.9.x-final to Tor: 0.2.???

Trac:
Sponsor: SponsorU-can to N/A

Unparenting these from legacy/trac#17293 (moved); holding for future work.

Trac:
Parent: legacy/trac#17293 (moved) to N/A

Milestone renamed

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.3.???

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

Trac:
Keywords: N/A deleted, tor-03-unspecified-201612 added
Milestone: Tor: 0.3.??? to Tor: unspecified

Remove an old triaging keyword.

Trac:
Keywords: tor-03-unspecified-201612 deleted, N/A added

Trac:
Keywords: tor-dos deleted, tor-dos oom tor-relay dns added

Trac:
Sponsor: N/A to SponsorV-can

Trac:
Cc: N/A to neel@neelc.org

My PR is available here: https://github.com/torproject/tor/pull/223

What I do for the OOM handler in this PR is that I clear the cache with purge_expired_resolves() with now as the current time, and increase the interval by an hour until either we cleared enough memory or there are no more entries.

Trac:
Status: new to needs_review
Milestone: Tor: unspecified to Tor: 0.3.5.x-final

Trac:
Keywords: tor-dos oom tor-relay dns deleted, oom, tor-dos, 035-triaged-in-20180711, tor-relay, dns added

Trac:
Reviewer: N/A to dgoulet

One comment to validate which could be an issue.

Also, there is a potential argument to only run that OOM handler if you are an Exit like options->ExitRelay.

Trac:
Status: needs_review to needs_revision

Thank you so much for your feedback.

My question is, what is that argument going to be called? If there is no name yet, should I create it?

If I have to make something, I am thinking about something like: OOMHandlerClearOnlyDNS (0/1) where 0 is the default (run all OOM checks) and 1 is to only run OOM on DNS (if you're an exit).

UPDATE: Also, should I allow this option for all relays or only exits?

Replying to neel:

If I have to make something, I am thinking about something like: OOMHandlerClearOnlyDNS (0/1) where 0 is the default (run all OOM checks) and 1 is to only run OOM on DNS (if you're an exit).

Wait no, no need for that at all. What I was saying is that we should only run that OOM handler in the case tor is running as an Exit and one way to look at that is with ExitRelay 1 global option ;).

OK thanks for the clarification.

My new PR is here: https://github.com/torproject/tor/pull/235

Trac:
Status: needs_revision to needs_review

Replying to dgoulet:

Replying to neel:

If I have to make something, I am thinking about something like: OOMHandlerClearOnlyDNS (0/1) where 0 is the default (run all OOM checks) and 1 is to only run OOM on DNS (if you're an exit).

Wait no, no need for that at all. What I was saying is that we should only run that OOM handler in the case tor is running as an Exit and one way to look at that is with ExitRelay 1 global option ;).

That's not how ExitRelay works:

ExitRelay 0|1|auto

Tells Tor whether to run as an exit relay. If Tor is running as a non-bridge server, and ExitRelay is set to 1, then Tor allows traffic to exit according to the ExitPolicy option (or the default ExitPolicy if none is specified).

If ExitRelay is set to 0, no traffic is allowed to exit, and the ExitPolicy option is ignored.

If ExitRelay is set to "auto", then Tor behaves as if it were set to 1, but warns the user if this would cause traffic to exit. In a future version, the default value will be 0. (Default: auto)

https://www.torproject.org/docs/tor-manual.html.en

Instead, try something like:

smartlist_t *exit_policy = router_get_my_routerinfo()->exit_policy;
if (!policy_is_reject_star(exit_policy, AF_INET) || !policy_is_reject_star(exit_policy, AF_INET6)) {
  /* Run the OOM handler on DNS */
}

But that won't handle OOM when the operator has turned exiting off, but used to have it on. So instead, maybe we should:

always run the OOM handler on DNS
check the approximate size of the DNS cache before running the OOM handler on it, or
set a flag when we run the OOM handler and we're not an exit, then clear the flag when we become an exit.

True, ExitRelay won't work. I'm also not very enthusiastic on using policy_is_reject_star() which possibly goes over the entire policy just to learn if we are an Exit relay with a non reject policy.

The Roles we've added recently for the callbacks in tor should be what we look at imo, but we don't have one for the Exit just yet (legacy/trac#25899 (moved)).

So for now, lets always run it, checking the HT size is very cheap and the OOM is only triggered on memory pressure so shouldn't be that often in most cases.

Sorry neel, the PR looks good, just remove the ExitRelay condition :P. Thanks!

Trac:
Status: needs_review to needs_revision

New PR is here: https://github.com/torproject/tor/pull/239

Trac:
Status: needs_revision to needs_review

dgoulet told me that he's on vacation until the 16th. Would someone else be willing to review this patch?

Trac:
Reviewer: dgoulet to N/A

Trac:
Reviewer: N/A to dgoulet

Trac:
Reviewer: dgoulet to N/A

Trac:
Reviewer: N/A to nickm

Hi! There's a fix I think we need in the second commit. Other than that, stuff looks fine here.

Trac:
Status: needs_review to needs_revision

In the PR, I added the requested line

Looks good; merging!

Trac:
Status: needs_revision to closed
Resolution: N/A to implemented

closed

changed time estimate to 8h

moved from legacy/trac#18642 (moved)

added Feature label and removed 1 deleted label

added DNS Relay + 1 deleted label and removed 1 deleted label

added DoS label and removed 1 deleted label

Teach the OOM handler about the DNS cache

Child items ...

Activity