tty: Fix race in tty release

Ian Abbott found that the tty layer would explode with the right set of
parallel open and close operations. This is because we race in the
handling of tty->drivers->termios[].

Correct this by
	Making tty_ldisc_release behave like nromal code (takes the lock,
			does stuff, drops the lock)
	Drop the tty lock earlier in tty_ldisc_release
	Taking the tty mutex around the driver->termios update in all cases
	Adding a WARN_ON to catch future screwups.

I also forgot to clean up the pty resources properly. With a pty pair we
need to pull both halves out of the tables.

Signed-off-by: Alan Cox <alan@linux.intel.com>
Tested-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 60c08ce..d6579a9 100644 (file)
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -282,6 +282,17 @@ done:
        return 0;
 }
 
+/**
+ *     pty_common_install              -       set up the pty pair
+ *     @driver: the pty driver
+ *     @tty: the tty being instantiated
+ *     @bool: legacy, true if this is BSD style
+ *
+ *     Perform the initial set up for the tty/pty pair. Called from the
+ *     tty layer when the port is first opened.
+ *
+ *     Locking: the caller must hold the tty_mutex
+ */
 static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
                bool legacy)
 {
@@ -364,6 +375,14 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
        return pty_common_install(driver, tty, true);
 }
 
+static void pty_remove(struct tty_driver *driver, struct tty_struct *tty)
+{
+       struct tty_struct *pair = tty->link;
+       driver->ttys[tty->index] = NULL;
+       if (pair)
+               pair->driver->ttys[pair->index] = NULL;
+}
+
 static int pty_bsd_ioctl(struct tty_struct *tty,
                         unsigned int cmd, unsigned long arg)
 {
@@ -395,7 +414,8 @@ static const struct tty_operations master_pty_ops_bsd = {
        .set_termios = pty_set_termios,
        .ioctl = pty_bsd_ioctl,
        .cleanup = pty_cleanup,
-       .resize = pty_resize
+       .resize = pty_resize,
+       .remove = pty_remove
 };
 
 static const struct tty_operations slave_pty_ops_bsd = {
@@ -409,7 +429,8 @@ static const struct tty_operations slave_pty_ops_bsd = {
        .unthrottle = pty_unthrottle,
        .set_termios = pty_set_termios,
        .cleanup = pty_cleanup,
-       .resize = pty_resize
+       .resize = pty_resize,
+       .remove = pty_remove
 };
 
 static void __init legacy_pty_init(void)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index be18d60..c6f4d71 100644 (file)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1465,7 +1465,6 @@ EXPORT_SYMBOL(tty_free_termios);
  *     in use. It also gets called when setup of a device fails.
  *
  *     Locking:
- *             tty_mutex - sometimes only
  *             takes the file list lock internally when working on the list
  *     of ttys that the driver keeps.
  *
@@ -1526,17 +1525,16 @@ EXPORT_SYMBOL(tty_kref_put);
  *     and decrement the refcount of the backing module.
  *
  *     Locking:
- *             tty_mutex - sometimes only
+ *             tty_mutex
  *             takes the file list lock internally when working on the list
  *     of ttys that the driver keeps.
- *             FIXME: should we require tty_mutex is held here ??
  *
  */
 static void release_tty(struct tty_struct *tty, int idx)
 {
        /* This should always be true but check for the moment */
        WARN_ON(tty->index != idx);
-
+       WARN_ON(!mutex_is_locked(&tty_mutex));
        if (tty->ops->shutdown)
                tty->ops->shutdown(tty);
        tty_free_termios(tty);
@@ -1708,6 +1706,9 @@ int tty_release(struct inode *inode, struct file *filp)
         * The closing flags are now consistent with the open counts on
         * both sides, and we've completed the last operation that could
         * block, so it's safe to proceed with closing.
+        *
+        * We must *not* drop the tty_mutex until we ensure that a further
+        * entry into tty_open can not pick up this tty.
         */
        if (pty_master) {
                if (--o_tty->count < 0) {
@@ -1759,12 +1760,13 @@ int tty_release(struct inode *inode, struct file *filp)
        }
 
        mutex_unlock(&tty_mutex);
+       tty_unlock();
+       /* At this point the TTY_CLOSING flag should ensure a dead tty
+          cannot be re-opened by a racing opener */
 
        /* check whether both sides are closing ... */
-       if (!tty_closing || (o_tty && !o_tty_closing)) {
-               tty_unlock();
+       if (!tty_closing || (o_tty && !o_tty_closing))
                return 0;
-       }
 
 #ifdef TTY_DEBUG_HANGUP
        printk(KERN_DEBUG "%s: freeing tty structure...\n", __func__);
@@ -1777,12 +1779,14 @@ int tty_release(struct inode *inode, struct file *filp)
         * The release_tty function takes care of the details of clearing
         * the slots and preserving the termios structure.
         */
+       mutex_lock(&tty_mutex);
        release_tty(tty, idx);
+       mutex_unlock(&tty_mutex);
 
        /* Make this pty number available for reallocation */
        if (devpts)
                devpts_kill_index(inode, idx);
-       tty_unlock();
+
        return 0;
 }
 

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index e6156c6..3d06871 100644 (file)
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -912,7 +912,6 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
         * race with the set_ldisc code path.
         */
 
-       tty_unlock();
        tty_ldisc_halt(tty);
        tty_ldisc_flush_works(tty);
        tty_lock();
@@ -930,6 +929,8 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
        tty_set_termios_ldisc(tty, N_TTY);
        mutex_unlock(&tty->ldisc_mutex);
 
+       tty_unlock();
+
        /* This will need doing differently if we need to lock */
        if (o_tty)
                tty_ldisc_release(o_tty, NULL);