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

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

The following reply was made to PR kern/37915; it has been noted by GNATS.

From: David Holland <dholland-bugs%netbsd.org@localhost>
To: dholland%eecs.harvard.edu@localhost
Cc: kern-bug-people%netbsd.org@localhost, gnats-admin%netbsd.org@localhost,
        netbsd-bugs%netbsd.org@localhost, gnats-bugs%netbsd.org@localhost
Subject: Re: kern/37915: vt100 + wscons + tty vmlocking changes = panic
Date: Fri, 2 Jan 2009 09:07:32 +0000

 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