[PATCH] pty_chars_in_buffer oops fix

The idea of this patch is to lock both sides of a ptmx/pty pair during line
discipline changing.  This is needed to ensure that say a poll on one side of
the pty doesn't occur while the line discipline is actively being changed.
This resulted in an oops reported on lkml, see:

	http://marc.theaimsgroup.com/?l=linux-kernel&m=111342171410005&w=2

A 'hacky' approach was previously implmemented which served to eliminate the
poll vs.  line discipline changing race.  However, this patch takes a more
general approach to the issue.  The patch only adds locking on a less often
used path, the line-discipline changing path, as opposed to locking the
ptmx/pty pair on read/write/poll paths.

The patch below, takes both ldisc locks in either order b/c the locks are both
taken under the same spinlock().  I thought about locking the ptmx/pty
separately, such as master always first but that introduces a 3 way deadlock.
For example, process 1 does a blocking read on the slave side.  Then, process
2 does an ldisc change on the slave side, which acquires the master ldisc lock
but not the slave's.  Finally, process 3 does a write which blocks on the
process 2's ldisc reference.

This patch does introduce some changes in semantics.  For example, a line
discipline change on side 'a' of a ptmx/pty pair, will now wait for a
read/write to complete on the other side, or side 'b'.  The current behavior
is to simply wait for any read/writes on only side 'a', not both sides 'a' and
'b'.  I think this behavior makes sense, but I wanted to point it out.

I've tested the patch with a bunch of read/write/poll while changing the line
discipline out from underneath.

This patch obviates the need for the above "hide the problem" patch.

Signed-off-by: Jason Baron <jbaron@redhat.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

2 files changed, 56 insertions, 28 deletions

diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index da32889d22c0..49f3997fd251 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -149,15 +149,14 @@ static int pty_write_room(struct tty_struct *tty)
 static int pty_chars_in_buffer(struct tty_struct *tty)
 {
         struct tty_struct *to = tty->link;
-        ssize_t (*chars_in_buffer)(struct tty_struct *);
         int count;
 
         /* We should get the line discipline lock for "tty->link" */
-        if (!to || !(chars_in_buffer = to->ldisc.chars_in_buffer))
+        if (!to || !to->ldisc.chars_in_buffer)
                 return 0;
 
         /* The ldisc must report 0 if no characters available to be read */
-        count = chars_in_buffer(to);
+        count = to->ldisc.chars_in_buffer(to);
 
         if (tty->driver->subtype == PTY_TYPE_SLAVE) return count;
 
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 9d657127f313..6a56ae4f7725 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -469,21 +469,19 @@ static void tty_ldisc_enable(struct tty_struct *tty)
  
 static int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 {
-        int     retval = 0;
-        struct  tty_ldisc o_ldisc;
+        int retval = 0;
+        struct tty_ldisc o_ldisc;
         char buf[64];
         int work;
         unsigned long flags;
         struct tty_ldisc *ld;
+        struct tty_struct *o_tty;
 
         if ((ldisc < N_TTY) || (ldisc >= NR_LDISCS))
                 return -EINVAL;
 
 restart:
 
-        if (tty->ldisc.num == ldisc)
-                return 0;       /* We are already in the desired discipline */
-        
         ld = tty_ldisc_get(ldisc);
         /* Eduardo Blanco <ejbs@cs.cs.com.uy> */
         /* Cyrus Durgin <cider@speakeasy.org> */
@@ -494,45 +492,74 @@ restart:
         if (ld == NULL)
                 return -EINVAL;
 
-        o_ldisc = tty->ldisc;
-
         tty_wait_until_sent(tty, 0);
 
+        if (tty->ldisc.num == ldisc) {
+                tty_ldisc_put(ldisc);
+                return 0;
+        }
+
+        o_ldisc = tty->ldisc;
+        o_tty = tty->link;
+
         /*
          *      Make sure we don't change while someone holds a
          *      reference to the line discipline. The TTY_LDISC bit
          *      prevents anyone taking a reference once it is clear.
          *      We need the lock to avoid racing reference takers.
          */
-         
+
         spin_lock_irqsave(&tty_ldisc_lock, flags);
-        if(tty->ldisc.refcount)
-        {
-                /* Free the new ldisc we grabbed. Must drop the lock
-                   first. */
+        if (tty->ldisc.refcount || (o_tty && o_tty->ldisc.refcount)) {
+                if(tty->ldisc.refcount) {
+                        /* Free the new ldisc we grabbed. Must drop the lock
+                           first. */
+                        spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+                        tty_ldisc_put(ldisc);
+                        /*
+                         * There are several reasons we may be busy, including
+                         * random momentary I/O traffic. We must therefore
+                         * retry. We could distinguish between blocking ops
+                         * and retries if we made tty_ldisc_wait() smarter. That
+                         * is up for discussion.
+                         */
+                        if (wait_event_interruptible(tty_ldisc_wait, tty->ldisc.refcount == 0) < 0)
+                                return -ERESTARTSYS;
+                        goto restart;
+                }
+                if(o_tty && o_tty->ldisc.refcount) {
+                        spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+                        tty_ldisc_put(ldisc);
+                        if (wait_event_interruptible(tty_ldisc_wait, o_tty->ldisc.refcount == 0) < 0)
+                                return -ERESTARTSYS;
+                        goto restart;
+                }
+        }
+
+        /* if the TTY_LDISC bit is set, then we are racing against another ldisc change */
+
+        if (!test_bit(TTY_LDISC, &tty->flags)) {
                 spin_unlock_irqrestore(&tty_ldisc_lock, flags);
                 tty_ldisc_put(ldisc);
-                /*
-                 * There are several reasons we may be busy, including
-                 * random momentary I/O traffic. We must therefore
-                 * retry. We could distinguish between blocking ops
-                 * and retries if we made tty_ldisc_wait() smarter. That
-                 * is up for discussion.
-                 */
-                if(wait_event_interruptible(tty_ldisc_wait, tty->ldisc.refcount == 0) < 0)
-                        return -ERESTARTSYS;                    
+                ld = tty_ldisc_ref_wait(tty);
+                tty_ldisc_deref(ld);
                 goto restart;
         }
-        clear_bit(TTY_LDISC, &tty->flags);      
+
+        clear_bit(TTY_LDISC, &tty->flags);
         clear_bit(TTY_DONT_FLIP, &tty->flags);
+        if (o_tty) {
+                clear_bit(TTY_LDISC, &o_tty->flags);
+                clear_bit(TTY_DONT_FLIP, &o_tty->flags);
+        }
         spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-        
+
         /*
          *      From this point on we know nobody has an ldisc
          *      usage reference, nor can they obtain one until
          *      we say so later on.
          */
-         
+
         work = cancel_delayed_work(&tty->flip.work);
         /*
          * Wait for ->hangup_work and ->flip.work handlers to terminate
@@ -583,10 +610,12 @@ restart:
          */
          
         tty_ldisc_enable(tty);
+        if (o_tty)
+                tty_ldisc_enable(o_tty);
         
         /* Restart it in case no characters kick it off. Safe if
            already running */
-        if(work)
+        if (work)
                 schedule_delayed_work(&tty->flip.work, 1);
         return retval;
 }