net: Don't pretend the proto qlock is thread safe
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 15 Dec 2016 20:34:13 +0000 (15:34 -0500)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 10 Jan 2017 00:01:39 +0000 (19:01 -0500)
commit8f73842c8fcdc44eed0a81544180f959f2bb8a7f
tree84d55d6195b41d346305c2c1b79382abf769acd1
parent585c5e3abada75036ecba63993e2563b5030314c
net: Don't pretend the proto qlock is thread safe

This one is a disaster.  AFAIK, the protocol qlock does nothing to protect
lookups from the IP hash table during input processing (either a receive
function or an advise function).

This commit merely documents this and removes the lock from protecting the
lookup (note that ipht has spinlocks internally).  I left the protocol
locks in places where it seems they might need them (TCP, around the limbo
funcs, UDP around Fsnewcall).

Here's the issue: the lifetime of conversations isn't managed properly.

Conversations are loosely refcounted, (not quite a kref), and only from
higher in the networking stack (grep closeconv).  When the last ref is
closed, then the protocol's close() happens.

At the same time, conversations are stored in per-protocol hash tables
(grep ipht).  This allows the lower half of the networking stack to find
the conversation when a packet arrives.  There is no refcount involved.
The convs are removed from the ipht during the protocol's close().

During packet recv, e.g. tcpiput, we have this:

    /* lock protocol while searching for a conversation */
    qlock(&tcp->qlock);

    /* Look for a matching conversation */
    s = iphtlook(&tpriv->ht, source, seg.source, dest, seg.dest);

which later unlocks.

    qlock(&s->qlock);
    qunlock(&tcp->qlock);

It looks like the qlock is protecting something, like the conv from
disappearing.  The "lock the conv before unlocking TCP" style implies that.
Note that in closeconv(), you have to lock the conv before decreffing too.

Here's the thing: that tcp->qlock doesn't appear to be held during any
closeconv() call - it's actually rarely held.  (limborexmit(), tcpiput(),
tcpadvise(), the latter of which does some form of conv lookup too, and
some places from devip.c).  closeconv() calls the protocol's close, which
removes the conv from the ipht.  It never locks the *proto's* qlock.

Here's a scenario:
- A conversation is set up in the ipht.  One core starts the packet
  reception, and finds the conv ('s') in the ipht.
- Meanwhile, another core calls closeconv(), then cleans up the conv,
  then calls tcpclose, and *then* removes it from the ipht.

So now we have a pointer to a conv that was closed, but not quite
freed.  Plan 9 does this nasty stuff where the convs are never freed -
just zombied until they are reused.  However, the stack actually relies
on them always being conversations.  You couldn't use a slab allocator
here properly (btw, this is partly why we have O(nr_tcp_convs)
operations at various points in the stack, and why conversations always
exist in /net/tcp/).

The way the code works now, the inbound TCP code (the one that got the
conv from the ipht) will qlock the conv, then see the state is closed,
then send a reset.

Seems ok, right?  But not if another TCP conv was created in the interim.
If the inbound code was delayed enough and a new conv was created, that new
conv would actually be the old one!  This is because convs are never really
freed and the network stack plays fast-and-loose with pointers to old
memory.

The worst case?  An inbound packet to the old conv (with its old port) gets
put in the receive queue of the new conv (with any port).  The ports don't
matter - this is after the port-to-conv lookup has happened.

The qlock(tcp->qlock) does nothing to prevent this, and as far as I can
tell, it just gives us the illusion of being thread-safe.  There may be one
'saving grace', and that is that our inbound network stack currently driven
by etherread4, and is single threaded.  Though that won't help if you
(technically) get connections on different mediums (or v6 and v4).  Also,
it's a rare race, and would require, at least for TCP, for the original
conv to be Closed, which might take time or otherwise block on etherread4()
making progress.

Solutions?  There's a lot of really nasty things involved: the weird
usage of the kref pattern, the lack of a slab allocator (which it'd be
nice to use), dealing with ipgen and the array of convs (there are
probably long-reaching assumptions about convs never going away, which
gets nasty at gen time).

A proper fix would probably involve rewriting things such that the
hashtable is a source for references, possibly with kref_get_not_zero.
That'll require all successful lookups from ipht to decref when they
are done, and a careful rewrite of closeconv() and Fsprotoclone() (which
gets a new conv).  We would *not* decref / make usable the conversation
until the protocol says it's done.  (see the nasty inuse checks in
Fsprotoclone).

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/src/net/tcp.c
kern/src/net/udp.c