Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 29204: Inspect circuit queues before sending padding. #719

Closed
wants to merge 2 commits into from

Conversation

Labels
None yet
Projects
None yet
4 participants
@mikeperry-tor
Copy link
Contributor

@mikeperry-tor mikeperry-tor commented Feb 21, 2019

Mitigates OOM conditions at relays.

"Callback: Sending padding to non-origin circuit.");
relay_send_command_from_edge(0, mi->on_circ, RELAY_COMMAND_DROP, NULL,
0, NULL);
if (TO_OR_CIRCUIT(circ)->p_chan_cells.n <= circpad_max_circ_queued_cells) {
Copy link
Contributor

@dgoulet-tor dgoulet-tor Feb 21, 2019

I don't think this will work as expected. I think the problem is more on the outbuf side. If none of the cells go on the wire but we keep queuing padding cells directly in the outbuf, we'll ultimately fill it.

The above will keep sending those cells as long as the circuit queue has not too many cells but that won't work because the bloating in this case happens at the outbuf level.

Furthermore, the circuit cell queues already have a max check (in append_cell_to_circuit_queue()):

  if (PREDICT_UNLIKELY(queue->n >= max_circuit_cell_queue_size)) {

I think to fix this, we need a way to ask the channel "can you write on the wire?" and if no, we do not queue anything and if yes, we check if the circuit queue and outbuf have stuff. If yes, I'm guessing do not send padding... (Not entirely sure about this because in between traffic pattern you might want to send padding especially for quiet circuit even though everything is still in the outbuf?)

Then you might want to couple the above with a "last cell emitted timestamp" to have some max threshold when you definitely stop?...

Also, the TROVE we just got in tor today makes it that KIST now considers the outbuf size in order to make a decision if there is enough space to flush cells from the circuit queue.

If the circuit padding subsystem does send the padding cell through the circuit queue, then we might be fine here. If it writes it directly into the outbuf, then problem.

@coveralls
Copy link

@coveralls coveralls commented Feb 21, 2019

Pull Request Test Coverage Report for Build 4113

  • 5 of 6 (83.33%) changed or added relevant lines in 1 file are covered.
  • 2241 unchanged lines in 19 files lost coverage.
  • Overall coverage increased (+0.009%) to 61.717%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/core/or/circuitpadding.c 5 6 83.33%
Files with Coverage Reduction New Missed Lines %
src/core/crypto/hs_ntor.c 3 97.83%
src/lib/crypt_ops/crypto_digest.c 4 97.94%
src/lib/crypt_ops/crypto_rand_fast.c 6 87.01%
src/feature/rend/rendcache.c 6 93.02%
src/feature/dirauth/voteflags.c 10 35.68%
src/lib/evloop/workqueue.c 11 93.33%
src/lib/crypt_ops/crypto_init.c 11 80.36%
src/feature/hs/hs_control.c 19 66.67%
src/feature/hs/hs_common.c 30 83.44%
src/feature/relay/router.c 31 40.06%
Totals Coverage Status
Change from base Build 3880: 0.009%
Covered Lines: 45345
Relevant Lines: 73472

💛 - Coveralls

Remove stray flag update. (not clear it is needed anyway).
@torproject-pusher torproject-pusher deleted the branch torproject:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment