Re: kern/37915: vt100 + wscons + tty vmlocking changes = panic

[David Holland <dholland-bugs%netbsd.org@localhost>]

On Wed, Jan 30, 2008 at 01:30:00AM +0000, dholland%eecs.harvard.edu@localhost 
wrote:
 > The problem is that ttwrite() holds tty_lock while calling ttstart();
 > this makes its way down to wsemul_vt100_handle_csi(), which for
 > several sequences that report stuff back calls wsdisplay_emulinput(),
 > which calls ttyinput(), which tries to get tty_lock again.

Making a small and noninvasive patch for this (as ad@ requested) turns
out to be not so trivial, since it turns out that spin mutexes don't
track who's holding them. (And for a while I thought tty_lock was
being released and reacquired somewhere inside ttinput, which would
have made it just about impossible, but that turns out to have been a
red herring.)

Thus, while the following patch could be considered a workaround, I
can't with a straight face call it a fix.

Index: tty.c
===================================================================
RCS file: /cvsroot/src/sys/kern/tty.c,v
retrieving revision 1.228
diff -u -p -r1.228 tty.c
--- tty.c       19 Nov 2008 18:36:07 -0000      1.228
+++ tty.c       2 Jan 2009 09:01:35 -0000
@@ -202,6 +202,10 @@ kmutex_t tty_lock;
 krwlock_t ttcompat_lock;
 int (*ttcompatvec)(struct tty *, u_long, void *, int, struct lwp *);
 
+/* XXX: names the cpu that's currently doing ttstart(). See PR 37915. */
+static struct cpu_info *in_ttstart_hack_cpu;
+static unsigned in_ttstart_hack_depth;
+
 uint64_t tk_cancc;
 uint64_t tk_nin;
 uint64_t tk_nout;
@@ -702,11 +706,20 @@ ttyinput_wlock(int c, struct tty *tp)
  *
  * XXX - this is a hack, all drivers must changed to acquire the
  *      lock before calling linesw->l_rint()
+ *
+ * XXX - it is a bigger hack; wscons sometimes already has the lock.
+ *      We record which cpu is in ttstart (and how many times) to
+ *      avoid re-entering the lock when we come back here via wscons.
+ *      This is a hack for a specific known case, not a general fix.
+ *      Note that we're also misusing mutex_owned, but the specific
+ *      usage ought to work and a real fix is planned for later.
+ *      PR 37915.
  */
 int
 ttyinput(int c, struct tty *tp)
 {
        int error;
+       int dolock = 1;
 
        /*
         * Unless the receiver is enabled, drop incoming data.
@@ -714,9 +727,16 @@ ttyinput(int c, struct tty *tp)
        if (!ISSET(tp->t_cflag, CREAD))
                return (0);
 
-       mutex_spin_enter(&tty_lock);
+       /* The bigger hack. */
+       if (mutex_owned(&tty_lock) && in_ttstart_hack_cpu == curcpu()) {
+               dolock = 0;
+       }
+
+       if (dolock)
+               mutex_spin_enter(&tty_lock);
        error = ttyinput_wlock(c, tp);
-       mutex_spin_exit(&tty_lock);
+       if (dolock)
+               mutex_spin_exit(&tty_lock);
 
        return (error);
 }
@@ -1551,9 +1571,39 @@ ttrstrt(void *tp_arg)
 int
 ttstart(struct tty *tp)
 {
+       struct cpu_info *mycpu;
+
+       mycpu = curcpu();
+       KASSERT(mutex_owned(&tty_lock));
+
+       /*
+        * Track who's in ttstart. See comment above ttyinput(), and
+        * PR 37915. If someone else was still in ttstart, overwrite
+        * them. This appears to be necessary because something
+        * releases and reacquires tty_lock inside ->t_oproc; I'm
+        * hoping it doesn't call ttyinput() again after that.
+        */
+       if (in_ttstart_hack_cpu == mycpu) {
+               in_ttstart_hack_depth++;
+       } else if (in_ttstart_hack_cpu == NULL) {
+               in_ttstart_hack_cpu = mycpu;
+               in_ttstart_hack_depth = 1;
+       } else {
+               panic("ttstart: unexpected reentrant call "
+                     "(tty_lock released inside t_oproc?)\n");
+       }
 
        if (tp->t_oproc != NULL)        /* XXX: Kludge for pty. */
                (*tp->t_oproc)(tp);
+
+       KASSERT(mycpu == curcpu());
+       KASSERT(in_ttstart_hack_cpu == mycpu);
+       KASSERT(in_ttstart_hack_depth > 0);
+       in_ttstart_hack_depth--;
+       if (in_ttstart_hack_depth == 0) {
+               in_ttstart_hack_cpu = NULL;
+       }
+
        return (0);
 }
 


-- 
David A. Holland
dholland%netbsd.org@localhost