mnt: Identify an RPC reader by kthread, not proc
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 25 Jul 2018 18:52:51 +0000 (14:52 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 30 Jul 2018 20:05:46 +0000 (16:05 -0400)
This fixes the dreaded mnt mismatch, e.g.:

mnt: proc current->text 18446744072670166253: mismatch from /net/tcp/1/data
? rep 0x0xffff800009cee020 tag 4 fid 1625 T120 R117 rp 5

The old code assumed the struct proc pointer uniquely identified a kernel
thread that was trying to get a response from a mnt chan.  That was never
the case, since you could have the same proc issue multiple syscalls
concurrently.

We had this bug since the beginning, but it was hard to trigger.  Now that
we have RCU CBs to close chans, which kick off an RKM, the kthread that
actually closes the chan is much more likely to not be for the original
process.  In that case, we're able to get the RPC response from some other
kthread, resulting in the mismatch.

Using the struct kthread to identify the kthread that is waiting on an RPC
is much saner.

I left in using current->user.name for the auth/attach name.  In those
cases, we should be in a syscall (which I check for).  Similarly, when
aborting, we just need to bail out if we are handling a process's syscall -
not if we're some arbitrary kthread.  I think.  There might be issues with
that.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/drivers/dev/mnt.c
kern/include/ns.h

index e714f52..ff4c99a 100644 (file)
@@ -321,6 +321,10 @@ struct chan *mntauth(struct chan *c, char *spec)
 
        r->request.type = Tauth;
        r->request.afid = c->fid;
 
        r->request.type = Tauth;
        r->request.afid = c->fid;
+       /* This assumes we're called from a syscall, which should always be true. */
+       if (!current_kthread->sysc)
+               warn("Kthread %s didn't have a syscall, current is %s",
+                    current_kthread->name, current ? current->progname : NULL);
        r->request.uname = current->user.name;
        r->request.aname = spec;
        mountrpc(m, r);
        r->request.uname = current->user.name;
        r->request.aname = spec;
        mountrpc(m, r);
@@ -381,6 +385,10 @@ static struct chan *mntattach(char *muxattach)
                r->request.afid = NOFID;
        else
                r->request.afid = params->authchan->fid;
                r->request.afid = NOFID;
        else
                r->request.afid = params->authchan->fid;
+       /* This assumes we're called from a syscall, which should always be true. */
+       if (!current_kthread->sysc)
+               warn("Kthread %s didn't have a syscall, current is %s",
+                    current_kthread->name, current ? current->progname : NULL);
        r->request.uname = current->user.name;
        r->request.aname = params->spec;
        mountrpc(m, r);
        r->request.uname = current->user.name;
        r->request.aname = params->spec;
        mountrpc(m, r);
@@ -798,31 +806,37 @@ void mountrpc(struct mnt *m, struct mntrpc *r)
                        cn = "?";
                        if (r->c != NULL && r->c->name != NULL)
                                cn = r->c->name->s;
                        cn = "?";
                        if (r->c != NULL && r->c->name != NULL)
                                cn = r->c->name->s;
-                       printk
-                               ("mnt: proc %s %lu: mismatch from %s %s rep 0x%p tag %d fid %d T%d R%d rp %d\n",
-                                "current->text", "current->pid", sn, cn, r, r->request.tag,
+                       warn("mnt: mismatch from %s %s rep %p tag %d fid %d T%d R%d rp %d\n",
+                                sn, cn, r, r->request.tag,
                                 r->request.fid, r->request.type, r->reply.type, r->reply.tag);
                        error(EPROTO, ERROR_FIXME);
        }
 }
 
                                 r->request.fid, r->request.type, r->reply.type, r->reply.tag);
                        error(EPROTO, ERROR_FIXME);
        }
 }
 
+static bool kth_proc_is_dying(struct kthread *kth)
+{
+       return kth->proc ? proc_is_dying(kth->proc) : false;
+}
+
 void mountio(struct mnt *m, struct mntrpc *r)
 {
        ERRSTACK(1);
        int n;
 
        while (waserror()) {
 void mountio(struct mnt *m, struct mntrpc *r)
 {
        ERRSTACK(1);
        int n;
 
        while (waserror()) {
-               if (m->rip == current)
+               if (m->rip == current_kthread)
                        mntgate(m);
                /* Syscall aborts are like Plan 9 Eintr.  For those, we need to change
                        mntgate(m);
                /* Syscall aborts are like Plan 9 Eintr.  For those, we need to change
-                * the old request to a flsh (mntflushalloc) and try again.  We'll
+                * the old request to a flush (mntflushalloc) and try again.  We'll
                 * always try to flush, and you can't get out until the flush either
                 * succeeds or errors out with a non-abort/Eintr error.
                 *
                 * This all means that regular aborts cannot break us out of here!  We
                 * can consider that policy in the future, if we need to.  Regardless,
                 * always try to flush, and you can't get out until the flush either
                 * succeeds or errors out with a non-abort/Eintr error.
                 *
                 * This all means that regular aborts cannot break us out of here!  We
                 * can consider that policy in the future, if we need to.  Regardless,
-                * if the process is dying, we really do need to abort. */
-               if ((get_errno() != EINTR) || proc_is_dying(current)) {
+                * if the process is dying, we really do need to abort.  We might not
+                * always have a process (RKM chan_release), but in that case we're fine
+                * - we're not preventing a process from dying. */
+               if ((get_errno() != EINTR) || kth_proc_is_dying(current_kthread)) {
                        /* all other errors or dying, bail out! */
                        mntflushfree(m, r);
                        nexterror();
                        /* all other errors or dying, bail out! */
                        mntflushfree(m, r);
                        nexterror();
@@ -863,7 +877,7 @@ void mountio(struct mnt *m, struct mntrpc *r)
                        return;
                }
        }
                        return;
                }
        }
-       m->rip = current;
+       m->rip = current_kthread;
        spin_unlock(&m->lock);
        while (r->done == 0) {
                if (mntrpcread(m, r) < 0)
        spin_unlock(&m->lock);
        while (r->done == 0) {
                if (mntrpcread(m, r) < 0)
index a0ba0ad..f28424c 100644 (file)
@@ -593,7 +593,7 @@ struct mnt {
        spinlock_t lock;
        /* references are counted using c->ref; channels on this mount point incref(c->mchan) == Mnt.c */
        struct chan *c;                         /* Channel to file service */
        spinlock_t lock;
        /* references are counted using c->ref; channels on this mount point incref(c->mchan) == Mnt.c */
        struct chan *c;                         /* Channel to file service */
-       struct proc *rip;                       /* Reader in progress */
+       struct kthread *rip;            /* Reader in progress */
        struct mntrpc *queue;           /* Queue of pending requests on this channel */
        uint32_t id;                            /* Multiplexer id for channel check */
        struct mnt *list;                       /* Free list */
        struct mntrpc *queue;           /* Queue of pending requests on this channel */
        uint32_t id;                            /* Multiplexer id for channel check */
        struct mnt *list;                       /* Free list */