Opened 3 years ago

Closed 2 months ago

#18642 closed enhancement (implemented)

Teach the OOM handler about the DNS cache

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dos, oom, tor-relay, dns, 035-triaged-in-20180711
Cc: neel@… Actual Points:
Parent ID: Points: 1
Reviewer: nickm Sponsor: SponsorV-can

Description


Child Tickets

Change History (37)

comment:1 Changed 3 years ago by nickm

Component: - Select a componentTor
Type: defectenhancement

comment:2 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 3 years ago by nickm

Keywords: tor-dos added

comment:4 Changed 2 years ago by isabela

Points: small1

comment:5 Changed 2 years ago by nickm

Parent ID: #17293

comment:6 Changed 2 years ago by nickm

Owner: nickm deleted
Status: acceptedassigned

by no means sure that I can get to these.

comment:7 Changed 2 years ago by nickm

Status: assignednew

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

comment:8 Changed 2 years ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:9 Changed 2 years ago by nickm

Sponsor: SponsorU-can

comment:10 Changed 2 years ago by nickm

Parent ID: #17293

Unparenting these from #17293; holding for future work.

comment:11 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:12 Changed 23 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

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

comment:13 Changed 17 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:14 Changed 16 months ago by nickm

Keywords: oom tor-relay dns added

comment:15 Changed 16 months ago by nickm

Sponsor: SponsorV-can

comment:16 Changed 4 months ago by neel

Cc: neel@… added

comment:17 Changed 3 months ago by neel

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.

comment:18 Changed 3 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: newneeds_review

comment:19 Changed 3 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:20 Changed 3 months ago by dgoulet

Reviewer: dgoulet

comment:21 Changed 3 months ago by dgoulet

Status: needs_reviewneeds_revision

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.

comment:22 Changed 3 months ago by neel

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?

Last edited 3 months ago by neel (previous) (diff)

comment:23 in reply to:  22 ; Changed 3 months ago by 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 ;).

comment:24 Changed 3 months ago by neel

OK thanks for the clarification.

comment:26 Changed 3 months ago by neel

Status: needs_revisionneeds_review

comment:27 in reply to:  23 Changed 3 months ago by teor

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.

comment:28 Changed 3 months ago by dgoulet

Status: needs_reviewneeds_revision

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 (#25899).

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!

Last edited 3 months ago by dgoulet (previous) (diff)

comment:30 Changed 3 months ago by neel

Status: needs_revisionneeds_review

comment:31 Changed 3 months ago by neel

Reviewer: dgoulet

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

comment:32 Changed 3 months ago by neel

Reviewer: dgoulet

comment:33 Changed 3 months ago by teor

Reviewer: dgoulet

comment:34 Changed 3 months ago by asn

Reviewer: nickm

comment:35 Changed 2 months ago by nickm

Status: needs_reviewneeds_revision

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

comment:36 Changed 2 months ago by neel

In the PR, I added the requested line

comment:37 Changed 2 months ago by nickm

Resolution: implemented
Status: needs_revisionclosed

Looks good; merging!

Note: See TracTickets for help on using tickets.