Re: [tor-bugs] #25122 [Core Tor/Tor]: geoip: Hook the client geoip cache into the OOM handler

[Tor Bug Tracker & Wiki]

#25122: geoip: Hook the client geoip cache into the OOM handler
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  enhancement  | Status:  closed
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.3.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-relay, tor-oom, tor-dos, |  implemented
  029-backport, 031-backport, 032-backport   |  Actual Points:
Parent ID:   | Points:
 Reviewer:  nickm|Sponsor:
-+-
Changes (by nickm):

 * status:  needs_review => closed
 * resolution:   => implemented


Comment:

 This is now merged into dgoulet's ticket24902_029_05, and onto master for
 0.3.3. It should get merged to older versions along with the rest of
 ticket24902_029_05.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25122 [Core Tor/Tor]: geoip: Hook the client geoip cache into the OOM handler

[Tor Bug Tracker & Wiki]

#25122: geoip: Hook the client geoip cache into the OOM handler
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  enhancement  | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.3.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-relay, tor-oom, tor-dos, |  Actual Points:
  029-backport, 031-backport, 032-backport   |
Parent ID:   | Points:
 Reviewer:  nickm|Sponsor:
-+-

Comment (by dgoulet):

 033 branch: `ticket25122_033_01`

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25122 [Core Tor/Tor]: geoip: Hook the client geoip cache into the OOM handler

[Tor Bug Tracker & Wiki]

#25122: geoip: Hook the client geoip cache into the OOM handler
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  enhancement  | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.3.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-relay, tor-oom, tor-dos, |  Actual Points:
  029-backport, 031-backport, 032-backport   |
Parent ID:   | Points:
 Reviewer:  nickm|Sponsor:
-+-
Changes (by dgoulet):

 * status:  needs_revision => needs_review


Comment:

 See the two new commits along with the fixup from previous comment. First
 one adds the inc/dec functions for the cache size. Second commit adds a
 new() function for a client map entry.

 Branch: `ticket25122_029_01`

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25122 [Core Tor/Tor]: geoip: Hook the client geoip cache into the OOM handler

[Tor Bug Tracker & Wiki]

#25122: geoip: Hook the client geoip cache into the OOM handler
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  enhancement  | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.3.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-relay, tor-oom, tor-dos, |  Actual Points:
  029-backport, 031-backport, 032-backport   |
Parent ID:   | Points:
 Reviewer:  nickm|Sponsor:
-+-
Changes (by dgoulet):

 * status:  needs_review => needs_revision
 * reviewer:   => nickm


Comment:

 Replying to [comment:4 nickm]:
 > Looks good.  But could you take a few minutes to make sure that there is
 nowhere else in all the codebase, before or after applying #24902, that
 allocates or frees one of these objects?  And that the transport_name
 field never changes once the object is allocated?

 I really hope the `transport_name` doesn't change because it is used to
 compare entries in the HT if present. I'll confirm that.

 >
 > Possibly, we should make a _new() function to handle the allocate-and-
 count part of this function.  That could be a separate ticket, though.

 Yes agree, we need a new() there. This will be a bit bigger change to
 backport but should be fairly trivial though.

 >
 > Possibly, we should check to make sure that we never underflow on
 geoip_client_history_cache_size

 Hmmm, I'll work on inc/dev functions for that value that validates.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25122 [Core Tor/Tor]: geoip: Hook the client geoip cache into the OOM handler

[Tor Bug Tracker & Wiki]

#25122: geoip: Hook the client geoip cache into the OOM handler
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  enhancement  | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.3.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-relay, tor-oom, tor-dos, |  Actual Points:
  029-backport, 031-backport, 032-backport   |
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (by nickm):

 Looks good.  But could you take a few minutes to make sure that there is
 nowhere else in all the codebase, before or after applying #24902, that
 allocates or frees one of these objects?  And that the transport_name
 field never changes once the object is allocated?

 Possibly, we should make a _new() function to handle the allocate-and-
 count part of this function.  That could be a separate ticket, though.

 Possibly, we should check to make sure that we never underflow on
 geoip_client_history_cache_size

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25122 [Core Tor/Tor]: geoip: Hook the client geoip cache into the OOM handler

[Tor Bug Tracker & Wiki]

#25122: geoip: Hook the client geoip cache into the OOM handler
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  enhancement  | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.3.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-relay, tor-oom, tor-dos, |  Actual Points:
  029-backport, 031-backport, 032-backport   |
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (by dgoulet):

 Replying to [comment:2 nickm]:
 > I'm guessing that clientmap_entry_size() is something we'd extend once
 we have this and #24902 merged to the same branch?

 Don't think so because the added stuff in a `clientmap_entry_t` in #24902
 are not allocated so the `sizeof()` will pick them up.

 >
 > The geoip_client_cache_total_allocation() function needs to be much
 faster -- otherwise it will eat tons of CPU by walking the whole map every
 time cell_queues_check_size() is called -- and it is called from inside
 append_cell_to_circuit_queue().  How about having a static size_t that we
 increment when we allocate an entry, and decrement when we free it?

 Ah right, indeed. What about this fixup commit: `64e595e07c42a6f4` ?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25122 [Core Tor/Tor]: geoip: Hook the client geoip cache into the OOM handler

[Tor Bug Tracker & Wiki]

#25122: geoip: Hook the client geoip cache into the OOM handler
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  enhancement  | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.3.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-relay, tor-oom, tor-dos, |  Actual Points:
  029-backport, 031-backport, 032-backport   |
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (by nickm):

 I'm guessing that clientmap_entry_size() is something we'd extend once we
 have this and #24902 merged to the same branch?

 The geoip_client_cache_total_allocation() function needs to be much faster
 -- otherwise it will eat tons of CPU by walking the whole map every time
 cell_queues_check_size() is called -- and it is called from inside
 append_cell_to_circuit_queue().  How about having a static size_t that we
 increment when we allocate an entry, and decrement when we free it?

 Otherwise, looks plausible.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25122 [Core Tor/Tor]: geoip: Hook the client geoip cache into the OOM handler

[Tor Bug Tracker & Wiki]

#25122: geoip: Hook the client geoip cache into the OOM handler
-+-
 Reporter:  dgoulet  |  Owner:  dgoulet
 Type:  enhancement  | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.3.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-relay, tor-oom, tor-dos, |  Actual Points:
  029-backport, 031-backport, 032-backport   |
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-
Changes (by dgoulet):

 * status:  assigned => needs_review
 * keywords:  tor-relay, tor-oom, tor-dos => tor-relay, tor-oom, tor-dos,
 029-backport, 031-backport, 032-backport
 * type:  defect => enhancement


Comment:

 Because this is especially important for the DoS mitigation, I've made
 this based on 029 for which we should backport along with #24902.

 You'll notice I've picked some constant timings here for which I think it
 is reasonable but I'm happy to reconsider them.

 Branch: `ticket25122_029_01`

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs