Opened 6 years ago

Closed 6 years ago

#11396 closed enhancement (implemented)

Detect maximum memory at runtime to allow lower default than 8GB

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay, oom, 025-triaged, andrea-review-0254
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The default value for MaxMemInQueues is 8 GB, which means that for many users, there simply won't be any support for OOM prevention.

I think we can do better than that by detecting the physical memory and picking a default MaxMemInQueues based on a percentage of that. Of course, the user should still be able to override that value.

Child Tickets

Change History (10)

comment:1 Changed 6 years ago by nickm

Status: newneeds_review
Type: defectenhancement

I've started a branch for this in "bug11396". We should decide whether it's 0.2.5-worthy. So far I've tested it on OSX and Linux.

comment:2 Changed 6 years ago by arma

Perhaps counterintuitively, so long as we get Linux sorted out, the others don't matter as much. That's because the main anonymity attacks involve knocking over many big relays, and if half the relays resist, your attack doesn't get very far.

But that said, we shouldn't do this ticket simply because of a particular anonymity attack that is made possible by this design flaw. We should do the ticket so relays can't be knocked over (or worse, be used to knock over other innocent processes if you run one).

comment:3 Changed 6 years ago by andrea

Keywords: 025-triaged added

I think I'm in favor of keeping this for 0.2.5, since OOM makes potential DoS against relays and since we always have the option of disabling support for querying the system memory per platform if it turns out unreliable. Adding 025-triaged.

comment:4 Changed 6 years ago by andrea

Begin code review:

7f36ab4bebd6b36622d30eb56a0816e869ba5db0:

  • I think the code for determining the memory size is correct, at least, but we should also test it on Windows and the *BSDs.
  • Slight concern about caching the value; it is possible for it to change at runtime [1] and the cached value never expires and there's no manual way to force a recheck here AFAICT.

c2d2d53b4195077ce83bf18e165da3db9ef4be7c:

  • Does changing the value in options from options_validate() mean if we write out the config, we'll lose track of it having originally been unspecified and emit a computed value instead?
  • Should we support saying MaxMemInQueues <nn>% as well as MaxMemInQueues <n> GB perhaps?

1a8299df65461ddde7034ff18ad2dfdbb6bf25ac:

  • Yes I like this version with CLOEXEC better :)

End code review

[1] Most typically in a VM, but systems with physically hotpluggable memory do exist. Running into OOM conditions on a relay running in a VM and then growing its memory allocation seems like a perfectly ordinary and reasonable thing for a relay admin to do.

comment:5 Changed 6 years ago by nickm

Okay, I think I've fixed nearly all the above issues in the current bug11396 branch.

I didn't add the <n>% percent thing, though: Let's see whether anybody wants that first.

Better now?

comment:6 Changed 6 years ago by nickm

Keywords: andrea-review-0254 added

Drop owners from needs_review tickets in tor 0.2.5

comment:7 in reply to:  5 Changed 6 years ago by andrea

Replying to nickm:

Okay, I think I've fixed nearly all the above issues in the current bug11396 branch.

I didn't add the <n>% percent thing, though: Let's see whether anybody wants that first.

Better now?

The bug11396 branch in your repository is just the three commits I already reviewed; where can I find the revised version?

comment:8 Changed 6 years ago by nickm

Ug. Branch "bug11396_v2" in my repo now has the stuff I worked on before. It doesn't have 1a8299df65461ddde7034ff18ad2dfdbb6bf25ac though; we'll need to cherry-pick it.

(Apparently I had two branches that diverged and I didn't notice it.)

comment:9 Changed 6 years ago by andrea

Begin code review:

  • fff77d89d781dbb219c426b86055c31d3984ddd7 looks good to me
  • 1d4d991a67c03f47e26036b93842f1508c3cf660 looks fine
  • 32e27ae64bc796b64bd864765ce42bdc27c60281 lools okay

Okay, I think this is ready to go if you feel confident in it. Be sure to cherrypick 1a8299df65461ddde7034ff18ad2dfdbb6bf25ac before merging.

comment:10 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

okay, done and merged to master . Thanks!

Note: See TracTickets for help on using tickets.