[tor-bugs] #6465 [Tor Relay]: Build abstraction layer around TLS

[Tor Bug Tracker & Wiki]

#6465: Build abstraction layer around TLS
-----------------------+----------------------------------------------------
 Reporter:  andrea     |          Owner:  andrea            
     Type:  project    |         Status:  needs_review      
 Priority:  major      |      Milestone:  Tor: 0.2.4.x-final
Component:  Tor Relay  |        Version:  Tor: unspecified  
 Keywords:             |         Parent:                    
   Points:             |   Actualpoints:                    
-----------------------+----------------------------------------------------

Comment(by andrea):

 Responses to part 2 (points in potential need of further discussion):

 > Another general thing: I'm not done reading yet, but I'm a little
 nervous
 > about queues and buffers here. As I understand it, there will be 5
 layers
 > where cells get queued:
 >
 >   The circuit's queue of cells
 >   The channel's queue of cells [*]
 >   The or_connection_t's queue of data to encrypt (conn->outbuf)
 >   SSL's write buffer. (ssl->wbuf)
 >   The kernel's write buffer.
 >
 > The one marked with [*] above is new; what does it give us that
 conn->outbuf
 > doesn't? And what are we doing to make sure it doesn't get too big? We
 don't
 > want it getting large, since we want to use the circuit pqueue mechanism
 to
 > choose an active circuit late. Filling buffers aggressively here means
 that
 > loud circuits fill the queue, and quiet low-latency circuits don't get
 heard.

 Well, under most circumstances the channel's outgoing cell queue can't get
 too large because if the channel is in the OPEN state and write_cell
 succeeds
 then it can go straight on down.  The normal path is for the lower layer
 to
 decide it wants more cells, call channel_flush_some_cells(), which drains
 the
 outgoing queue if any, and then calls
 channel_flush_from_first_active_circuit()
 to do the ewma circuit selection thing and get more.  The only way for
 cells
 to get into the queue other than by request from the lower layer like that
 is a
 few places where they get queued for events that only happen once per
 channel
 lifetime or once per circuit lifetime, mostly calls to
 channel_send_destroy()
 from various places.

 I wouldn't totally mind finding a way to simplify this queueing structure,
 though.

 > Crazy idea: How badly, if at all, would we break things by making the
 > channel_s definition internal so that only channel.c (and later others
 if
 > needed) was allowed to look at it? It's not how we do most stuff right
 now,
 > but I bet it would make us happier in the long run.

 I'd like that better and tried to do it that way originally, and had to
 move
 it because channel_t had to carry all the circuit queue stuff
 or_connection_t
 used to and the circuit code reaches into that in a bazillion places.  It
 might get a lot easier to deal with it after I refactor for 6816 though.

 > I'm confused about why channel_send_destroy() is here, but there aren't
 other
 > channel_send_foo() functions.

 Because that's the only one that gets called from things that happen above
 the
 channel layer.  There are still a bunch of connection_or_send_foo()
 functions,
 but they only get used when handshaking in channeltls.c.

 > In channel_process_cells, I feel like maybe we're going to hit trouble
 if
 > there isn't a way to break out in the middle of processing all those
 cells?
 > I guess that might be happening in the middle of the processing
 functions,
 > where they note if there's an error and don't process anything if an
 error
 > has occurred.

 The latter; the cell handlers that calls are just the old code path in
 command.c, less the handshaking stuff that went to channeltls.c.

 > In channel_send_destroy, I am confused about why we're calling
 > channel_write_cell instead of channel_queue_cell. In fact, it's likely
 > that I'm going to have this confusion about every channel_write_cell/
 > channel_queue_cell instance. Hmm. I wonder if there's any way we can
 make
 > those more distinct/obvious/safe?

 Hmm, how would you feel about s/channel_write_cell/channel_send_cell/g and
 s/channel_queue_cell/channel_recv_cell/g ?

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6465#comment:28>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online