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)
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

index 14c5b23..83a1817 100644 (file)
@@ -2040,20 +2040,20 @@ void tcpiput(struct Proto *tcp, struct Ipifc *unused, struct block *bp)
                }
        }
 
-       /* 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);
        if (s == NULL) {
                netlog(f, Logtcp, "iphtlook failed\n");
 reset:
-               qunlock(&tcp->qlock);
                sndrst(tcp, source, dest, length, &seg, version, "no conversation");
                freeblist(bp);
                return;
        }
 
+       /* lock protocol for unstate Plan 9 invariants.  funcs like limbo or
+        * incoming might rely on it. */
+       qlock(&tcp->qlock);
+
        /* if it's a listener, look for the right flags and get a new conv */
        tcb = (Tcpctl *) s->ptcl;
        if (tcb->state == Listen) {
@@ -2077,8 +2077,10 @@ reset:
                 *  return it in state Syn_received
                 */
                s = tcpincoming(s, &seg, source, dest, version);
-               if (s == NULL)
+               if (s == NULL) {
+                       qunlock(&tcp->qlock);
                        goto reset;
+               }
        }
 
        /* The rest of the input state machine is run with the control block
@@ -3062,7 +3064,6 @@ void tcpadvise(struct Proto *tcp, struct block *bp, char *msg)
        }
 
        /* Look for a connection */
-       qlock(&tcp->qlock);
        for (p = tcp->conv; *p; p++) {
                s = *p;
                tcb = (Tcpctl *) s->ptcl;
@@ -3072,7 +3073,6 @@ void tcpadvise(struct Proto *tcp, struct block *bp, char *msg)
                                        if (ipcmp(s->raddr, dest) == 0)
                                                if (ipcmp(s->laddr, source) == 0) {
                                                        qlock(&s->qlock);
-                                                       qunlock(&tcp->qlock);
                                                        switch (tcb->state) {
                                                                case Syn_sent:
                                                                        localclose(s, msg);
@@ -3083,7 +3083,6 @@ void tcpadvise(struct Proto *tcp, struct block *bp, char *msg)
                                                        return;
                                                }
        }
-       qunlock(&tcp->qlock);
        freeblist(bp);
 }
 
index fc12762..19ddfc1 100644 (file)
@@ -446,13 +446,10 @@ void udpiput(struct Proto *udp, struct Ipifc *ifc, struct block *bp)
                        return; /* to avoid a warning */
        }
 
-       qlock(&udp->qlock);
-
        c = iphtlook(&upriv->ht, raddr, rport, laddr, lport);
        if (c == NULL) {
                /* no converstation found */
                upriv->ustats.udpNoPorts++;
-               qunlock(&udp->qlock);
                netlog(f, Logudp, "udp: no conv %I!%d -> %I!%d\n", raddr, rport,
                           laddr, lport);
 
@@ -487,9 +484,10 @@ void udpiput(struct Proto *udp, struct Ipifc *ifc, struct block *bp)
                                                panic("udpiput3: version %d", version);
                                }
                        }
+                       qlock(&udp->qlock);
                        c = Fsnewcall(c, raddr, rport, laddr, lport, version);
+                       qunlock(&udp->qlock);
                        if (c == NULL) {
-                               qunlock(&udp->qlock);
                                freeblist(bp);
                                return;
                        }
@@ -499,7 +497,6 @@ void udpiput(struct Proto *udp, struct Ipifc *ifc, struct block *bp)
        }
 
        qlock(&c->qlock);
-       qunlock(&udp->qlock);
 
        /*
         * Trim the packet down to data size
@@ -616,7 +613,6 @@ void udpadvise(struct Proto *udp, struct block *bp, char *msg)
        }
 
        /* Look for a connection */
-       qlock(&udp->qlock);
        for (p = udp->conv; *p; p++) {
                s = *p;
                if (s->rport == pdest)
@@ -626,7 +622,6 @@ void udpadvise(struct Proto *udp, struct block *bp, char *msg)
                                                if (s->ignoreadvice)
                                                        break;
                                                qlock(&s->qlock);
-                                               qunlock(&udp->qlock);
                                                qhangup(s->rq, msg);
                                                qhangup(s->wq, msg);
                                                qunlock(&s->qlock);
@@ -634,7 +629,6 @@ void udpadvise(struct Proto *udp, struct block *bp, char *msg)
                                                return;
                                        }
        }
-       qunlock(&udp->qlock);
        freeblist(bp);
 }