Opened 8 months ago

Closed 8 months ago

#25122 closed enhancement (implemented)

geoip: Hook the client geoip cache into the OOM handler

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay, tor-oom, tor-dos, 029-backport, 031-backport, 032-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

The geoip cache can possibly grow quite a bit so it would be wise to make our OOM aware of it.

Child Tickets

Change History (8)

comment:1 Changed 8 months ago by dgoulet

Keywords: 029-backport 031-backport 032-backport added
Status: assignedneeds_review
Type: defectenhancement

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

comment:2 Changed 8 months ago 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.

comment:3 in reply to:  2 Changed 8 months ago by dgoulet

Replying to 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 ?

comment:4 Changed 8 months ago 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

comment:5 in reply to:  4 Changed 8 months ago by dgoulet

Reviewer: nickm
Status: needs_reviewneeds_revision

Replying to 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.

comment:6 Changed 8 months ago by dgoulet

Status: needs_revisionneeds_review

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

comment:7 Changed 8 months ago by dgoulet

033 branch: ticket25122_033_01

comment:8 Changed 8 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

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.

Note: See TracTickets for help on using tickets.