#28780: circpadding: Add machine flag for not closing circuit if machine is active -------------------------------------------------+------------------------- Reporter: asn | Owner: (none) Type: defect | Status: | needs_revision Priority: Very High | Milestone: Tor: | 0.4.1.x-final Component: Core Tor/Tor | Version: Severity: Normal | Resolution: Keywords: wtf-pad, tor-relay, tor-cell, | Actual Points: 6 padding, 041-proposed, network-team- | roadmap-2019-Q1Q2 | Parent ID: #28634 | Points: 5 Reviewer: asn | Sponsor: | Sponsor2 -------------------------------------------------+------------------------- Changes (by nickm):
* status: merge_ready => needs_revision Comment: I want to talk about this approach more before we merge it. As I understand it, the idea here is that some circuits that would otherwise close should instead stay open longer, so that padding can be sent on them. This patch achieves that by adding a call inside `circuit_mark_for_close` to `circpad_circuit_should_be_marked_for_close`, which potentially overrides the decision to mark the circuit, and changes its purpose instead. Later, the circuit is supposed to get expired naturally when it times out. Here are some things that worry me about the logic: 1. Intercepting `circuit_mark_for_close()` is pretty risky IMO. It no longer 'does what it says on the label' and instead might keep the circuit open. There are over 100 callers to circuit_mark_for_close(); I think it will be hard to audit them all. 2. The function `circpad_circuit_should_be_marked_for_close()` is named as if it were a predicate function, but in fact it changes the circuit's purpose in some cases. (Also, it doesn't answer the question "should this circuit be marked for close"; it answers "can this circuit be allowed to close, given that somebody else wants to mark it.") 3. The function `circuit_expire_old_circuits_clientside()`, which we are relying on to kill of circuits eventually, uses `circuit_mark_for_close()` to close the circuits it wants to expire, and circuit_mark_for_close is now potentially overridden. Here's one possibility of we could do instead: 1. Rename the new purpose from C_CIRCUIT_PADDING to CIRCUIT_PADDING_SHUTDOWN to make it clear that the circuit's purpose is "sending padding until it dies." 2. Leave circuit_mark_for_close() untouched from the current codebase. Instead add a new function, `circuit_transition_to_shutdown()`. This function should either mark the circuit for close immediately by calling circuit_mark_for_close(), or should change the circuit's purpose to CIRCUIT_PADDING_SHUTDOWN. 3. Audit the codebase so that we change some calls from circuit_mark_for_close to circuit_transition_to_shutdown. We should only do this for calls that use REASON_FINISHED or REASON_NONE or REASON_IP_NOW_REDUNDANT today. How does that sound? -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/28780#comment:29> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs