Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#1795 closed enhancement (fixed)

Prop 174: Optimistic Data for Tor: Server Side

Reported by: nickm Owned by: iang
Priority: Medium Milestone: Deliverable-Mar2011
Component: Core Tor/Tor Version:
Severity: Keywords: prop174 tor-relay
Cc: iang@… Actual Points:
Parent ID: #1849 Points:
Reviewer: Sponsor:

Description

Attached is Ian's patch to implement the server side of proposal 174. The goal is to accept and queue DATA cells received at an exit when the connection is not yet opened.

Notes from Ian:

The current code actually correctly handles queued data at the Exit; if there is queued data in a EXIT_CONN_STATE_CONNECTING stream, that data will be immediately sent when the connection succeeds. If the connection fails, the data will be correctly ignored and freed. The problem with the current server code is that the server currently drops DATA cells on streams in the EXIT_CONN_STATE_CONNECTING state. Also, if you try to queue data in the EXIT_CONN_STATE_RESOLVING state, bad things happen because streams in that state don't yet have conn->write_event set, and so some existing sanity checks (any stream with queued data is at least potentially writable) are no longer sound.

The solution is to simply not drop received DATA cells while in the EXIT_CONN_STATE_CONNECTING state. Also do not send SENDME cells in this state, so that the OP cannot send more than one window's worth of data to be queued at the Exit. Finally, patch the sanity checks so that streams in the EXIT_CONN_STATE_RESOLVING state that have buffered data can pass.

[...] Here is a simple patch. It seems to work with both regular streams and hidden services, but there may be other corner cases I'm not aware of. (Do streams used for directory fetches, hidden services, etc. take a different code path?)

Child Tickets

Attachments (2)

prop174_v1.diff (3.7 KB) - added by nickm 9 years ago.
Initial implementation of proposal 174
optimistic-server_2a.diff (5.3 KB) - added by iang 9 years ago.

Download all attachments as: .zip

Change History (35)

Changed 9 years ago by nickm

Attachment: prop174_v1.diff added

Initial implementation of proposal 174

comment:1 Changed 9 years ago by nickm

Status: newneeds_review

comment:2 Changed 9 years ago by iang

Cc: iang@… added

comment:3 Changed 9 years ago by nickm

So, piece by piece, initial thoughts.

connection.c:

The check for conn->state seems wrong, since the connection state's values are relative to the connection's type. Only if conn->type ==CONN_TYPE_EXIT is it meaningful to say "conn->state == EXIT_CONN_STATE_anything".

On the other hand, if what we're really concerned about is the possible absence of conn->write_event, we could as easily check for conn->write_event rather than the state.

The same note applies to the second check for conn->state in the tor_assert below.

relay.c:

Rather than special-casing the code that called connection_edge_process_relay_cell_not_open(), could we not move the new handling for DATA cells *into* connection_edge_process_relay_cell_not_open() ? Or would that make stuff ugly? (Again, you need to check conn->type before conn->state is meaningful)

' log_warn(domain, "Optimistic data received."); ' is debugging code.

I worry that making the consider_sending_sendme() calls conditional might put us in a position where they could never be called. If a client exhausts the window of a sendme cell before the resolve and connect are done, will a sendme eventually be sent? If so, what calls it?

comment:4 Changed 9 years ago by iang

You're totally right about checking for CONN_TYPE_EXIT, of course. I must have been confused by a quick glance at or.h and seeing that the EXIT and AP states were disjoint, and not noticing that the others are not.

Checking for write_event in chunk 1 is also a plausible solution; in chunk 2, I think the state check (suitably corrected to also check for CONN_TYPE_EXIT) is the right choice.

I don't think we want to move code into connection_edge_process_relay_cell_not_open(), since the patch causes control to skip that call, and fall through to the RELAY_COMMAND_DATA handling below. All that stuff would have to be copied into connection_edge_process_relay_cell_not_open(), which seems poor.

Agreed on the debugging code; sorry that slipped in.

Isn't it the case that when the cells get *sent* to the server, it will trigger the check to see if SENDMEs should be sent? SENDMEs are sent when the buffer empties, not when it fills, right? I expect that not wrapping these calls would have the same (i.e. no) effect, but it seems cleaner to me to not check if you should send a SENDME if you know you won't.

comment:5 Changed 9 years ago by nickm

Parent ID: #1849

comment:6 Changed 9 years ago by arma

Milestone: Deliverable-Mar2011

comment:7 Changed 9 years ago by iang

Who's got the conch here? Should I make a new version of the patch, reflecting the above discussion?

comment:8 Changed 9 years ago by nickm

Yes, you've got the hacking-stick for now, unless you want to formally relinquish it.

Stuff has been a bit quiet here recently since we're crazy-focused on September-30 deliverables and getting 0.2.2.x into an rc state.

comment:9 Changed 9 years ago by nickm

FWIW, we are no longer in codefreeze: 0.2.2.x is its own branch, and master is again ready for new stuff.

comment:10 Changed 9 years ago by nickm

Owner: set to iang
Status: needs_reviewassigned

Kicking this out of needs-review for now; it needs revision.

Changed 9 years ago by iang

Attachment: optimistic-server_2a.diff added

comment:11 Changed 9 years ago by iang

OK, here's a new version of the patch, tested against 0.2.3.

comment:12 Changed 9 years ago by iang

Status: assignedneeds_review

comment:13 Changed 9 years ago by iang

Who's got the reviewing stick? Is it Nick?

comment:14 Changed 9 years ago by nickm

Yup. I am buried in stuff now, though, so if anybody else has time to look at this too, that would rock.

comment:15 Changed 8 years ago by nickm

Okay. Hm. In the meantime, merging the bufferevent stuff has made it so that checking conn->write_event doesn't make sense for bufferevent-based connections. I'll need to look into that and figure out the right approach. Further, if a bufferevent-based connection is in STATE_RESOLVING, I am not sure that it actually _has_ a bufferevent object to queue the outgoing data on yet. We should check that out.

A question making the circuit_consider_sending_sendme() code conditional on "!optimistic_data": suppose that the client sends enough optimistic data for the server to exhaust its window, then the client stops sending data before the connection is complete. In this case, will the server ever send the client SENDME cell?

comment:16 Changed 8 years ago by iang

Won't the SENDME get sent once the exit starts actually forwarding the data on to the server?

comment:17 Changed 8 years ago by nickm

What part of the code does that? I'm willing to believe you, but the logic is abit convoluted for me. :/

comment:18 Changed 8 years ago by iang

That's not part of this code. Isn't that just the normal behaviour of SENDMEs? When a certain number (100 for circuits, 50 for streams, is it?) of cells are sent from the exit to the server, the exit sends a SENDME back to the client asking for more?

comment:19 Changed 8 years ago by nickm

My point is that i see this patch making some SENDMEs conditional: these sendmes are sent from the exit towards the client in connection_edge_process_relay_cell when the exit receives a relay_data cell. Your patch changes that, so that these cells are not sent if the relay_data cell that would have triggered them is an opportunistic cell.

So are the two affected calls to circuit_consider_sending_sendme and connection_edge_consider_sending_sendme redundant, or no? If they're redundant, we should maybe think about removing them. If they're not, then this patch introduces a redundant bug. If they're only sometimes redundant but sometimes needed, we should analyze better.

comment:20 Changed 8 years ago by iang

No, it's my bad; I stand corrected. The conditionals should just be reverted (i.e. removed), I'm pretty sure.

comment:21 Changed 8 years ago by nickm

Well, I'm not 100% sure there. Is it okay to give the client a sendme on a stream that isn't open yet? Will the client handle that properly?

We might need code to leave (one of?) those conditionals in, and have code to defer the sendme(s) until the stream is open and the data is sent.

comment:22 Changed 8 years ago by nickm

ping? any progress here?

comment:23 Changed 8 years ago by iang

I thought you were checking things vis-a-vis bufferevent?

comment:24 Changed 8 years ago by nickm

I thought we were still trying to figure out right solution for the sendme issues.

comment:25 Changed 8 years ago by iang

Ah, sorry. Yes, I agree with "We might need code to leave (one of?) those conditionals in, and have code to defer the sendme(s) until the stream is open and the data is sent."

Sending the circuit SENDME is fine (we don't even need to make it conditional); we should not send the stream SENDME, and we should just call connection_edge_consider_sending_sendme() after the stream is connected.

But I know nothing at all about the bufferevent stuff.

comment:26 Changed 8 years ago by nickm

So the right fixes, fwict, are:

  • Make the circuit_consider_sending_sendme unconditional again. The circuit-level sendme shouldn't depend on the stream at all.
  • The stream-level connection_edge_consider_sending_sendme() should probably actually move completely: it belongs in a new "connection_edge_flushed_some()". We shouldn't actually be sending relay sendmes just because we're queueing cells on the outbuf! But that's another bug.
  • The connection_start_writing() check should probably be more like:
   IF_HAS_BUFFEREVENT(conn, { 
       connection_start_writing
   }) ELSE_IF_NO_BUFFEREVENT {
       if (conn->write_event)
           connection_start_writing
   }

I'm going to grab the patch stick on this stuff now. Should have a patch for you rsn.

comment:27 Changed 8 years ago by nickm

I've moved the stream-level connection_edge_consider_sending_sendme() issue to #2756, since that's a bug independent of this code.

comment:28 Changed 8 years ago by nickm

See branch "optimistic server" in my public repository:

  • I left the connection_start_writing alone, since it turns out that code is only hit after a previous IF_HAS_BUFFEREVENT check.
  • I made the circuit sendmes unconditional.
  • I added a changes file.
  • For patches on the stream-level sendme, see #2756.

When I merge, I must remember that there is a spec patch needs to get put in the torspec repository (since we split the proposals and specs into their own repo).

Let me know what you think?

comment:29 Changed 8 years ago by nickm

err, the branch name is optimistic_server . Underscore, not space.

comment:30 Changed 8 years ago by arma

See the optimistic_server branch in my git for a whitespace cleanup commit.

The overall patch looks plausible. I had looked at it a few months ago and identified the same issue Nick patched ("if you send 1000 cells to a circuit optimistically, it will never send a circuit-level sendme back"), but my patch was different -- I had thought to call circuit_consider_sending_sendme() from inside connection_edge_finished_flushing() (and I guess also from inside connection_edge_flushed_some() now that #2756 exists).

The benefit of the approach Nick took is that we can have optimistic data queued for an unbounded number of not-yet-connected streams before any of them connects. The benefit of my approach is that we can't. But that said, you can queue a bunch of data cells at exit relays in plenty of other ways, such as by having actual open streams that never flush, so my "benefit" isn't really much use.

Fine by me to merge.

comment:31 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay, merging the patch to master, and applied the tor-spec.txt changes. Closing at last. Thanks again, everybody!

comment:32 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:33 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.