[PATCH -next 00/27] tty locking changes

[PATCH -next 00/27] tty locking changes

[Peter Hurley]

Hi Greg,

This patch series has 3 major changes to how tty locking behaves:
1. the lock order of tty_lock() and tty->ldisc_sem is reversed;
   this eliminates a bunch of lock drop/reacquire which, in turn,
   eliminates tty state tracking that can no longer be observed.
   This also allows the pty driver to wait for input processing to
   complete while closing before setting TTY_OTHER_CLOSED (which
   eliminates the ugliness of checking input twice in n_tty_read() and
   n_tty_poll()).
2. the footprint of tty_mutex is reduced to only adding and removing
   ttys and is no longer held to acquire the tty_lock() in tty_open();
   this allows for multiple ttys to be opened concurrently, even if
   one open stalls waiting for its tty_lock().
3. pty pair locking is reordered to master first, then slave, rather
   than by address. This works because, while releasing the master pty,
   the slave tty count needs to be changed, whereas, when releasing the
   slave, the master pty does not need to be accessed.
   This furthur eliminates more lock drop/reacquire.

The longer-term goals, which this series builds towards, is:
1. simplifying the tty open/close behavior
2. eliminating the ASYNC_CLOSING code without breaking existing userspace
3. eliminating returning -EIO from tty_open(). Not sure if this is possible yet.

Regards,

Peter Hurley (27):
  tty: Don't hold tty_lock for ldisc release
  tty: Invert tty_lock/ldisc_sem lock order
  tty: Remove TTY_HUPPING
  tty: Clarify re-open behavior of master ptys
  tty: Check tty->count instead of TTY_CLOSING in tty_reopen()
  pty: Always return -EIO if slave BSD pty opened first
  tty: Re-open /dev/tty without tty_mutex
  tty: Drop tty_mutex before tty reopen
  tty: Remove TTY_CLOSING
  tty: Don't take tty_mutex for tty count changes
  tty: Don't release tty locks for wait queue sanity check
  tty: Document check_tty_count() requires tty_lock held
  tty: Simplify pty pair teardown logic
  tty: Fold pty pair handling into tty_flush_works()
  tty: Simplify tty_ldisc_release() interface
  tty: Simplify tty_release_checks() interface
  tty: Simplify tty_release() state checks
  tty: Change tty lock order to master->slave
  tty: Remove tty_unhangup() declaration
  tty: Refactor __tty_hangup to enable lockdep annotation
  pty: Don't drop pty master tty lock to hangup slave
  tty: Document hangup call tree
  pty, n_tty: Simplify input processing on final close
  tty: Prefix tty_ldisc_{lock,lock_nested,unlock} functions
  tty: Fix hung task on pty hangup
  tty: Fix timeout on pty set ldisc
  tty: Flush ldisc buffer atomically with tty flip buffers

 drivers/tty/n_tty.c      |  46 ++++----
 drivers/tty/pty.c        |  12 +-
 drivers/tty/tty_buffer.c |  10 +-
 drivers/tty/tty_io.c     | 279 ++++++++++++++++++++++++-----------------------
 drivers/tty/tty_ldisc.c  | 106 +++++++++---------
 drivers/tty/tty_mutex.c  |  32 +++---
 include/linux/tty.h      |  15 +--
 7 files changed, 250 insertions(+), 250 deletions(-)

-- 
2.1.1

[PATCH -next 01/27] tty: Don't hold tty_lock for ldisc release

[Peter Hurley]

The tty->ldisc_sem write lock is sufficient for serializing changes
to tty->ldisc; holding the tty lock is not required.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 2d822aa..332a622 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -764,6 +764,8 @@ static void tty_ldisc_kill(struct tty_struct *tty)
  *	Called during the final close of a tty/pty pair in order to shut down
  *	the line discpline layer. On exit the ldisc assigned is N_TTY and the
  *	ldisc has not been opened.
+ *
+ *	Holding ldisc_sem write lock serializes tty->ldisc changes.
  */
 
 void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
@@ -776,13 +778,9 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
 	tty_ldisc_debug(tty, "closing ldisc: %p\n", tty->ldisc);
 
 	tty_ldisc_lock_pair(tty, o_tty);
-	tty_lock_pair(tty, o_tty);
-
 	tty_ldisc_kill(tty);
 	if (o_tty)
 		tty_ldisc_kill(o_tty);
-
-	tty_unlock_pair(tty, o_tty);
 	tty_ldisc_unlock_pair(tty, o_tty);
 
 	/* And the memory resources remaining (buffers, termios) will be
-- 
2.1.1

[PATCH -next 02/27] tty: Invert tty_lock/ldisc_sem lock order

[Peter Hurley]

Dropping the tty lock to acquire the tty->ldisc_sem allows several
race conditions (such as hangup while changing the ldisc) which requires
extra states and testing. The ldisc_sem->tty_lock lock order has
not been required since tty buffer ownership was moved to tty_port.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 332a622..28858eb 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -523,9 +523,11 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	if (IS_ERR(new_ldisc))
 		return PTR_ERR(new_ldisc);
 
+	tty_lock(tty);
 	retval = tty_ldisc_lock_pair_timeout(tty, o_tty, 5 * HZ);
 	if (retval) {
 		tty_ldisc_put(new_ldisc);
+		tty_unlock(tty);
 		return retval;
 	}
 
@@ -536,11 +538,11 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	if (tty->ldisc->ops->num == ldisc) {
 		tty_ldisc_enable_pair(tty, o_tty);
 		tty_ldisc_put(new_ldisc);
+		tty_unlock(tty);
 		return 0;
 	}
 
 	old_ldisc = tty->ldisc;
-	tty_lock(tty);
 
 	if (test_bit(TTY_HUPPING, &tty->flags) ||
 	    test_bit(TTY_HUPPED, &tty->flags)) {
@@ -675,8 +677,6 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	wake_up_interruptible_poll(&tty->write_wait, POLLOUT);
 	wake_up_interruptible_poll(&tty->read_wait, POLLIN);
 
-	tty_unlock(tty);
-
 	/*
 	 * Shutdown the current line discipline, and reset it to
 	 * N_TTY if need be.
@@ -684,7 +684,6 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	 * Avoid racing set_ldisc or tty_ldisc_release
 	 */
 	tty_ldisc_lock_pair(tty, tty->link);
-	tty_lock(tty);
 
 	if (tty->ldisc) {
 
-- 
2.1.1

[PATCH -next 03/27] tty: Remove TTY_HUPPING

[Peter Hurley]

Now that tty_ldisc_hangup() does not drop the tty lock, it is no
longer possible to observe TTY_HUPPING while holding the tty lock
on another cpu.

Remove TTY_HUPPING bit definition.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c    | 12 +-----------
 drivers/tty/tty_ldisc.c |  3 +--
 include/linux/tty.h     |  1 -
 3 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e18491a..fd06512 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -690,9 +690,6 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 		return;
 	}
 
-	/* some functions below drop BTM, so we need this bit */
-	set_bit(TTY_HUPPING, &tty->flags);
-
 	/* inuse_filps is protected by the single tty lock,
 	   this really needs to change if we want to flush the
 	   workqueue with the lock held */
@@ -717,10 +714,6 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 	while (refs--)
 		tty_kref_put(tty);
 
-	/*
-	 * it drops BTM and thus races with reopen
-	 * we protect the race by TTY_HUPPING
-	 */
 	tty_ldisc_hangup(tty);
 
 	spin_lock_irq(&tty->ctrl_lock);
@@ -752,8 +745,6 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 	 * can't yet guarantee all that.
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
-	clear_bit(TTY_HUPPING, &tty->flags);
-
 	tty_unlock(tty);
 
 	if (f)
@@ -1461,8 +1452,7 @@ static int tty_reopen(struct tty_struct *tty)
 {
 	struct tty_driver *driver = tty->driver;
 
-	if (test_bit(TTY_CLOSING, &tty->flags) ||
-			test_bit(TTY_HUPPING, &tty->flags))
+	if (test_bit(TTY_CLOSING, &tty->flags))
 		return -EIO;
 
 	if (driver->type == TTY_DRIVER_TYPE_PTY &&
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 28858eb..49001fa 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -544,8 +544,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	old_ldisc = tty->ldisc;
 
-	if (test_bit(TTY_HUPPING, &tty->flags) ||
-	    test_bit(TTY_HUPPED, &tty->flags)) {
+	if (test_bit(TTY_HUPPED, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
 		   the ldisc data and closed the ldisc down */
 		tty_ldisc_enable_pair(tty, o_tty);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index b36b0b4..ff0dd7a 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -321,7 +321,6 @@ struct tty_file_private {
 #define TTY_PTY_LOCK 		16	/* pty private */
 #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
 #define TTY_HUPPED 		18	/* Post driver->hangup() */
-#define TTY_HUPPING 		21	/* ->hangup() in progress */
 #define TTY_LDISC_HALTED	22	/* Line discipline is halted */
 
 #define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
-- 
2.1.1

[PATCH -next 04/27] tty: Clarify re-open behavior of master ptys

[Peter Hurley]

Re-opening master ptys is not allowed. Once opened and for the remaining
lifetime of the master pty, its tty count is 1. If its tty count has
dropped to 0, then the master pty was closed and TTY_CLOSING was set,
and destruction may begin imminently.

Besides the normal case of a legacy BSD pty master being re-opened
(which always returns -EIO), this code is only reachable in 2 degenerate
cases:
1. The pty master is the controlling terminal (this is possible through
   the TIOCSCTTY ioctl). pty masters are not designed to be controlling
   terminals and it's an oversight that tiocsctty() ever let that happen.
   The attempted open of /dev/tty will always fail. No known program does
   this.
2. The legacy BSD pty slave was opened first. The slave open will fail
   in pty_open() and tty_release() will commence. But before tty_release()
   claims the tty_mutex, there is a very small window where a parallel
   master open might succeed. In a test of racing legacy BSD slave and
   master parallel opens, where:
      slave open attempts:  10000   success:4527  failure:5473
      master open attempts: 11728   success:5789  failure:5939
   only 8 master open attempts would have succeeded reaching this code and
   successfully opened the master pty. This case is not possible with
   SysV ptys.

Always return -EIO if a master pty is re-opened or the slave is opened
first and the master opened in parallel (for legacy BSD ptys).

Furthermore, now that changing the slave's count is not required,
the tty_lock is sufficient for preventing concurrent changes to the
tty being re-opened (or failing re-opening).

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index fd06512..5b6253f 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1444,9 +1444,9 @@ void tty_driver_remove_tty(struct tty_driver *driver, struct tty_struct *tty)
  * 	@tty	- the tty to open
  *
  *	Return 0 on success, -errno on error.
+ *	Re-opens on master ptys are not allowed and return -EIO.
  *
- *	Locking: tty_mutex must be held from the time the tty was found
- *		 till this open completes.
+ *	Locking: Caller must hold tty_lock
  */
 static int tty_reopen(struct tty_struct *tty)
 {
@@ -1456,16 +1456,9 @@ static int tty_reopen(struct tty_struct *tty)
 		return -EIO;
 
 	if (driver->type == TTY_DRIVER_TYPE_PTY &&
-	    driver->subtype == PTY_TYPE_MASTER) {
-		/*
-		 * special case for PTY masters: only one open permitted,
-		 * and the slave side open count is incremented as well.
-		 */
-		if (tty->count)
-			return -EIO;
+	    driver->subtype == PTY_TYPE_MASTER)
+		return -EIO;
 
-		tty->link->count++;
-	}
 	tty->count++;
 
 	WARN_ON(!tty->ldisc);
-- 
2.1.1

[PATCH -next 05/27] tty: Check tty->count instead of TTY_CLOSING in tty_reopen()

[Peter Hurley]

Although perhaps not obvious, the TTY_CLOSING bit is set when the
tty count has been decremented to 0 (which occurs while holding
tty_lock). The only other case when tty count is 0 during a re-open
is when a legacy BSD pty master has been opened in parallel but
after the pty slave, which is unsupported and returns an error.

Thus !tty->count contains the complete set of degenerate conditions
under which a tty open fails.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 5b6253f..52bcdd3 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1452,7 +1452,7 @@ static int tty_reopen(struct tty_struct *tty)
 {
 	struct tty_driver *driver = tty->driver;
 
-	if (test_bit(TTY_CLOSING, &tty->flags))
+	if (!tty->count)
 		return -EIO;
 
 	if (driver->type == TTY_DRIVER_TYPE_PTY &&
-- 
2.1.1

[PATCH -next 06/27] pty: Always return -EIO if slave BSD pty opened first

[Peter Hurley]

Opening the slave BSD pty first already returns -EIO from the slave
pty_open(), which in turn causes the newly installed tty pair to be
released before returning from tty_open(). However, this can also
cause a parallel master BSD pty open to fail because the pty pair
destruction may already been taking place in tty_release().

Failing at driver->install() if the slave pty is opened first ensures
that a pty master open cannot fail, because the driver tables will
not have been updated so tty_driver_lookup_tty() won't find the
master pty (and attempt to "re-open" it).

In turn, this guarantees that any tty with a tty->count == 0 is
in final close (rather than never opened).

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 7a1a538..bdb8fd1 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -383,6 +383,10 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	int idx = tty->index;
 	int retval = -ENOMEM;
 
+	/* Opening the slave first has always returned -EIO */
+	if (driver->subtype != PTY_TYPE_MASTER)
+		return -EIO;
+
 	ports[0] = kmalloc(sizeof **ports, GFP_KERNEL);
 	ports[1] = kmalloc(sizeof **ports, GFP_KERNEL);
 	if (!ports[0] || !ports[1])
@@ -419,8 +423,6 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	 * Everything allocated ... set up the o_tty structure.
 	 */
 	tty_driver_kref_get(driver->other);
-	if (driver->subtype == PTY_TYPE_MASTER)
-		o_tty->count++;
 	/* Establish the links in both directions */
 	tty->link   = o_tty;
 	o_tty->link = tty;
@@ -432,6 +434,7 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 
 	tty_driver_kref_get(driver);
 	tty->count++;
+	o_tty->count++;
 	return 0;
 err_free_termios:
 	if (legacy)
-- 
2.1.1

[PATCH -next 07/27] tty: Re-open /dev/tty without tty_mutex

[Peter Hurley]

Opening /dev/tty (ie., the controlling tty for the current task)
is always a re-open of the underlying tty. Because holding the
tty_lock is sufficient for safely re-opening a tty, and because
having a tty kref is sufficient for safely acquiring the tty_lock [1],
tty_open_current_tty() does not require holding tty_mutex.

Repurpose tty_open_current_tty() to perform the re-open itself and
refactor tty_open().

[1] Analysis of safely re-opening the current tty w/o tty_mutex

get_current_tty() gets a tty kref from the already kref'ed tty value of
current->signal->tty while holding the sighand lock for the current
task. This guarantees that the tty pointer returned from
get_current_tty() points to a tty which remains referenceable
while holding the kref.

Although release_tty() may run concurrently, and thus the driver
reference may be removed, release_one_tty() cannot have run, and
won't while holding the tty kref.

This, in turn, guarantees the tty_lock() can safely be acquired
(since tty->magic and tty->legacy_mutex are still a valid dereferences).
The tty_lock() also gets a tty kref to prevent the tty_unlock() from
dereferencing a released tty. Thus, the kref returned from
get_current_tty() can be released.

Lastly, the first operation of tty_reopen() is to check the tty count.
If non-zero, this ensures release_tty() is not running concurrently,
and the driver references have not been removed.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 53 ++++++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 52bcdd3..1b6f38c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1943,20 +1943,20 @@ int tty_release(struct inode *inode, struct file *filp)
 }
 
 /**
- *	tty_open_current_tty - get tty of current task for open
+ *	tty_open_current_tty - get locked tty of current task
  *	@device: device number
  *	@filp: file pointer to tty
- *	@return: tty of the current task iff @device is /dev/tty
+ *	@return: locked tty of the current task iff @device is /dev/tty
+ *
+ *	Performs a re-open of the current task's controlling tty.
  *
  *	We cannot return driver and index like for the other nodes because
  *	devpts will not work then. It expects inodes to be from devpts FS.
- *
- *	We need to move to returning a refcounted object from all the lookup
- *	paths including this one.
  */
 static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
 {
 	struct tty_struct *tty;
+	int retval;
 
 	if (device != MKDEV(TTYAUX_MAJOR, 0))
 		return NULL;
@@ -1967,9 +1967,14 @@ static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
 
 	filp->f_flags |= O_NONBLOCK; /* Don't let /dev/tty block */
 	/* noctty = 1; */
-	tty_kref_put(tty);
-	/* FIXME: we put a reference and return a TTY! */
-	/* This is only safe because the caller holds tty_mutex */
+	tty_lock(tty);
+	tty_kref_put(tty);	/* safe to drop the kref now */
+
+	retval = tty_reopen(tty);
+	if (retval < 0) {
+		tty_unlock(tty);
+		tty = ERR_PTR(retval);
+	}
 	return tty;
 }
 
@@ -2067,13 +2072,9 @@ retry_open:
 	index  = -1;
 	retval = 0;
 
-	mutex_lock(&tty_mutex);
-	/* This is protected by the tty_mutex */
 	tty = tty_open_current_tty(device, filp);
-	if (IS_ERR(tty)) {
-		retval = PTR_ERR(tty);
-		goto err_unlock;
-	} else if (!tty) {
+	if (!tty) {
+		mutex_lock(&tty_mutex);
 		driver = tty_lookup_driver(device, filp, &noctty, &index);
 		if (IS_ERR(driver)) {
 			retval = PTR_ERR(driver);
@@ -2086,21 +2087,21 @@ retry_open:
 			retval = PTR_ERR(tty);
 			goto err_unlock;
 		}
-	}
 
-	if (tty) {
-		tty_lock(tty);
-		retval = tty_reopen(tty);
-		if (retval < 0) {
-			tty_unlock(tty);
-			tty = ERR_PTR(retval);
-		}
-	} else	/* Returns with the tty_lock held for now */
-		tty = tty_init_dev(driver, index);
+		if (tty) {
+			tty_lock(tty);
+			retval = tty_reopen(tty);
+			if (retval < 0) {
+				tty_unlock(tty);
+				tty = ERR_PTR(retval);
+			}
+		} else	/* Returns with the tty_lock held for now */
+			tty = tty_init_dev(driver, index);
 
-	mutex_unlock(&tty_mutex);
-	if (driver)
+		mutex_unlock(&tty_mutex);
 		tty_driver_kref_put(driver);
+	}
+
 	if (IS_ERR(tty)) {
 		retval = PTR_ERR(tty);
 		goto err_file;
-- 
2.1.1

[PATCH -next 08/27] tty: Drop tty_mutex before tty reopen

[Peter Hurley]

Holding tty_mutex for a tty re-open is no longer necessary since
"tty: Clarify re-open behavior of master ptys". Because the
slave tty count is no longer accessed by tty_reopen(), holding
tty_mutex to prevent concurrent final tty_release() of the slave
pty is not required.

As with "tty: Re-open /dev/tty without tty_mutex", holding a
tty kref until the tty_lock is acquired is sufficient to ensure
the tty has not been freed, which, in turn, is sufficient to
ensure the tty_lock can be safely acquired and the tty count
can be safely retrieved. A non-zero tty count with the tty lock
held guarantees that release_tty() has not run and cannot
run concurrently with tty_reopen().

Change tty_driver_lookup_tty() to acquire the tty kref, which
allows the tty_mutex to be dropped before acquiring the tty lock.
Dropping the tty_mutex before attempting the tty_lock allows
other ttys to be opened and released, without needing this
tty_reopen() to complete.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 1b6f38c..ded0445 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1344,19 +1344,24 @@ static ssize_t tty_line_name(struct tty_driver *driver, int index, char *p)
  *	@driver: the driver for the tty
  *	@idx:	 the minor number
  *
- *	Return the tty, if found or ERR_PTR() otherwise.
+ *	Return the tty, if found. If not found, return NULL or ERR_PTR() if the
+ *	driver lookup() method returns an error.
  *
- *	Locking: tty_mutex must be held. If tty is found, the mutex must
- *	be held until the 'fast-open' is also done. Will change once we
- *	have refcounting in the driver and per driver locking
+ *	Locking: tty_mutex must be held. If the tty is found, bump the tty kref.
  */
 static struct tty_struct *tty_driver_lookup_tty(struct tty_driver *driver,
 		struct inode *inode, int idx)
 {
+	struct tty_struct *tty;
+
 	if (driver->ops->lookup)
-		return driver->ops->lookup(driver, inode, idx);
+		tty = driver->ops->lookup(driver, inode, idx);
+	else
+		tty = driver->ttys[idx];
 
-	return driver->ttys[idx];
+	if (!IS_ERR(tty))
+		tty_kref_get(tty);
+	return tty;
 }
 
 /**
@@ -2089,16 +2094,20 @@ retry_open:
 		}
 
 		if (tty) {
+			mutex_unlock(&tty_mutex);
 			tty_lock(tty);
+			/* safe to drop the kref from tty_driver_lookup_tty() */
+			tty_kref_put(tty);
 			retval = tty_reopen(tty);
 			if (retval < 0) {
 				tty_unlock(tty);
 				tty = ERR_PTR(retval);
 			}
-		} else	/* Returns with the tty_lock held for now */
+		} else { /* Returns with the tty_lock held for now */
 			tty = tty_init_dev(driver, index);
+			mutex_unlock(&tty_mutex);
+		}
 
-		mutex_unlock(&tty_mutex);
 		tty_driver_kref_put(driver);
 	}
 
-- 
2.1.1

[PATCH -next 09/27] tty: Remove TTY_CLOSING

[Peter Hurley]

Now that re-open is not permitted for a legacy BSD pty master,
using TTY_CLOSING to indicate when a tty can be torn-down is
no longer necessary.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 14 ++------------
 include/linux/tty.h  |  1 -
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index ded0445..55f9931 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1197,7 +1197,7 @@ void tty_write_message(struct tty_struct *tty, char *msg)
 	if (tty) {
 		mutex_lock(&tty->atomic_write_lock);
 		tty_lock(tty);
-		if (tty->ops->write && !test_bit(TTY_CLOSING, &tty->flags)) {
+		if (tty->ops->write && tty->count > 0) {
 			tty_unlock(tty);
 			tty->ops->write(tty, msg, strlen(msg));
 		} else
@@ -1887,16 +1887,6 @@ int tty_release(struct inode *inode, struct file *filp)
 	/*
 	 * Perform some housekeeping before deciding whether to return.
 	 *
-	 * Set the TTY_CLOSING flag if this was the last open.  In the
-	 * case of a pty we may have to wait around for the other side
-	 * to close, and TTY_CLOSING makes sure we can't be reopened.
-	 */
-	if (tty_closing)
-		set_bit(TTY_CLOSING, &tty->flags);
-	if (o_tty_closing)
-		set_bit(TTY_CLOSING, &o_tty->flags);
-
-	/*
 	 * If _either_ side is closing, make sure there aren't any
 	 * processes that still think tty or o_tty is their controlling
 	 * tty.
@@ -1911,7 +1901,7 @@ int tty_release(struct inode *inode, struct file *filp)
 
 	mutex_unlock(&tty_mutex);
 	tty_unlock_pair(tty, o_tty);
-	/* At this point the TTY_CLOSING flag should ensure a dead tty
+	/* At this point, the tty->count == 0 should ensure a dead tty
 	   cannot be re-opened by a racing opener */
 
 	/* check whether both sides are closing ... */
diff --git a/include/linux/tty.h b/include/linux/tty.h
index ff0dd7a..35b29f0 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -316,7 +316,6 @@ struct tty_file_private {
 #define TTY_EXCLUSIVE 		3	/* Exclusive open mode */
 #define TTY_DEBUG 		4	/* Debugging */
 #define TTY_DO_WRITE_WAKEUP 	5	/* Call write_wakeup after queuing new */
-#define TTY_CLOSING 		7	/* ->close() in progress */
 #define TTY_LDISC_OPEN	 	11	/* Line discipline is open */
 #define TTY_PTY_LOCK 		16	/* pty private */
 #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
-- 
2.1.1

[PATCH -next 10/27] tty: Don't take tty_mutex for tty count changes

[Peter Hurley]

Holding tty_mutex is no longer required to serialize changes to
the tty_count or to prevent concurrent opens of closing ttys;
tty_lock() is sufficient.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 55f9931..7b40247 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1805,10 +1805,6 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * each iteration we avoid any problems.
 	 */
 	while (1) {
-		/* Guard against races with tty->count changes elsewhere and
-		   opens on /dev/tty */
-
-		mutex_lock(&tty_mutex);
 		tty_lock_pair(tty, o_tty);
 		tty_closing = tty->count <= 1;
 		o_tty_closing = o_tty &&
@@ -1844,7 +1840,6 @@ int tty_release(struct inode *inode, struct file *filp)
 			       __func__, tty_name(tty, buf));
 		}
 		tty_unlock_pair(tty, o_tty);
-		mutex_unlock(&tty_mutex);
 		schedule_timeout_killable(timeout);
 		if (timeout < 120 * HZ)
 			timeout = 2 * timeout + 1;
@@ -1899,7 +1894,6 @@ int tty_release(struct inode *inode, struct file *filp)
 		read_unlock(&tasklist_lock);
 	}
 
-	mutex_unlock(&tty_mutex);
 	tty_unlock_pair(tty, o_tty);
 	/* At this point, the tty->count == 0 should ensure a dead tty
 	   cannot be re-opened by a racing opener */
-- 
2.1.1

[PATCH -next 11/27] tty: Don't release tty locks for wait queue sanity check

[Peter Hurley]

Releasing the tty locks while waiting for the tty wait queues to
be empty is no longer necessary nor desirable. Prior to
"tty: Don't take tty_mutex for tty count changes", dropping the
tty locks was necessary to reestablish the correct lock order between
tty_mutex and the tty locks. Dropping the global tty_mutex was necessary;
otherwise new ttys could not have been opened while waiting.

However, without needing the global tty_mutex held, the tty locks for
the releasing tty can now be held through the sleep. The sanity check
is for abnormal conditions caused by kernel bugs, not for recoverable
errors caused by misbehaving userspace; dropping the tty locks only
allows the tty state to get more sideways.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 7b40247..50118ce 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1799,13 +1799,10 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * first, its count will be one, since the master side holds an open.
 	 * Thus this test wouldn't be triggered at the time the slave closes,
 	 * so we do it now.
-	 *
-	 * Note that it's possible for the tty to be opened again while we're
-	 * flushing out waiters.  By recalculating the closing flags before
-	 * each iteration we avoid any problems.
 	 */
+	tty_lock_pair(tty, o_tty);
+
 	while (1) {
-		tty_lock_pair(tty, o_tty);
 		tty_closing = tty->count <= 1;
 		o_tty_closing = o_tty &&
 			(o_tty->count <= (pty_master ? 1 : 0));
@@ -1839,7 +1836,6 @@ int tty_release(struct inode *inode, struct file *filp)
 			printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
 			       __func__, tty_name(tty, buf));
 		}
-		tty_unlock_pair(tty, o_tty);
 		schedule_timeout_killable(timeout);
 		if (timeout < 120 * HZ)
 			timeout = 2 * timeout + 1;
-- 
2.1.1

Re: [PATCH -next 11/27] tty: Don't release tty locks for wait queue sanity check

[One Thousand Gnomes]

> However, without needing the global tty_mutex held, the tty locks for
> the releasing tty can now be held through the sleep. The sanity check
> is for abnormal conditions caused by kernel bugs, not for recoverable
> errors caused by misbehaving userspace; dropping the tty locks only
> allows the tty state to get more sideways.

An open with O_NDELAY on the closing port now appears to be able to jam
for 2 minutes ? Peviously it would at least be released by a signal.

That seems like a regression (and given the timeout is long) a bug.

Given that some code handles multiple tty devices using select and
nonblocking opens on physical ports this one bothers me a little. The old
behaviour wasn't right either (and actually stops Linux running some
modem manager type tools), but the new behaviour looks worse.

Probably though the right way to fix it is in the open path ?

Alan

Re: [PATCH -next 11/27] tty: Don't release tty locks for wait queue sanity check

[Peter Hurley]

On 10/22/2014 11:29 AM, One Thousand Gnomes wrote:
>> However, without needing the global tty_mutex held, the tty locks for
>> the releasing tty can now be held through the sleep. The sanity check
>> is for abnormal conditions caused by kernel bugs, not for recoverable
>> errors caused by misbehaving userspace; dropping the tty locks only
>> allows the tty state to get more sideways.
> 
> An open with O_NDELAY on the closing port now appears to be able to jam
> for 2 minutes ? Peviously it would at least be released by a signal.
> 
> That seems like a regression (and given the timeout is long) a bug.

This patch should only affect _really abnormal_ situations.

The only way that a tty is spinning in this loop and not getting released
is if the tty count is going to zero but some other thread is still on one
of the wait queues, but that's only possible if either:
1. the other thread never removed itself from the wait queue because it
crashed while on the wait queue, or
2. if somehow a thread is sleeping on one of the wait queues without having
passed through vfs.

IOW, since the tty count is going zero, the release in progress must be
for the last file descriptor for this tty, so how can some other thread
be on one of the wait queues without an in-use descriptor.

Both are serious errors, and the failed sanity test shows that the tty state
is corrupted; an open should not succeed as long as this is true.

It'll take some experimentation to see if the first situation is identifiable
and remediable; I'll put it on my todo list.

> Given that some code handles multiple tty devices using select and
> nonblocking opens on physical ports this one bothers me a little. The old
> behaviour wasn't right either (and actually stops Linux running some
> modem manager type tools), but the new behaviour looks worse.
> 
> Probably though the right way to fix it is in the open path ?

Yes, the tty lock in tty_open() should be interruptible. I've built a matrix
of how open() races with the previous release behavior at different locking
points so that the existing outcome can be replicated (or more easily analyzed
to decide if that's the behavior we want and how/whether to change that
behavior). The sticking point right now is dealing with how ASYNC_HUP_NOTIFY
modifies the outcome of the open. This also entails significant code archaeology.

I'm also exploring making the tty count atomic so that a racing open
can prevent a concurrent release from going to final close, which will
help to minimize the time window that an open will fail with EIO.

But first, I need to push out some more patches that have been unit-tested
(and -- don't laugh -- explore why printk disables interrupts and prevents
cpu migration while calling the console drivers. Seems ok to me...)

Regards,
Peter Hurley

Re: [PATCH -next 11/27] tty: Don't release tty locks for wait queue sanity check

[One Thousand Gnomes]

> (and -- don't laugh -- explore why printk disables interrupts and prevents
> cpu migration while calling the console drivers. Seems ok to me...)

It disables interrupts so that the chance of the oops getting out is
maximised, it locks down some of the other bits to try and stop
interleaved oopses producing garbage when say a 16 way server goes pop and
all the cpus oops at once.

Not sure about the migration 8)

Alan

[PATCH -next 12/27] tty: Document check_tty_count() requires tty_lock held

[Peter Hurley]

Holding the tty_lock() is necessary to prevent concurrent changes
to the tty count that may cause it to differ from the open file
list count. The tty_lock() is already held at all call sites.

NB: Note that the check for the pty master tty count is safe because
the slave's tty_lock() is held while decrementing the pty master
tty count.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 50118ce..5c09cc4 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -275,6 +275,7 @@ int tty_paranoia_check(struct tty_struct *tty, struct inode *inode,
 	return 0;
 }
 
+/* Caller must hold tty_lock */
 static int check_tty_count(struct tty_struct *tty, const char *routine)
 {
 #ifdef CHECK_TTY_COUNT
-- 
2.1.1

[PATCH -next 13/27] tty: Simplify pty pair teardown logic

[Peter Hurley]

When the slave side closes and its tty count is 0, the pty
pair can be destroyed; the master side must have already
closed for the slave side tty count to be 0. Thus, only the
pty master close must check if the slave side has closed by
checking the slave tty count.

Remove the pre-computed closing flags and check the actual count(s).
Regular ttys are unaffected by this change.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 5c09cc4..978896d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1754,7 +1754,7 @@ int tty_release(struct inode *inode, struct file *filp)
 {
 	struct tty_struct *tty = file_tty(filp);
 	struct tty_struct *o_tty;
-	int	pty_master, tty_closing, o_tty_closing, do_sleep;
+	int	pty_master, do_sleep, final;
 	int	idx;
 	char	buf[64];
 	long	timeout = 0;
@@ -1798,18 +1798,15 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * The test for the o_tty closing is necessary, since the master and
 	 * slave sides may close in any order.  If the slave side closes out
 	 * first, its count will be one, since the master side holds an open.
-	 * Thus this test wouldn't be triggered at the time the slave closes,
+	 * Thus this test wouldn't be triggered at the time the slave closed,
 	 * so we do it now.
 	 */
 	tty_lock_pair(tty, o_tty);
 
 	while (1) {
-		tty_closing = tty->count <= 1;
-		o_tty_closing = o_tty &&
-			(o_tty->count <= (pty_master ? 1 : 0));
 		do_sleep = 0;
 
-		if (tty_closing) {
+		if (tty->count <= 1) {
 			if (waitqueue_active(&tty->read_wait)) {
 				wake_up_poll(&tty->read_wait, POLLIN);
 				do_sleep++;
@@ -1819,7 +1816,7 @@ int tty_release(struct inode *inode, struct file *filp)
 				do_sleep++;
 			}
 		}
-		if (o_tty_closing) {
+		if (pty_master && o_tty->count <= 1) {
 			if (waitqueue_active(&o_tty->read_wait)) {
 				wake_up_poll(&o_tty->read_wait, POLLIN);
 				do_sleep++;
@@ -1844,14 +1841,6 @@ int tty_release(struct inode *inode, struct file *filp)
 			timeout = MAX_SCHEDULE_TIMEOUT;
 	}
 
-	/*
-	 * 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) {
 			printk(KERN_WARNING "%s: bad pty slave count (%d) for %s\n",
@@ -1883,20 +1872,22 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * processes that still think tty or o_tty is their controlling
 	 * tty.
 	 */
-	if (tty_closing || o_tty_closing) {
+	if (!tty->count) {
 		read_lock(&tasklist_lock);
 		session_clear_tty(tty->session);
-		if (o_tty)
+		if (pty_master)
 			session_clear_tty(o_tty->session);
 		read_unlock(&tasklist_lock);
 	}
 
+	/* check whether both sides are closing ... */
+	final = !tty->count && !(pty_master && o_tty->count);
+
 	tty_unlock_pair(tty, o_tty);
 	/* At this point, the tty->count == 0 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))
+	if (!final)
 		return 0;
 
 #ifdef TTY_DEBUG_HANGUP
-- 
2.1.1

[PATCH -next 14/27] tty: Fold pty pair handling into tty_flush_works()

[Peter Hurley]

Perform work flush for both ends of a pty pair within tty_flush_works(),
rather than calling twice.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 978896d..6a3be01 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1584,15 +1584,19 @@ void tty_free_termios(struct tty_struct *tty)
 EXPORT_SYMBOL(tty_free_termios);
 
 /**
- *	tty_flush_works		-	flush all works of a tty
- *	@tty: tty device to flush works for
+ *	tty_flush_works		-	flush all works of a tty/pty pair
+ *	@tty: tty device to flush works for (or either end of a pty pair)
  *
- *	Sync flush all works belonging to @tty.
+ *	Sync flush all works belonging to @tty (and the 'other' tty).
  */
 static void tty_flush_works(struct tty_struct *tty)
 {
 	flush_work(&tty->SAK_work);
 	flush_work(&tty->hangup_work);
+	if (tty->link) {
+		flush_work(&tty->link->SAK_work);
+		flush_work(&tty->link->hangup_work);
+	}
 }
 
 /**
@@ -1900,8 +1904,6 @@ int tty_release(struct inode *inode, struct file *filp)
 
 	/* Wait for pending work before tty destruction commmences */
 	tty_flush_works(tty);
-	if (o_tty)
-		tty_flush_works(o_tty);
 
 #ifdef TTY_DEBUG_HANGUP
 	printk(KERN_DEBUG "%s: %s: freeing structure...\n", __func__, tty_name(tty, buf));
-- 
2.1.1

[PATCH -next 15/27] tty: Simplify tty_ldisc_release() interface

[Peter Hurley]

Passing the 'other' tty to tty_ldisc_release() only makes sense
for a pty pair; make o_tty function local instead.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c    |  2 +-
 drivers/tty/tty_ldisc.c | 15 +++++++--------
 include/linux/tty.h     |  2 +-
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 6a3be01..5c782dc 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1900,7 +1900,7 @@ int tty_release(struct inode *inode, struct file *filp)
 	/*
 	 * Ask the line discipline code to release its structures
 	 */
-	tty_ldisc_release(tty, o_tty);
+	tty_ldisc_release(tty);
 
 	/* Wait for pending work before tty destruction commmences */
 	tty_flush_works(tty);
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 49001fa..1c4d7b6 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -756,18 +756,17 @@ static void tty_ldisc_kill(struct tty_struct *tty)
 
 /**
  *	tty_ldisc_release		-	release line discipline
- *	@tty: tty being shut down
- *	@o_tty: pair tty for pty/tty pairs
- *
- *	Called during the final close of a tty/pty pair in order to shut down
- *	the line discpline layer. On exit the ldisc assigned is N_TTY and the
- *	ldisc has not been opened.
+ *	@tty: tty being shut down (or one end of pty pair)
  *
- *	Holding ldisc_sem write lock serializes tty->ldisc changes.
+ *	Called during the final close of a tty or a pty pair in order to shut
+ *	down the line discpline layer. On exit, each ldisc assigned is N_TTY and
+ *	each ldisc has not been opened.
  */
 
-void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
+void tty_ldisc_release(struct tty_struct *tty)
 {
+	struct tty_struct *o_tty = tty->link;
+
 	/*
 	 * Shutdown this line discipline. As this is the final close,
 	 * it does not race with the set_ldisc code path.
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 35b29f0..af1a7f3 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -557,7 +557,7 @@ extern int tty_register_ldisc(int disc, struct tty_ldisc_ops *new_ldisc);
 extern int tty_unregister_ldisc(int disc);
 extern int tty_set_ldisc(struct tty_struct *tty, int ldisc);
 extern int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty);
-extern void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty);
+extern void tty_ldisc_release(struct tty_struct *tty);
 extern void tty_ldisc_init(struct tty_struct *tty);
 extern void tty_ldisc_deinit(struct tty_struct *tty);
 extern void tty_ldisc_begin(void);
-- 
2.1.1

[PATCH -next 16/27] tty: Simplify tty_release_checks() interface

[Peter Hurley]

Passing the 'other' tty to tty_release_checks() only makes sense
for a pty pair; make o_tty scope local instead.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 5c782dc..8fedd22 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1701,8 +1701,7 @@ static void release_tty(struct tty_struct *tty, int idx)
  *	Performs some paranoid checking before true release of the @tty.
  *	This is a no-op unless TTY_PARANOIA_CHECK is defined.
  */
-static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty,
-		int idx)
+static int tty_release_checks(struct tty_struct *tty, int idx)
 {
 #ifdef TTY_PARANOIA_CHECK
 	if (idx < 0 || idx >= tty->driver->num) {
@@ -1721,6 +1720,8 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty,
 		return -1;
 	}
 	if (tty->driver->other) {
+		struct tty_struct *o_tty = tty->link;
+
 		if (o_tty != tty->driver->other->ttys[idx]) {
 			printk(KERN_DEBUG "%s: other->table[%d] not o_tty for (%s)\n",
 					__func__, idx, tty->name);
@@ -1778,7 +1779,7 @@ int tty_release(struct inode *inode, struct file *filp)
 	/* Review: parallel close */
 	o_tty = tty->link;
 
-	if (tty_release_checks(tty, o_tty, idx)) {
+	if (tty_release_checks(tty, idx)) {
 		tty_unlock(tty);
 		return 0;
 	}
-- 
2.1.1

[PATCH -next 17/27] tty: Simplify tty_release() state checks

[Peter Hurley]

The local o_tty variable in tty_release() is now accessed only
when closing the pty master.

Set o_tty to slave pty when closing pty master, otherwise NULL;
use o_tty != NULL as replacement for pty_master.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 8fedd22..fb043b9 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1758,8 +1758,8 @@ static int tty_release_checks(struct tty_struct *tty, int idx)
 int tty_release(struct inode *inode, struct file *filp)
 {
 	struct tty_struct *tty = file_tty(filp);
-	struct tty_struct *o_tty;
-	int	pty_master, do_sleep, final;
+	struct tty_struct *o_tty = NULL;
+	int	do_sleep, final;
 	int	idx;
 	char	buf[64];
 	long	timeout = 0;
@@ -1774,10 +1774,9 @@ int tty_release(struct inode *inode, struct file *filp)
 	__tty_fasync(-1, filp, 0);
 
 	idx = tty->index;
-	pty_master = (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
-		      tty->driver->subtype == PTY_TYPE_MASTER);
-	/* Review: parallel close */
-	o_tty = tty->link;
+	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
+	    tty->driver->subtype == PTY_TYPE_MASTER)
+		o_tty = tty->link;
 
 	if (tty_release_checks(tty, idx)) {
 		tty_unlock(tty);
@@ -1821,7 +1820,7 @@ int tty_release(struct inode *inode, struct file *filp)
 				do_sleep++;
 			}
 		}
-		if (pty_master && o_tty->count <= 1) {
+		if (o_tty && o_tty->count <= 1) {
 			if (waitqueue_active(&o_tty->read_wait)) {
 				wake_up_poll(&o_tty->read_wait, POLLIN);
 				do_sleep++;
@@ -1846,7 +1845,7 @@ int tty_release(struct inode *inode, struct file *filp)
 			timeout = MAX_SCHEDULE_TIMEOUT;
 	}
 
-	if (pty_master) {
+	if (o_tty) {
 		if (--o_tty->count < 0) {
 			printk(KERN_WARNING "%s: bad pty slave count (%d) for %s\n",
 				__func__, o_tty->count, tty_name(o_tty, buf));
@@ -1880,13 +1879,13 @@ int tty_release(struct inode *inode, struct file *filp)
 	if (!tty->count) {
 		read_lock(&tasklist_lock);
 		session_clear_tty(tty->session);
-		if (pty_master)
+		if (o_tty)
 			session_clear_tty(o_tty->session);
 		read_unlock(&tasklist_lock);
 	}
 
 	/* check whether both sides are closing ... */
-	final = !tty->count && !(pty_master && o_tty->count);
+	final = !tty->count && !(o_tty && o_tty->count);
 
 	tty_unlock_pair(tty, o_tty);
 	/* At this point, the tty->count == 0 should ensure a dead tty
-- 
2.1.1

[PATCH -next 18/27] tty: Change tty lock order to master->slave

[Peter Hurley]

When releasing the master pty, the slave pty also needs to be locked
to prevent concurrent tty count changes for the slave pty and to
ensure that only one parallel master and slave release observe the
final close, and proceed to destruct the pty pair. Conversely, when
releasing the slave pty, locking the master pty is not necessary
(since the master's state can be inferred by the slave tty count).

Introduce tty_lock_slave()/tty_unlock_slave() which acquires/releases
the tty lock of the slave pty. Remove tty_lock_pair()/tty_unlock_pair().

Dropping the tty_lock is no longer required to re-establish a stable
lock order.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c    | 10 ++++++----
 drivers/tty/tty_mutex.c | 32 +++++++++++++-------------------
 include/linux/tty.h     |  7 ++-----
 3 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index fb043b9..58015b3 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1791,7 +1791,9 @@ int tty_release(struct inode *inode, struct file *filp)
 	if (tty->ops->close)
 		tty->ops->close(tty, filp);
 
-	tty_unlock(tty);
+	/* If tty is pty master, lock the slave pty (stable lock order) */
+	tty_lock_slave(o_tty);
+
 	/*
 	 * Sanity check: if tty->count is going to zero, there shouldn't be
 	 * any waiters on tty->read_wait or tty->write_wait.  We test the
@@ -1805,8 +1807,6 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * Thus this test wouldn't be triggered at the time the slave closed,
 	 * so we do it now.
 	 */
-	tty_lock_pair(tty, o_tty);
-
 	while (1) {
 		do_sleep = 0;
 
@@ -1887,7 +1887,9 @@ int tty_release(struct inode *inode, struct file *filp)
 	/* check whether both sides are closing ... */
 	final = !tty->count && !(o_tty && o_tty->count);
 
-	tty_unlock_pair(tty, o_tty);
+	tty_unlock_slave(o_tty);
+	tty_unlock(tty);
+
 	/* At this point, the tty->count == 0 should ensure a dead tty
 	   cannot be re-opened by a racing opener */
 
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 2e41abe..f43e995 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -4,6 +4,11 @@
 #include <linux/semaphore.h>
 #include <linux/sched.h>
 
+/*
+ * Nested tty locks are necessary for releasing pty pairs.
+ * The stable lock order is master pty first, then slave pty.
+ */
+
 /* Legacy tty mutex glue */
 
 enum {
@@ -45,29 +50,18 @@ void __lockfunc tty_unlock(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(tty_unlock);
 
-/*
- * Getting the big tty mutex for a pair of ttys with lock ordering
- * On a non pty/tty pair tty2 can be NULL which is just fine.
- */
-void __lockfunc tty_lock_pair(struct tty_struct *tty,
-					struct tty_struct *tty2)
+void __lockfunc tty_lock_slave(struct tty_struct *tty)
 {
-	if (tty < tty2) {
-		tty_lock(tty);
-		tty_lock_nested(tty2, TTY_MUTEX_NESTED);
-	} else {
-		if (tty2 && tty2 != tty)
-			tty_lock(tty2);
+	if (tty && tty != tty->link) {
+		WARN_ON(!mutex_is_locked(&tty->link->legacy_mutex) ||
+			!tty->driver->type == TTY_DRIVER_TYPE_PTY ||
+			!tty->driver->type == PTY_TYPE_SLAVE);
 		tty_lock_nested(tty, TTY_MUTEX_NESTED);
 	}
 }
-EXPORT_SYMBOL(tty_lock_pair);
 
-void __lockfunc tty_unlock_pair(struct tty_struct *tty,
-						struct tty_struct *tty2)
+void __lockfunc tty_unlock_slave(struct tty_struct *tty)
 {
-	tty_unlock(tty);
-	if (tty2 && tty2 != tty)
-		tty_unlock(tty2);
+	if (tty && tty != tty->link)
+		tty_unlock(tty);
 }
-EXPORT_SYMBOL(tty_unlock_pair);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index af1a7f3..a07b4b4 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -638,11 +638,8 @@ extern long vt_compat_ioctl(struct tty_struct *tty,
 /* functions for preparation of BKL removal */
 extern void __lockfunc tty_lock(struct tty_struct *tty);
 extern void __lockfunc tty_unlock(struct tty_struct *tty);
-extern void __lockfunc tty_lock_pair(struct tty_struct *tty,
-				struct tty_struct *tty2);
-extern void __lockfunc tty_unlock_pair(struct tty_struct *tty,
-				struct tty_struct *tty2);
-
+extern void __lockfunc tty_lock_slave(struct tty_struct *tty);
+extern void __lockfunc tty_unlock_slave(struct tty_struct *tty);
 /*
  * this shall be called only from where BTM is held (like close)
  *
-- 
2.1.1

[PATCH -next 19/27] tty: Remove tty_unhangup() declaration

[Peter Hurley]

The tty_unhangup() function is not defined.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 include/linux/tty.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/tty.h b/include/linux/tty.h
index a07b4b4..d470a86 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -435,7 +435,6 @@ extern int is_ignored(int sig);
 extern int tty_signal(int sig, struct tty_struct *tty);
 extern void tty_hangup(struct tty_struct *tty);
 extern void tty_vhangup(struct tty_struct *tty);
-extern void tty_unhangup(struct file *filp);
 extern int tty_hung_up_p(struct file *filp);
 extern void do_SAK(struct tty_struct *tty);
 extern void __do_SAK(struct tty_struct *tty);
-- 
2.1.1

[PATCH -next 20/27] tty: Refactor __tty_hangup to enable lockdep annotation

[Peter Hurley]

Refactor __tty_hangup() into:
1. __tty_hangup_common(), the portion requiring the tty lock
2. __tty_hangup(), which performs the pre- and post-lock processing
   (TIOCCONS redirect undo) and calls through a function ptr parameter
   to lock/hangup/unlock
3. __tty_hangup_standard(), which performs the lock/hangup/unlock

Allows an alternate function to lock/hangup/unlock with the
nested tty lock.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 51 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 58015b3..48c1def 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -665,32 +665,17 @@ static int tty_signal_session_leader(struct tty_struct *tty, int exit_session)
  *		  tasklist_lock to walk task list for hangup event
  *		    ->siglock to protect ->signal/->sighand
  */
-static void __tty_hangup(struct tty_struct *tty, int exit_session)
+static void __tty_hangup_common(struct tty_struct *tty, int exit_session)
 {
 	struct file *cons_filp = NULL;
-	struct file *filp, *f = NULL;
+	struct file *filp;
 	struct tty_file_private *priv;
 	int    closecount = 0, n;
 	int refs;
 
-	if (!tty)
+	if (test_bit(TTY_HUPPED, &tty->flags))
 		return;
 
-
-	spin_lock(&redirect_lock);
-	if (redirect && file_tty(redirect) == tty) {
-		f = redirect;
-		redirect = NULL;
-	}
-	spin_unlock(&redirect_lock);
-
-	tty_lock(tty);
-
-	if (test_bit(TTY_HUPPED, &tty->flags)) {
-		tty_unlock(tty);
-		return;
-	}
-
 	/* inuse_filps is protected by the single tty lock,
 	   this really needs to change if we want to flush the
 	   workqueue with the lock held */
@@ -746,7 +731,31 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 	 * can't yet guarantee all that.
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
+}
+
+static void __tty_hangup_standard(struct tty_struct* tty, int exit_session)
+{
+	tty_lock(tty);
+	__tty_hangup_common(tty, exit_session);
 	tty_unlock(tty);
+}
+
+static void __tty_hangup(struct tty_struct *tty, int exit_session,
+			 void (*hangup)(struct tty_struct *, int))
+{
+	struct file *f = NULL;
+
+	if (!tty)
+		return;
+
+	spin_lock(&redirect_lock);
+	if (redirect && file_tty(redirect) == tty) {
+		f = redirect;
+		redirect = NULL;
+	}
+	spin_unlock(&redirect_lock);
+
+	hangup(tty, exit_session);
 
 	if (f)
 		fput(f);
@@ -757,7 +766,7 @@ static void do_tty_hangup(struct work_struct *work)
 	struct tty_struct *tty =
 		container_of(work, struct tty_struct, hangup_work);
 
-	__tty_hangup(tty, 0);
+	__tty_hangup(tty, 0, __tty_hangup_standard);
 }
 
 /**
@@ -795,7 +804,7 @@ void tty_vhangup(struct tty_struct *tty)
 
 	printk(KERN_DEBUG "%s vhangup...\n", tty_name(tty, buf));
 #endif
-	__tty_hangup(tty, 0);
+	__tty_hangup(tty, 0, __tty_hangup_standard);
 }
 
 EXPORT_SYMBOL(tty_vhangup);
@@ -836,7 +845,7 @@ static void tty_vhangup_session(struct tty_struct *tty)
 
 	printk(KERN_DEBUG "%s vhangup session...\n", tty_name(tty, buf));
 #endif
-	__tty_hangup(tty, 1);
+	__tty_hangup(tty, 1, __tty_hangup_standard);
 }
 
 /**
-- 
2.1.1

Re: [PATCH -next 20/27] tty: Refactor __tty_hangup to enable lockdep annotation

[Peter Hurley]

On 10/16/2014 04:25 PM, Peter Hurley wrote:
> Refactor __tty_hangup() into:
> 1. __tty_hangup_common(), the portion requiring the tty lock
> 2. __tty_hangup(), which performs the pre- and post-lock processing
>    (TIOCCONS redirect undo) and calls through a function ptr parameter
>    to lock/hangup/unlock
> 3. __tty_hangup_standard(), which performs the lock/hangup/unlock
> 
> Allows an alternate function to lock/hangup/unlock with the
> nested tty lock.

I just discovered that lockdep provides an interface for setting the
lock subclass after lock initialization. Which means that the lock
subclass can be changed just for slave ptys, which allows lock nesting
without specifying the subclass at lock time. Which eliminates the need
for this patch and the follow-on.

I'll respin this series.

Regards,
Peter Hurley

[PATCH -next 21/27] pty: Don't drop pty master tty lock to hangup slave

[Peter Hurley]

Introduce tty_vhangup_slave(), which takes tty_lock_slave() instead
of tty_lock(), and thus is callable while holding the pty master
tty lock.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c    |  4 +---
 drivers/tty/tty_io.c | 20 ++++++++++++++++++++
 include/linux/tty.h  |  1 +
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index bdb8fd1..09fb513 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -66,9 +66,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 			mutex_unlock(&devpts_mutex);
 		}
 #endif
-		tty_unlock(tty);
-		tty_vhangup(tty->link);
-		tty_lock(tty);
+		tty_vhangup_slave(tty->link);
 	}
 }
 
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 48c1def..25e85b0 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -809,6 +809,26 @@ void tty_vhangup(struct tty_struct *tty)
 
 EXPORT_SYMBOL(tty_vhangup);
 
+/**
+ *	tty_vhangup_slave	-	process vhangup for pty slave
+ *	@tty: pty slave to hangup
+ *
+ *	Identical to tty_vhangup(), except can specifically be used
+ *	to synchronously hangup a pty slave while holding pty master
+ *	tty lock.
+ */
+
+static void __tty_vhangup_slave(struct tty_struct *tty, int exit_session)
+{
+	tty_lock_slave(tty);
+	__tty_hangup_common(tty, 0);
+	tty_unlock_slave(tty);
+}
+
+void tty_vhangup_slave(struct tty_struct *tty)
+{
+	__tty_hangup(tty, 0, __tty_vhangup_slave);
+}
 
 /**
  *	tty_vhangup_self	-	process vhangup for own ctty
diff --git a/include/linux/tty.h b/include/linux/tty.h
index d470a86..8a90253 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -435,6 +435,7 @@ extern int is_ignored(int sig);
 extern int tty_signal(int sig, struct tty_struct *tty);
 extern void tty_hangup(struct tty_struct *tty);
 extern void tty_vhangup(struct tty_struct *tty);
+extern void tty_vhangup_slave(struct tty_struct *tty);
 extern int tty_hung_up_p(struct file *filp);
 extern void do_SAK(struct tty_struct *tty);
 extern void __do_SAK(struct tty_struct *tty);
-- 
2.1.1

[PATCH -next 22/27] tty: Document hangup call tree

[Peter Hurley]

Add at-a-glance call tree for the various hangup methods.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 25e85b0..8effd44 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -664,6 +664,19 @@ static int tty_signal_session_leader(struct tty_struct *tty, int exit_session)
  *		  termios_rwsem resetting termios data
  *		  tasklist_lock to walk task list for hangup event
  *		    ->siglock to protect ->signal/->sighand
+ *
+ *	Call tree:
+ *	tty_hangup() =>	do_tty_hangup() -+
+ *					 |
+ *	tty_vhangup() ---------+	 |
+ *	tty_vhangup_self() ----+	 |
+ *	tty_vhangup_session() -+---------+
+ *      				 |
+ *				  __tty_hangup() -> __tty_hangup_standard() -+
+ *									     |
+ *      tty_vhangup_slave() ----> __tty_hangup() -> __tty_vhangup_slave() ---+
+ *									     |
+ *								 __tty_hangup_common()
  */
 static void __tty_hangup_common(struct tty_struct *tty, int exit_session)
 {
-- 
2.1.1

[PATCH -next 23/27] pty, n_tty: Simplify input processing on final close

[Peter Hurley]

When releasing one end of a pty pair, that end may just have written
to the other, which the input processing worker, flush_to_ldisc(), is
still working on but has not completed the copy to the other end's
read buffer. So input may not appear to be available to a waiting
reader but yet TTY_OTHER_CLOSED is now observed. The n_tty line
discipline has worked around this by waiting for input processing
to complete and then re-checking if input is available before
exiting with -EIO.

Since the tty/ldisc lock reordering, the wait for input processing
to complete can now occur during final close before setting
TTY_OTHER_CLOSED. In this way, a waiting reader is guaranteed to
see input available (if any) before observing TTY_OTHER_CLOSED.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 46 ++++++++++++++++++++--------------------------
 drivers/tty/pty.c   |  1 +
 2 files changed, 21 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 38ac7b9..a8ec82d 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2197,34 +2197,28 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 
 		if (!input_available_p(tty, 0)) {
 			if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
-				up_read(&tty->termios_rwsem);
-				tty_flush_to_ldisc(tty);
-				down_read(&tty->termios_rwsem);
-				if (!input_available_p(tty, 0)) {
-					retval = -EIO;
-					break;
-				}
-			} else {
-				if (tty_hung_up_p(file))
-					break;
-				if (!timeout)
-					break;
-				if (file->f_flags & O_NONBLOCK) {
-					retval = -EAGAIN;
-					break;
-				}
-				if (signal_pending(current)) {
-					retval = -ERESTARTSYS;
-					break;
-				}
-				n_tty_set_room(tty);
-				up_read(&tty->termios_rwsem);
+				retval = -EIO;
+				break;
+			}
+			if (tty_hung_up_p(file))
+				break;
+			if (!timeout)
+				break;
+			if (file->f_flags & O_NONBLOCK) {
+				retval = -EAGAIN;
+				break;
+			}
+			if (signal_pending(current)) {
+				retval = -ERESTARTSYS;
+				break;
+			}
+			n_tty_set_room(tty);
+			up_read(&tty->termios_rwsem);
 
-				timeout = schedule_timeout(timeout);
+			timeout = schedule_timeout(timeout);
 
-				down_read(&tty->termios_rwsem);
-				continue;
-			}
+			down_read(&tty->termios_rwsem);
+			continue;
 		}
 		__set_current_state(TASK_RUNNING);
 
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 09fb513..461ca53 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -53,6 +53,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 	/* Review - krefs on tty_link ?? */
 	if (!tty->link)
 		return;
+	tty_flush_to_ldisc(tty->link);
 	set_bit(TTY_OTHER_CLOSED, &tty->link->flags);
 	wake_up_interruptible(&tty->link->read_wait);
 	wake_up_interruptible(&tty->link->write_wait);
-- 
2.1.1

[PATCH -next 24/27] tty: Prefix tty_ldisc_{lock,lock_nested,unlock} functions

[Peter Hurley]

tty_ldisc_lock(), tty_ldisc_unlock(), and tty_ldisc_lock_nested()
are low-level aliases for the underlying lock mechanism. Rename
with double underscore to allow for new, higher level functions
with those names.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 1c4d7b6..5bdc241 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -308,19 +308,19 @@ EXPORT_SYMBOL_GPL(tty_ldisc_deref);
 
 
 static inline int __lockfunc
-tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
+__tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
 {
 	return ldsem_down_write(&tty->ldisc_sem, timeout);
 }
 
 static inline int __lockfunc
-tty_ldisc_lock_nested(struct tty_struct *tty, unsigned long timeout)
+__tty_ldisc_lock_nested(struct tty_struct *tty, unsigned long timeout)
 {
 	return ldsem_down_write_nested(&tty->ldisc_sem,
 				       LDISC_SEM_OTHER, timeout);
 }
 
-static inline void tty_ldisc_unlock(struct tty_struct *tty)
+static inline void __tty_ldisc_unlock(struct tty_struct *tty)
 {
 	return ldsem_up_write(&tty->ldisc_sem);
 }
@@ -332,24 +332,24 @@ tty_ldisc_lock_pair_timeout(struct tty_struct *tty, struct tty_struct *tty2,
 	int ret;
 
 	if (tty < tty2) {
-		ret = tty_ldisc_lock(tty, timeout);
+		ret = __tty_ldisc_lock(tty, timeout);
 		if (ret) {
-			ret = tty_ldisc_lock_nested(tty2, timeout);
+			ret = __tty_ldisc_lock_nested(tty2, timeout);
 			if (!ret)
-				tty_ldisc_unlock(tty);
+				__tty_ldisc_unlock(tty);
 		}
 	} else {
 		/* if this is possible, it has lots of implications */
 		WARN_ON_ONCE(tty == tty2);
 		if (tty2 && tty != tty2) {
-			ret = tty_ldisc_lock(tty2, timeout);
+			ret = __tty_ldisc_lock(tty2, timeout);
 			if (ret) {
-				ret = tty_ldisc_lock_nested(tty, timeout);
+				ret = __tty_ldisc_lock_nested(tty, timeout);
 				if (!ret)
-					tty_ldisc_unlock(tty2);
+					__tty_ldisc_unlock(tty2);
 			}
 		} else
-			ret = tty_ldisc_lock(tty, timeout);
+			ret = __tty_ldisc_lock(tty, timeout);
 	}
 
 	if (!ret)
@@ -370,9 +370,9 @@ tty_ldisc_lock_pair(struct tty_struct *tty, struct tty_struct *tty2)
 static void __lockfunc tty_ldisc_unlock_pair(struct tty_struct *tty,
 					     struct tty_struct *tty2)
 {
-	tty_ldisc_unlock(tty);
+	__tty_ldisc_unlock(tty);
 	if (tty2)
-		tty_ldisc_unlock(tty2);
+		__tty_ldisc_unlock(tty2);
 }
 
 static void __lockfunc tty_ldisc_enable_pair(struct tty_struct *tty,
-- 
2.1.1

[PATCH -next 25/27] tty: Fix hung task on pty hangup

[Peter Hurley]

When hanging up one end of a pty pair, there may be waiting
readers/writers on the other end which may not exit, preventing
tty_ldisc_lock_pair() from acquiring the other side's ldisc lock.

Only acquire this side's ldisc lock; although this will no longer
prevent the other side from writing new input, that input will not
be processing until after the ldisc hangup is complete.

Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 5bdc241..1dbe278 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -326,6 +326,24 @@ static inline void __tty_ldisc_unlock(struct tty_struct *tty)
 }
 
 static int __lockfunc
+tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
+{
+	int ret;
+
+	ret = __tty_ldisc_lock(tty, timeout);
+	if (!ret)
+		return -EBUSY;
+	set_bit(TTY_LDISC_HALTED, &tty->flags);
+	return 0;
+}
+
+static void tty_ldisc_unlock(struct tty_struct *tty)
+{
+	clear_bit(TTY_LDISC_HALTED, &tty->flags);
+	__tty_ldisc_unlock(tty);
+}
+
+static int __lockfunc
 tty_ldisc_lock_pair_timeout(struct tty_struct *tty, struct tty_struct *tty2,
 			    unsigned long timeout)
 {
@@ -682,7 +700,7 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	 *
 	 * Avoid racing set_ldisc or tty_ldisc_release
 	 */
-	tty_ldisc_lock_pair(tty, tty->link);
+	tty_ldisc_lock(tty, MAX_SCHEDULE_TIMEOUT);
 
 	if (tty->ldisc) {
 
@@ -704,7 +722,7 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 			WARN_ON(tty_ldisc_open(tty, tty->ldisc));
 		}
 	}
-	tty_ldisc_enable_pair(tty, tty->link);
+	tty_ldisc_unlock(tty);
 	if (reset)
 		tty_reset_termios(tty);
 
-- 
2.1.1

[PATCH -next 26/27] tty: Fix timeout on pty set ldisc

[Peter Hurley]

When changing the ldisc on one end of a pty pair, there may be
waiting readers/writers on the other end which may not exit from
the ldisc i/o loop, preventing tty_ldisc_lock_pair_timeout() from
acquiring the other side's ldisc lock.

Only acquire this side's ldisc lock; although this will no longer
prevent the other side from writing new input, that input will not
be processed until after the ldisc change completes. This has no
effect on normal ttys; new input from the driver was never disabled.

Remove tty_ldisc_enable_pair().

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 1dbe278..6368dd9 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -393,16 +393,6 @@ static void __lockfunc tty_ldisc_unlock_pair(struct tty_struct *tty,
 		__tty_ldisc_unlock(tty2);
 }
 
-static void __lockfunc tty_ldisc_enable_pair(struct tty_struct *tty,
-					     struct tty_struct *tty2)
-{
-	clear_bit(TTY_LDISC_HALTED, &tty->flags);
-	if (tty2)
-		clear_bit(TTY_LDISC_HALTED, &tty2->flags);
-
-	tty_ldisc_unlock_pair(tty, tty2);
-}
-
 /**
  *	tty_ldisc_flush	-	flush line discipline queue
  *	@tty: tty
@@ -535,14 +525,13 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 {
 	int retval;
 	struct tty_ldisc *old_ldisc, *new_ldisc;
-	struct tty_struct *o_tty = tty->link;
 
 	new_ldisc = tty_ldisc_get(tty, ldisc);
 	if (IS_ERR(new_ldisc))
 		return PTR_ERR(new_ldisc);
 
 	tty_lock(tty);
-	retval = tty_ldisc_lock_pair_timeout(tty, o_tty, 5 * HZ);
+	retval = tty_ldisc_lock(tty, 5 * HZ);
 	if (retval) {
 		tty_ldisc_put(new_ldisc);
 		tty_unlock(tty);
@@ -554,7 +543,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	 */
 
 	if (tty->ldisc->ops->num == ldisc) {
-		tty_ldisc_enable_pair(tty, o_tty);
+		tty_ldisc_unlock(tty);
 		tty_ldisc_put(new_ldisc);
 		tty_unlock(tty);
 		return 0;
@@ -565,7 +554,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	if (test_bit(TTY_HUPPED, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
 		   the ldisc data and closed the ldisc down */
-		tty_ldisc_enable_pair(tty, o_tty);
+		tty_ldisc_unlock(tty);
 		tty_ldisc_put(new_ldisc);
 		tty_unlock(tty);
 		return -EIO;
@@ -599,13 +588,11 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	/*
 	 *	Allow ldisc referencing to occur again
 	 */
-	tty_ldisc_enable_pair(tty, o_tty);
+	tty_ldisc_unlock(tty);
 
 	/* Restart the work queue in case no characters kick it off. Safe if
 	   already running */
 	schedule_work(&tty->port->buf.work);
-	if (o_tty)
-		schedule_work(&o_tty->port->buf.work);
 
 	tty_unlock(tty);
 	return retval;
-- 
2.1.1

[PATCH -next 27/27] tty: Flush ldisc buffer atomically with tty flip buffers

[Peter Hurley]

tty_ldisc_flush() first clears the line discipline input buffer,
then clears the tty flip buffers. However, this allows for existing
data in the tty flip buffers to be added after the ldisc input
buffer has been cleared, but before the flip buffers have been cleared.

Add an optional ldisc parameter to tty_buffer_flush() to allow
tty_ldisc_flush() to pass the ldisc to clear.

NB: Initially, the plan was to do this automatically in
tty_buffer_flush(). However, an audit of the behavior of existing
line disciplines showed that performing a ldisc buffer flush on
ioctl(TCFLSH) was not always the outcome. For example, some line
disciplines have flush_buffer() methods but not ioctl() methods,
so a ->flush_buffer() command would be unexpected.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_buffer.c | 10 ++++++++--
 drivers/tty/tty_io.c     |  2 +-
 drivers/tty/tty_ldisc.c  | 12 +++++-------
 include/linux/tty.h      |  2 +-
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 143deb6..3605103 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -202,14 +202,16 @@ static void tty_buffer_free(struct tty_port *port, struct tty_buffer *b)
 /**
  *	tty_buffer_flush		-	flush full tty buffers
  *	@tty: tty to flush
+ *	@ld:  optional ldisc ptr (must be referenced)
  *
- *	flush all the buffers containing receive data.
+ *	flush all the buffers containing receive data. If ld != NULL,
+ *	flush the ldisc input buffer.
  *
  *	Locking: takes buffer lock to ensure single-threaded flip buffer
  *		 'consumer'
  */
 
-void tty_buffer_flush(struct tty_struct *tty)
+void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
 {
 	struct tty_port *port = tty->port;
 	struct tty_bufhead *buf = &port->buf;
@@ -223,6 +225,10 @@ void tty_buffer_flush(struct tty_struct *tty)
 		buf->head = next;
 	}
 	buf->head->read = buf->head->commit;
+
+	if (ld && ld->ops->flush_buffer)
+		ld->ops->flush_buffer(tty);
+
 	atomic_dec(&buf->priority);
 	mutex_unlock(&buf->lock);
 }
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 8effd44..2931f42 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2939,7 +2939,7 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		case TCIFLUSH:
 		case TCIOFLUSH:
 		/* flush tty buffer and allow ldisc to process ioctl */
-			tty_buffer_flush(tty);
+			tty_buffer_flush(tty, NULL);
 			break;
 		}
 		break;
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 6368dd9..b66a81d 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -397,19 +397,17 @@ static void __lockfunc tty_ldisc_unlock_pair(struct tty_struct *tty,
  *	tty_ldisc_flush	-	flush line discipline queue
  *	@tty: tty
  *
- *	Flush the line discipline queue (if any) for this tty. If there
- *	is no line discipline active this is a no-op.
+ *	Flush the line discipline queue (if any) and the tty flip buffers
+ *	for this tty.
  */
 
 void tty_ldisc_flush(struct tty_struct *tty)
 {
 	struct tty_ldisc *ld = tty_ldisc_ref(tty);
-	if (ld) {
-		if (ld->ops->flush_buffer)
-			ld->ops->flush_buffer(tty);
+
+	tty_buffer_flush(tty, ld);
+	if (ld)
 		tty_ldisc_deref(ld);
-	}
-	tty_buffer_flush(tty);
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_flush);
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 8a90253..90256ee 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -442,7 +442,7 @@ extern void __do_SAK(struct tty_struct *tty);
 extern void no_tty(void);
 extern void tty_flush_to_ldisc(struct tty_struct *tty);
 extern void tty_buffer_free_all(struct tty_port *port);
-extern void tty_buffer_flush(struct tty_struct *tty);
+extern void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld);
 extern void tty_buffer_init(struct tty_port *port);
 extern speed_t tty_termios_baud_rate(struct ktermios *termios);
 extern speed_t tty_termios_input_baud_rate(struct ktermios *termios);
-- 
2.1.1

Re: [PATCH -next 00/27] tty locking changes

[One Thousand Gnomes]

On Thu, 16 Oct 2014 16:24:58 -0400
Peter Hurley <peter@hurleysoftware.com> wrote:

> Hi Greg,
> 
> This patch series has 3 major changes to how tty locking behaves:
> 1. the lock order of tty_lock() and tty->ldisc_sem is reversed;
>    this eliminates a bunch of lock drop/reacquire which, in turn,
>    eliminates tty state tracking that can no longer be observed.
>    This also allows the pty driver to wait for input processing to
>    complete while closing before setting TTY_OTHER_CLOSED (which
>    eliminates the ugliness of checking input twice in n_tty_read() and
>    n_tty_poll()).
> 2. the footprint of tty_mutex is reduced to only adding and removing
>    ttys and is no longer held to acquire the tty_lock() in tty_open();
>    this allows for multiple ttys to be opened concurrently, even if
>    one open stalls waiting for its tty_lock().
> 3. pty pair locking is reordered to master first, then slave, rather
>    than by address. This works because, while releasing the master pty,
>    the slave tty count needs to be changed, whereas, when releasing the
>    slave, the master pty does not need to be accessed.
>    This furthur eliminates more lock drop/reacquire.
> 
> The longer-term goals, which this series builds towards, is:
> 1. simplifying the tty open/close behavior
> 2. eliminating the ASYNC_CLOSING code without breaking existing userspace
> 3. eliminating returning -EIO from tty_open(). Not sure if this is possible yet.


Reviewed-by: Alan Cox <alan@linux.intel.com>

[PATCH -next v2 00/26] tty locking changes

[Peter Hurley]

Changes in v2:
* New patch 19 ('tty: Preset lock subclass for nested tty locks') makes
  passing the tty_lock subclass at lock time unnecessary; regular ttys and
  master ptys use subclass 0 and slave ptys use subclass 1. The expected
  lock order is first subclass 0 then subclass 1. Lockdep will warn if
  the reverse order is used.

* That makes introducing tty_hangup_slave and refactoring tty_hangup et al
  unnecessary, so the previous patch 20 ('tty: Refactor __tty_hangup to
  enable lockdep annotation') was removed, along with 'tty: Document hangup
  call tree'.

* Added Alan's Reviewed-by tag to the patches Alan already reviewed,
  which just leaves the following patches needing re-review:
     19/26 tty: Preset lock subclass for nested tty locks
     21/26 pty: Don't drop pty master tty lock to hangup slave

I forgot to point this out in v1; this series depends on
'tty: Fix high cpu load if tty is unreleasable'
'tty: Prevent "read/write wait queue active!" log flooding'

==
Hi Greg,

This patch series has 3 major changes to how tty locking behaves:
1. the lock order of tty_lock() and tty->ldisc_sem is reversed;
   this eliminates a bunch of lock drop/reacquire which, in turn,
   eliminates tty state tracking that can no longer be observed.
   This also allows the pty driver to wait for input processing to
   complete while closing before setting TTY_OTHER_CLOSED (which
   eliminates the ugliness of checking input twice in n_tty_read() and
   n_tty_poll()).
2. the footprint of tty_mutex is reduced to only adding and removing
   ttys and is no longer held to acquire the tty_lock() in tty_open();
   this allows for multiple ttys to be opened concurrently, even if
   one open stalls waiting for its tty_lock().
3. pty pair locking is reordered to master first, then slave, rather
   than by address. This works because, while releasing the master pty,
   the slave tty count needs to be changed, whereas, when releasing the
   slave, the master pty does not need to be accessed.
   This furthur eliminates more lock drop/reacquire.

The longer-term goals, which this series builds towards, is:
1. simplifying the tty open/close behavior
2. eliminating the ASYNC_CLOSING code without breaking existing userspace
3. eliminating returning -EIO from tty_open(). Not sure if this is possible yet.

Regards,

Peter Hurley (26):
  tty: Don't hold tty_lock for ldisc release
  tty: Invert tty_lock/ldisc_sem lock order
  tty: Remove TTY_HUPPING
  tty: Clarify re-open behavior of master ptys
  tty: Check tty->count instead of TTY_CLOSING in tty_reopen()
  pty: Always return -EIO if slave BSD pty opened first
  tty: Re-open /dev/tty without tty_mutex
  tty: Drop tty_mutex before tty reopen
  tty: Remove TTY_CLOSING
  tty: Don't take tty_mutex for tty count changes
  tty: Don't release tty locks for wait queue sanity check
  tty: Document check_tty_count() requires tty_lock held
  tty: Simplify pty pair teardown logic
  tty: Fold pty pair handling into tty_flush_works()
  tty: Simplify tty_ldisc_release() interface
  tty: Simplify tty_release_checks() interface
  tty: Simplify tty_release() state checks
  tty: Change tty lock order to master->slave
  tty: Preset lock subclass for nested tty locks
  tty: Remove tty_unhangup() declaration
  pty: Don't drop pty master tty lock to hangup slave
  pty, n_tty: Simplify input processing on final close
  tty: Prefix tty_ldisc_{lock,lock_nested,unlock} functions
  tty: Fix hung task on pty hangup
  tty: Fix timeout on pty set ldisc
  tty: Flush ldisc buffer atomically with tty flip buffers

 drivers/tty/n_tty.c      |  46 +++++------
 drivers/tty/pty.c        |  12 ++-
 drivers/tty/tty_buffer.c |  10 ++-
 drivers/tty/tty_io.c     | 195 ++++++++++++++++++++---------------------------
 drivers/tty/tty_ldisc.c  | 106 +++++++++++++-------------
 drivers/tty/tty_mutex.c  |  49 +++++-------
 include/linux/tty.h      |  15 ++--
 7 files changed, 196 insertions(+), 237 deletions(-)

-- 
2.1.3

[PATCH -next v2 01/26] tty: Don't hold tty_lock for ldisc release

[Peter Hurley]

The tty->ldisc_sem write lock is sufficient for serializing changes
to tty->ldisc; holding the tty lock is not required.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 2d822aa..332a622 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -764,6 +764,8 @@ static void tty_ldisc_kill(struct tty_struct *tty)
  *	Called during the final close of a tty/pty pair in order to shut down
  *	the line discpline layer. On exit the ldisc assigned is N_TTY and the
  *	ldisc has not been opened.
+ *
+ *	Holding ldisc_sem write lock serializes tty->ldisc changes.
  */
 
 void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
@@ -776,13 +778,9 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
 	tty_ldisc_debug(tty, "closing ldisc: %p\n", tty->ldisc);
 
 	tty_ldisc_lock_pair(tty, o_tty);
-	tty_lock_pair(tty, o_tty);
-
 	tty_ldisc_kill(tty);
 	if (o_tty)
 		tty_ldisc_kill(o_tty);
-
-	tty_unlock_pair(tty, o_tty);
 	tty_ldisc_unlock_pair(tty, o_tty);
 
 	/* And the memory resources remaining (buffers, termios) will be
-- 
2.1.3

[PATCH -next v2 02/26] tty: Invert tty_lock/ldisc_sem lock order

[Peter Hurley]

Dropping the tty lock to acquire the tty->ldisc_sem allows several
race conditions (such as hangup while changing the ldisc) which requires
extra states and testing. The ldisc_sem->tty_lock lock order has
not been required since tty buffer ownership was moved to tty_port.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 332a622..28858eb 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -523,9 +523,11 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	if (IS_ERR(new_ldisc))
 		return PTR_ERR(new_ldisc);
 
+	tty_lock(tty);
 	retval = tty_ldisc_lock_pair_timeout(tty, o_tty, 5 * HZ);
 	if (retval) {
 		tty_ldisc_put(new_ldisc);
+		tty_unlock(tty);
 		return retval;
 	}
 
@@ -536,11 +538,11 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	if (tty->ldisc->ops->num == ldisc) {
 		tty_ldisc_enable_pair(tty, o_tty);
 		tty_ldisc_put(new_ldisc);
+		tty_unlock(tty);
 		return 0;
 	}
 
 	old_ldisc = tty->ldisc;
-	tty_lock(tty);
 
 	if (test_bit(TTY_HUPPING, &tty->flags) ||
 	    test_bit(TTY_HUPPED, &tty->flags)) {
@@ -675,8 +677,6 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	wake_up_interruptible_poll(&tty->write_wait, POLLOUT);
 	wake_up_interruptible_poll(&tty->read_wait, POLLIN);
 
-	tty_unlock(tty);
-
 	/*
 	 * Shutdown the current line discipline, and reset it to
 	 * N_TTY if need be.
@@ -684,7 +684,6 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	 * Avoid racing set_ldisc or tty_ldisc_release
 	 */
 	tty_ldisc_lock_pair(tty, tty->link);
-	tty_lock(tty);
 
 	if (tty->ldisc) {
 
-- 
2.1.3

[PATCH -next v2 03/26] tty: Remove TTY_HUPPING

[Peter Hurley]

Now that tty_ldisc_hangup() does not drop the tty lock, it is no
longer possible to observe TTY_HUPPING while holding the tty lock
on another cpu.

Remove TTY_HUPPING bit definition.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c    | 12 +-----------
 drivers/tty/tty_ldisc.c |  3 +--
 include/linux/tty.h     |  1 -
 3 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 45cc09e..bb4d2f2 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -690,9 +690,6 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 		return;
 	}
 
-	/* some functions below drop BTM, so we need this bit */
-	set_bit(TTY_HUPPING, &tty->flags);
-
 	/* inuse_filps is protected by the single tty lock,
 	   this really needs to change if we want to flush the
 	   workqueue with the lock held */
@@ -717,10 +714,6 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 	while (refs--)
 		tty_kref_put(tty);
 
-	/*
-	 * it drops BTM and thus races with reopen
-	 * we protect the race by TTY_HUPPING
-	 */
 	tty_ldisc_hangup(tty);
 
 	spin_lock_irq(&tty->ctrl_lock);
@@ -752,8 +745,6 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 	 * can't yet guarantee all that.
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
-	clear_bit(TTY_HUPPING, &tty->flags);
-
 	tty_unlock(tty);
 
 	if (f)
@@ -1461,8 +1452,7 @@ static int tty_reopen(struct tty_struct *tty)
 {
 	struct tty_driver *driver = tty->driver;
 
-	if (test_bit(TTY_CLOSING, &tty->flags) ||
-			test_bit(TTY_HUPPING, &tty->flags))
+	if (test_bit(TTY_CLOSING, &tty->flags))
 		return -EIO;
 
 	if (driver->type == TTY_DRIVER_TYPE_PTY &&
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 28858eb..49001fa 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -544,8 +544,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	old_ldisc = tty->ldisc;
 
-	if (test_bit(TTY_HUPPING, &tty->flags) ||
-	    test_bit(TTY_HUPPED, &tty->flags)) {
+	if (test_bit(TTY_HUPPED, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
 		   the ldisc data and closed the ldisc down */
 		tty_ldisc_enable_pair(tty, o_tty);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index b36b0b4..ff0dd7a 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -321,7 +321,6 @@ struct tty_file_private {
 #define TTY_PTY_LOCK 		16	/* pty private */
 #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
 #define TTY_HUPPED 		18	/* Post driver->hangup() */
-#define TTY_HUPPING 		21	/* ->hangup() in progress */
 #define TTY_LDISC_HALTED	22	/* Line discipline is halted */
 
 #define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
-- 
2.1.3

[PATCH -next v2 04/26] tty: Clarify re-open behavior of master ptys

[Peter Hurley]

Re-opening master ptys is not allowed. Once opened and for the remaining
lifetime of the master pty, its tty count is 1. If its tty count has
dropped to 0, then the master pty was closed and TTY_CLOSING was set,
and destruction may begin imminently.

Besides the normal case of a legacy BSD pty master being re-opened
(which always returns -EIO), this code is only reachable in 2 degenerate
cases:
1. The pty master is the controlling terminal (this is possible through
   the TIOCSCTTY ioctl). pty masters are not designed to be controlling
   terminals and it's an oversight that tiocsctty() ever let that happen.
   The attempted open of /dev/tty will always fail. No known program does
   this.
2. The legacy BSD pty slave was opened first. The slave open will fail
   in pty_open() and tty_release() will commence. But before tty_release()
   claims the tty_mutex, there is a very small window where a parallel
   master open might succeed. In a test of racing legacy BSD slave and
   master parallel opens, where:
      slave open attempts:  10000   success:4527  failure:5473
      master open attempts: 11728   success:5789  failure:5939
   only 8 master open attempts would have succeeded reaching this code and
   successfully opened the master pty. This case is not possible with
   SysV ptys.

Always return -EIO if a master pty is re-opened or the slave is opened
first and the master opened in parallel (for legacy BSD ptys).

Furthermore, now that changing the slave's count is not required,
the tty_lock is sufficient for preventing concurrent changes to the
tty being re-opened (or failing re-opening).

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index bb4d2f2..6dc3fa0 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1444,9 +1444,9 @@ void tty_driver_remove_tty(struct tty_driver *driver, struct tty_struct *tty)
  * 	@tty	- the tty to open
  *
  *	Return 0 on success, -errno on error.
+ *	Re-opens on master ptys are not allowed and return -EIO.
  *
- *	Locking: tty_mutex must be held from the time the tty was found
- *		 till this open completes.
+ *	Locking: Caller must hold tty_lock
  */
 static int tty_reopen(struct tty_struct *tty)
 {
@@ -1456,16 +1456,9 @@ static int tty_reopen(struct tty_struct *tty)
 		return -EIO;
 
 	if (driver->type == TTY_DRIVER_TYPE_PTY &&
-	    driver->subtype == PTY_TYPE_MASTER) {
-		/*
-		 * special case for PTY masters: only one open permitted,
-		 * and the slave side open count is incremented as well.
-		 */
-		if (tty->count)
-			return -EIO;
+	    driver->subtype == PTY_TYPE_MASTER)
+		return -EIO;
 
-		tty->link->count++;
-	}
 	tty->count++;
 
 	WARN_ON(!tty->ldisc);
-- 
2.1.3

[PATCH -next v2 05/26] tty: Check tty->count instead of TTY_CLOSING in tty_reopen()

[Peter Hurley]

Although perhaps not obvious, the TTY_CLOSING bit is set when the
tty count has been decremented to 0 (which occurs while holding
tty_lock). The only other case when tty count is 0 during a re-open
is when a legacy BSD pty master has been opened in parallel but
after the pty slave, which is unsupported and returns an error.

Thus !tty->count contains the complete set of degenerate conditions
under which a tty open fails.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 6dc3fa0..97445c2 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1452,7 +1452,7 @@ static int tty_reopen(struct tty_struct *tty)
 {
 	struct tty_driver *driver = tty->driver;
 
-	if (test_bit(TTY_CLOSING, &tty->flags))
+	if (!tty->count)
 		return -EIO;
 
 	if (driver->type == TTY_DRIVER_TYPE_PTY &&
-- 
2.1.3

[PATCH -next v2 06/26] pty: Always return -EIO if slave BSD pty opened first

[Peter Hurley]

Opening the slave BSD pty first already returns -EIO from the slave
pty_open(), which in turn causes the newly installed tty pair to be
released before returning from tty_open(). However, this can also
cause a parallel master BSD pty open to fail because the pty pair
destruction may already been taking place in tty_release().

Failing at driver->install() if the slave pty is opened first ensures
that a pty master open cannot fail, because the driver tables will
not have been updated so tty_driver_lookup_tty() won't find the
master pty (and attempt to "re-open" it).

In turn, this guarantees that any tty with a tty->count == 0 is
in final close (rather than never opened).

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 7a1a538..bdb8fd1 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -383,6 +383,10 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	int idx = tty->index;
 	int retval = -ENOMEM;
 
+	/* Opening the slave first has always returned -EIO */
+	if (driver->subtype != PTY_TYPE_MASTER)
+		return -EIO;
+
 	ports[0] = kmalloc(sizeof **ports, GFP_KERNEL);
 	ports[1] = kmalloc(sizeof **ports, GFP_KERNEL);
 	if (!ports[0] || !ports[1])
@@ -419,8 +423,6 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	 * Everything allocated ... set up the o_tty structure.
 	 */
 	tty_driver_kref_get(driver->other);
-	if (driver->subtype == PTY_TYPE_MASTER)
-		o_tty->count++;
 	/* Establish the links in both directions */
 	tty->link   = o_tty;
 	o_tty->link = tty;
@@ -432,6 +434,7 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 
 	tty_driver_kref_get(driver);
 	tty->count++;
+	o_tty->count++;
 	return 0;
 err_free_termios:
 	if (legacy)
-- 
2.1.3

[PATCH -next v2 07/26] tty: Re-open /dev/tty without tty_mutex

[Peter Hurley]

Opening /dev/tty (ie., the controlling tty for the current task)
is always a re-open of the underlying tty. Because holding the
tty_lock is sufficient for safely re-opening a tty, and because
having a tty kref is sufficient for safely acquiring the tty_lock [1],
tty_open_current_tty() does not require holding tty_mutex.

Repurpose tty_open_current_tty() to perform the re-open itself and
refactor tty_open().

[1] Analysis of safely re-opening the current tty w/o tty_mutex

get_current_tty() gets a tty kref from the already kref'ed tty value of
current->signal->tty while holding the sighand lock for the current
task. This guarantees that the tty pointer returned from
get_current_tty() points to a tty which remains referenceable
while holding the kref.

Although release_tty() may run concurrently, and thus the driver
reference may be removed, release_one_tty() cannot have run, and
won't while holding the tty kref.

This, in turn, guarantees the tty_lock() can safely be acquired
(since tty->magic and tty->legacy_mutex are still a valid dereferences).
The tty_lock() also gets a tty kref to prevent the tty_unlock() from
dereferencing a released tty. Thus, the kref returned from
get_current_tty() can be released.

Lastly, the first operation of tty_reopen() is to check the tty count.
If non-zero, this ensures release_tty() is not running concurrently,
and the driver references have not been removed.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 53 ++++++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 97445c2..fb4583c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1944,20 +1944,20 @@ int tty_release(struct inode *inode, struct file *filp)
 }
 
 /**
- *	tty_open_current_tty - get tty of current task for open
+ *	tty_open_current_tty - get locked tty of current task
  *	@device: device number
  *	@filp: file pointer to tty
- *	@return: tty of the current task iff @device is /dev/tty
+ *	@return: locked tty of the current task iff @device is /dev/tty
+ *
+ *	Performs a re-open of the current task's controlling tty.
  *
  *	We cannot return driver and index like for the other nodes because
  *	devpts will not work then. It expects inodes to be from devpts FS.
- *
- *	We need to move to returning a refcounted object from all the lookup
- *	paths including this one.
  */
 static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
 {
 	struct tty_struct *tty;
+	int retval;
 
 	if (device != MKDEV(TTYAUX_MAJOR, 0))
 		return NULL;
@@ -1968,9 +1968,14 @@ static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
 
 	filp->f_flags |= O_NONBLOCK; /* Don't let /dev/tty block */
 	/* noctty = 1; */
-	tty_kref_put(tty);
-	/* FIXME: we put a reference and return a TTY! */
-	/* This is only safe because the caller holds tty_mutex */
+	tty_lock(tty);
+	tty_kref_put(tty);	/* safe to drop the kref now */
+
+	retval = tty_reopen(tty);
+	if (retval < 0) {
+		tty_unlock(tty);
+		tty = ERR_PTR(retval);
+	}
 	return tty;
 }
 
@@ -2068,13 +2073,9 @@ retry_open:
 	index  = -1;
 	retval = 0;
 
-	mutex_lock(&tty_mutex);
-	/* This is protected by the tty_mutex */
 	tty = tty_open_current_tty(device, filp);
-	if (IS_ERR(tty)) {
-		retval = PTR_ERR(tty);
-		goto err_unlock;
-	} else if (!tty) {
+	if (!tty) {
+		mutex_lock(&tty_mutex);
 		driver = tty_lookup_driver(device, filp, &noctty, &index);
 		if (IS_ERR(driver)) {
 			retval = PTR_ERR(driver);
@@ -2087,21 +2088,21 @@ retry_open:
 			retval = PTR_ERR(tty);
 			goto err_unlock;
 		}
-	}
 
-	if (tty) {
-		tty_lock(tty);
-		retval = tty_reopen(tty);
-		if (retval < 0) {
-			tty_unlock(tty);
-			tty = ERR_PTR(retval);
-		}
-	} else	/* Returns with the tty_lock held for now */
-		tty = tty_init_dev(driver, index);
+		if (tty) {
+			tty_lock(tty);
+			retval = tty_reopen(tty);
+			if (retval < 0) {
+				tty_unlock(tty);
+				tty = ERR_PTR(retval);
+			}
+		} else	/* Returns with the tty_lock held for now */
+			tty = tty_init_dev(driver, index);
 
-	mutex_unlock(&tty_mutex);
-	if (driver)
+		mutex_unlock(&tty_mutex);
 		tty_driver_kref_put(driver);
+	}
+
 	if (IS_ERR(tty)) {
 		retval = PTR_ERR(tty);
 		goto err_file;
-- 
2.1.3

[PATCH -next v2 08/26] tty: Drop tty_mutex before tty reopen

[Peter Hurley]

Holding tty_mutex for a tty re-open is no longer necessary since
"tty: Clarify re-open behavior of master ptys". Because the
slave tty count is no longer accessed by tty_reopen(), holding
tty_mutex to prevent concurrent final tty_release() of the slave
pty is not required.

As with "tty: Re-open /dev/tty without tty_mutex", holding a
tty kref until the tty_lock is acquired is sufficient to ensure
the tty has not been freed, which, in turn, is sufficient to
ensure the tty_lock can be safely acquired and the tty count
can be safely retrieved. A non-zero tty count with the tty lock
held guarantees that release_tty() has not run and cannot
run concurrently with tty_reopen().

Change tty_driver_lookup_tty() to acquire the tty kref, which
allows the tty_mutex to be dropped before acquiring the tty lock.
Dropping the tty_mutex before attempting the tty_lock allows
other ttys to be opened and released, without needing this
tty_reopen() to complete.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index fb4583c..fb74107 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1344,19 +1344,24 @@ static ssize_t tty_line_name(struct tty_driver *driver, int index, char *p)
  *	@driver: the driver for the tty
  *	@idx:	 the minor number
  *
- *	Return the tty, if found or ERR_PTR() otherwise.
+ *	Return the tty, if found. If not found, return NULL or ERR_PTR() if the
+ *	driver lookup() method returns an error.
  *
- *	Locking: tty_mutex must be held. If tty is found, the mutex must
- *	be held until the 'fast-open' is also done. Will change once we
- *	have refcounting in the driver and per driver locking
+ *	Locking: tty_mutex must be held. If the tty is found, bump the tty kref.
  */
 static struct tty_struct *tty_driver_lookup_tty(struct tty_driver *driver,
 		struct inode *inode, int idx)
 {
+	struct tty_struct *tty;
+
 	if (driver->ops->lookup)
-		return driver->ops->lookup(driver, inode, idx);
+		tty = driver->ops->lookup(driver, inode, idx);
+	else
+		tty = driver->ttys[idx];
 
-	return driver->ttys[idx];
+	if (!IS_ERR(tty))
+		tty_kref_get(tty);
+	return tty;
 }
 
 /**
@@ -2090,16 +2095,20 @@ retry_open:
 		}
 
 		if (tty) {
+			mutex_unlock(&tty_mutex);
 			tty_lock(tty);
+			/* safe to drop the kref from tty_driver_lookup_tty() */
+			tty_kref_put(tty);
 			retval = tty_reopen(tty);
 			if (retval < 0) {
 				tty_unlock(tty);
 				tty = ERR_PTR(retval);
 			}
-		} else	/* Returns with the tty_lock held for now */
+		} else { /* Returns with the tty_lock held for now */
 			tty = tty_init_dev(driver, index);
+			mutex_unlock(&tty_mutex);
+		}
 
-		mutex_unlock(&tty_mutex);
 		tty_driver_kref_put(driver);
 	}
 
-- 
2.1.3

[PATCH -next v2 09/26] tty: Remove TTY_CLOSING

[Peter Hurley]

Now that re-open is not permitted for a legacy BSD pty master,
using TTY_CLOSING to indicate when a tty can be torn-down is
no longer necessary.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 14 ++------------
 include/linux/tty.h  |  1 -
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index fb74107..49167ed 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1197,7 +1197,7 @@ void tty_write_message(struct tty_struct *tty, char *msg)
 	if (tty) {
 		mutex_lock(&tty->atomic_write_lock);
 		tty_lock(tty);
-		if (tty->ops->write && !test_bit(TTY_CLOSING, &tty->flags)) {
+		if (tty->ops->write && tty->count > 0) {
 			tty_unlock(tty);
 			tty->ops->write(tty, msg, strlen(msg));
 		} else
@@ -1888,16 +1888,6 @@ int tty_release(struct inode *inode, struct file *filp)
 	/*
 	 * Perform some housekeeping before deciding whether to return.
 	 *
-	 * Set the TTY_CLOSING flag if this was the last open.  In the
-	 * case of a pty we may have to wait around for the other side
-	 * to close, and TTY_CLOSING makes sure we can't be reopened.
-	 */
-	if (tty_closing)
-		set_bit(TTY_CLOSING, &tty->flags);
-	if (o_tty_closing)
-		set_bit(TTY_CLOSING, &o_tty->flags);
-
-	/*
 	 * If _either_ side is closing, make sure there aren't any
 	 * processes that still think tty or o_tty is their controlling
 	 * tty.
@@ -1912,7 +1902,7 @@ int tty_release(struct inode *inode, struct file *filp)
 
 	mutex_unlock(&tty_mutex);
 	tty_unlock_pair(tty, o_tty);
-	/* At this point the TTY_CLOSING flag should ensure a dead tty
+	/* At this point, the tty->count == 0 should ensure a dead tty
 	   cannot be re-opened by a racing opener */
 
 	/* check whether both sides are closing ... */
diff --git a/include/linux/tty.h b/include/linux/tty.h
index ff0dd7a..35b29f0 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -316,7 +316,6 @@ struct tty_file_private {
 #define TTY_EXCLUSIVE 		3	/* Exclusive open mode */
 #define TTY_DEBUG 		4	/* Debugging */
 #define TTY_DO_WRITE_WAKEUP 	5	/* Call write_wakeup after queuing new */
-#define TTY_CLOSING 		7	/* ->close() in progress */
 #define TTY_LDISC_OPEN	 	11	/* Line discipline is open */
 #define TTY_PTY_LOCK 		16	/* pty private */
 #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
-- 
2.1.3

[PATCH -next v2 10/26] tty: Don't take tty_mutex for tty count changes

[Peter Hurley]

Holding tty_mutex is no longer required to serialize changes to
the tty_count or to prevent concurrent opens of closing ttys;
tty_lock() is sufficient.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 49167ed..f1c4b07 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1806,10 +1806,6 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * each iteration we avoid any problems.
 	 */
 	while (1) {
-		/* Guard against races with tty->count changes elsewhere and
-		   opens on /dev/tty */
-
-		mutex_lock(&tty_mutex);
 		tty_lock_pair(tty, o_tty);
 		tty_closing = tty->count <= 1;
 		o_tty_closing = o_tty &&
@@ -1845,7 +1841,6 @@ int tty_release(struct inode *inode, struct file *filp)
 			       __func__, tty_name(tty, buf));
 		}
 		tty_unlock_pair(tty, o_tty);
-		mutex_unlock(&tty_mutex);
 		schedule_timeout_killable(timeout);
 		if (timeout < 120 * HZ)
 			timeout = 2 * timeout + 1;
@@ -1900,7 +1895,6 @@ int tty_release(struct inode *inode, struct file *filp)
 		read_unlock(&tasklist_lock);
 	}
 
-	mutex_unlock(&tty_mutex);
 	tty_unlock_pair(tty, o_tty);
 	/* At this point, the tty->count == 0 should ensure a dead tty
 	   cannot be re-opened by a racing opener */
-- 
2.1.3

Re: [PATCH -next v2 10/26] tty: Don't take tty_mutex for tty count changes

[Greg Kroah-Hartman]

On Wed, Nov 05, 2014 at 12:12:53PM -0500, Peter Hurley wrote:
> Holding tty_mutex is no longer required to serialize changes to
> the tty_count or to prevent concurrent opens of closing ttys;
> tty_lock() is sufficient.
> 
> Reviewed-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/tty_io.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index ea8c6cae8d12..e59de81c39a9 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1804,10 +1804,6 @@ int tty_release(struct inode *inode, struct file *filp)
>  	 * each iteration we avoid any problems.
>  	 */
>  	while (1) {
> -		/* Guard against races with tty->count changes elsewhere and
> -		   opens on /dev/tty */
> -
> -		mutex_lock(&tty_mutex);
>  		tty_lock_pair(tty, o_tty);
>  		tty_closing = tty->count <= 1;
>  		o_tty_closing = o_tty &&
> @@ -1840,7 +1836,6 @@ int tty_release(struct inode *inode, struct file *filp)
>  		printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
>  				__func__, tty_name(tty, buf));
>  		tty_unlock_pair(tty, o_tty);
> -		mutex_unlock(&tty_mutex);
>  		schedule();
>  	}
>  

The code in my tree in this section of tty_release() looks a bit
different, so I had to hand-apply this patch.  I've included the version
I used below, please verify I didn't mess it up.

thanks,

greg k-h



diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index ea8c6cae8d12..e59de81c39a9 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1804,10 +1804,6 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * each iteration we avoid any problems.
 	 */
 	while (1) {
-		/* Guard against races with tty->count changes elsewhere and
-		   opens on /dev/tty */
-
-		mutex_lock(&tty_mutex);
 		tty_lock_pair(tty, o_tty);
 		tty_closing = tty->count <= 1;
 		o_tty_closing = o_tty &&
@@ -1840,7 +1836,6 @@ int tty_release(struct inode *inode, struct file *filp)
 		printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
 				__func__, tty_name(tty, buf));
 		tty_unlock_pair(tty, o_tty);
-		mutex_unlock(&tty_mutex);
 		schedule();
 	}
 
@@ -1891,7 +1886,6 @@ int tty_release(struct inode *inode, struct file *filp)
 		read_unlock(&tasklist_lock);
 	}
 
-	mutex_unlock(&tty_mutex);
 	tty_unlock_pair(tty, o_tty);
 	/* At this point, the tty->count == 0 should ensure a dead tty
 	   cannot be re-opened by a racing opener */

Re: [PATCH -next v2 10/26] tty: Don't take tty_mutex for tty count changes

[Peter Hurley]

On 11/05/2014 09:33 PM, Greg Kroah-Hartman wrote:
> On Wed, Nov 05, 2014 at 12:12:53PM -0500, Peter Hurley wrote:
>> Holding tty_mutex is no longer required to serialize changes to
>> the tty_count or to prevent concurrent opens of closing ttys;
>> tty_lock() is sufficient.
>>
>> Reviewed-by: Alan Cox <alan@linux.intel.com>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>  drivers/tty/tty_io.c | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index ea8c6cae8d12..e59de81c39a9 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -1804,10 +1804,6 @@ int tty_release(struct inode *inode, struct file *filp)
>>  	 * each iteration we avoid any problems.
>>  	 */
>>  	while (1) {
>> -		/* Guard against races with tty->count changes elsewhere and
>> -		   opens on /dev/tty */
>> -
>> -		mutex_lock(&tty_mutex);
>>  		tty_lock_pair(tty, o_tty);
>>  		tty_closing = tty->count <= 1;
>>  		o_tty_closing = o_tty &&
>> @@ -1840,7 +1836,6 @@ int tty_release(struct inode *inode, struct file *filp)
>>  		printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
>>  				__func__, tty_name(tty, buf));
>>  		tty_unlock_pair(tty, o_tty);
>> -		mutex_unlock(&tty_mutex);
>>  		schedule();
>>  	}
>>  
> 
> The code in my tree in this section of tty_release() looks a bit
> different, so I had to hand-apply this patch.

Although there's nothing wrong with your version, I'm wondering why this
didn't apply cleanly.

While I go look at your tree, can you check that these patches are
sitting on top of the earlier two patches you applied to your tty-linus
branch; specifically 'tty: Fix high cpu load if tty is unreleasable' and
'tty: Prevent "read/write wait queue active!" log flooding'?

Regards,
Peter

> I've included the version
> I used below, please verify I didn't mess it up.
> 
> thanks,
> 
> greg k-h
> 
> 
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index ea8c6cae8d12..e59de81c39a9 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1804,10 +1804,6 @@ int tty_release(struct inode *inode, struct file *filp)
>  	 * each iteration we avoid any problems.
>  	 */
>  	while (1) {
> -		/* Guard against races with tty->count changes elsewhere and
> -		   opens on /dev/tty */
> -
> -		mutex_lock(&tty_mutex);
>  		tty_lock_pair(tty, o_tty);
>  		tty_closing = tty->count <= 1;
>  		o_tty_closing = o_tty &&
> @@ -1840,7 +1836,6 @@ int tty_release(struct inode *inode, struct file *filp)
>  		printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
>  				__func__, tty_name(tty, buf));
>  		tty_unlock_pair(tty, o_tty);
> -		mutex_unlock(&tty_mutex);
>  		schedule();
>  	}
>  
> @@ -1891,7 +1886,6 @@ int tty_release(struct inode *inode, struct file *filp)
>  		read_unlock(&tasklist_lock);
>  	}
>  
> -	mutex_unlock(&tty_mutex);
>  	tty_unlock_pair(tty, o_tty);
>  	/* At this point, the tty->count == 0 should ensure a dead tty
>  	   cannot be re-opened by a racing opener */
> 

Re: [PATCH -next v2 10/26] tty: Don't take tty_mutex for tty count changes

[Peter Hurley]

On 11/05/2014 09:39 PM, Peter Hurley wrote:
> On 11/05/2014 09:33 PM, Greg Kroah-Hartman wrote:
>> On Wed, Nov 05, 2014 at 12:12:53PM -0500, Peter Hurley wrote:
>>> Holding tty_mutex is no longer required to serialize changes to
>>> the tty_count or to prevent concurrent opens of closing ttys;
>>> tty_lock() is sufficient.
>>>
>>> Reviewed-by: Alan Cox <alan@linux.intel.com>
>>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>>> ---
>>>  drivers/tty/tty_io.c | 6 ------
>>>  1 file changed, 6 deletions(-)
>>>
>>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>>> index ea8c6cae8d12..e59de81c39a9 100644
>>> --- a/drivers/tty/tty_io.c
>>> +++ b/drivers/tty/tty_io.c
>>> @@ -1804,10 +1804,6 @@ int tty_release(struct inode *inode, struct file *filp)
>>>  	 * each iteration we avoid any problems.
>>>  	 */
>>>  	while (1) {
>>> -		/* Guard against races with tty->count changes elsewhere and
>>> -		   opens on /dev/tty */
>>> -
>>> -		mutex_lock(&tty_mutex);
>>>  		tty_lock_pair(tty, o_tty);
>>>  		tty_closing = tty->count <= 1;
>>>  		o_tty_closing = o_tty &&
>>> @@ -1840,7 +1836,6 @@ int tty_release(struct inode *inode, struct file *filp)
>>>  		printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
>>>  				__func__, tty_name(tty, buf));
>>>  		tty_unlock_pair(tty, o_tty);
>>> -		mutex_unlock(&tty_mutex);
>>>  		schedule();
>>>  	}
>>>  
>>
>> The code in my tree in this section of tty_release() looks a bit
>> different, so I had to hand-apply this patch.
> 
> Although there's nothing wrong with your version, I'm wondering why this
> didn't apply cleanly.
> 
> While I go look at your tree, can you check that these patches are
> sitting on top of the earlier two patches you applied to your tty-linus
> branch; specifically 'tty: Fix high cpu load if tty is unreleasable' and
> 'tty: Prevent "read/write wait queue active!" log flooding'?

Yep, that's the problem: your 'tty-testing' branch doesn't have the 3 patches
from me that you put in your 'tty-linus' branch earlier this evening. Those are:

serial: Fix divide-by-zero fault in uart_get_divisor()
tty: Fix high cpu load if tty is unreleasable
tty: Prevent "read/write wait queue active!" log flooding

How can I help fix this?

Regards,
Peter Hurley


>> I've included the version
>> I used below, please verify I didn't mess it up.
>>
>> thanks,
>>
>> greg k-h
>>
>>
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index ea8c6cae8d12..e59de81c39a9 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -1804,10 +1804,6 @@ int tty_release(struct inode *inode, struct file *filp)
>>  	 * each iteration we avoid any problems.
>>  	 */
>>  	while (1) {
>> -		/* Guard against races with tty->count changes elsewhere and
>> -		   opens on /dev/tty */
>> -
>> -		mutex_lock(&tty_mutex);
>>  		tty_lock_pair(tty, o_tty);
>>  		tty_closing = tty->count <= 1;
>>  		o_tty_closing = o_tty &&
>> @@ -1840,7 +1836,6 @@ int tty_release(struct inode *inode, struct file *filp)
>>  		printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
>>  				__func__, tty_name(tty, buf));
>>  		tty_unlock_pair(tty, o_tty);
>> -		mutex_unlock(&tty_mutex);
>>  		schedule();
>>  	}
>>  
>> @@ -1891,7 +1886,6 @@ int tty_release(struct inode *inode, struct file *filp)
>>  		read_unlock(&tasklist_lock);
>>  	}
>>  
>> -	mutex_unlock(&tty_mutex);
>>  	tty_unlock_pair(tty, o_tty);
>>  	/* At this point, the tty->count == 0 should ensure a dead tty
>>  	   cannot be re-opened by a racing opener */
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH -next v2 10/26] tty: Don't take tty_mutex for tty count changes

[Greg Kroah-Hartman]

On Wed, Nov 05, 2014 at 09:50:22PM -0500, Peter Hurley wrote:
> On 11/05/2014 09:39 PM, Peter Hurley wrote:
> > On 11/05/2014 09:33 PM, Greg Kroah-Hartman wrote:
> >> On Wed, Nov 05, 2014 at 12:12:53PM -0500, Peter Hurley wrote:
> >>> Holding tty_mutex is no longer required to serialize changes to
> >>> the tty_count or to prevent concurrent opens of closing ttys;
> >>> tty_lock() is sufficient.
> >>>
> >>> Reviewed-by: Alan Cox <alan@linux.intel.com>
> >>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> >>> ---
> >>>  drivers/tty/tty_io.c | 6 ------
> >>>  1 file changed, 6 deletions(-)
> >>>
> >>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> >>> index ea8c6cae8d12..e59de81c39a9 100644
> >>> --- a/drivers/tty/tty_io.c
> >>> +++ b/drivers/tty/tty_io.c
> >>> @@ -1804,10 +1804,6 @@ int tty_release(struct inode *inode, struct file *filp)
> >>>  	 * each iteration we avoid any problems.
> >>>  	 */
> >>>  	while (1) {
> >>> -		/* Guard against races with tty->count changes elsewhere and
> >>> -		   opens on /dev/tty */
> >>> -
> >>> -		mutex_lock(&tty_mutex);
> >>>  		tty_lock_pair(tty, o_tty);
> >>>  		tty_closing = tty->count <= 1;
> >>>  		o_tty_closing = o_tty &&
> >>> @@ -1840,7 +1836,6 @@ int tty_release(struct inode *inode, struct file *filp)
> >>>  		printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
> >>>  				__func__, tty_name(tty, buf));
> >>>  		tty_unlock_pair(tty, o_tty);
> >>> -		mutex_unlock(&tty_mutex);
> >>>  		schedule();
> >>>  	}
> >>>  
> >>
> >> The code in my tree in this section of tty_release() looks a bit
> >> different, so I had to hand-apply this patch.
> > 
> > Although there's nothing wrong with your version, I'm wondering why this
> > didn't apply cleanly.
> > 
> > While I go look at your tree, can you check that these patches are
> > sitting on top of the earlier two patches you applied to your tty-linus
> > branch; specifically 'tty: Fix high cpu load if tty is unreleasable' and
> > 'tty: Prevent "read/write wait queue active!" log flooding'?
> 
> Yep, that's the problem: your 'tty-testing' branch doesn't have the 3 patches
> from me that you put in your 'tty-linus' branch earlier this evening. Those are:
> 
> serial: Fix divide-by-zero fault in uart_get_divisor()
> tty: Fix high cpu load if tty is unreleasable
> tty: Prevent "read/write wait queue active!" log flooding
> 
> How can I help fix this?

Ah, didn't realize that was the issue, I've merged the branches together
now, so all should be good.

Sorry for the noise,

greg k-h

[PATCH -next v2 11/26] tty: Don't release tty locks for wait queue sanity check

[Peter Hurley]

Releasing the tty locks while waiting for the tty wait queues to
be empty is no longer necessary nor desirable. Prior to
"tty: Don't take tty_mutex for tty count changes", dropping the
tty locks was necessary to reestablish the correct lock order between
tty_mutex and the tty locks. Dropping the global tty_mutex was necessary;
otherwise new ttys could not have been opened while waiting.

However, without needing the global tty_mutex held, the tty locks for
the releasing tty can now be held through the sleep. The sanity check
is for abnormal conditions caused by kernel bugs, not for recoverable
errors caused by misbehaving userspace; dropping the tty locks only
allows the tty state to get more sideways.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index f1c4b07..276d939 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1800,13 +1800,10 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * first, its count will be one, since the master side holds an open.
 	 * Thus this test wouldn't be triggered at the time the slave closes,
 	 * so we do it now.
-	 *
-	 * Note that it's possible for the tty to be opened again while we're
-	 * flushing out waiters.  By recalculating the closing flags before
-	 * each iteration we avoid any problems.
 	 */
+	tty_lock_pair(tty, o_tty);
+
 	while (1) {
-		tty_lock_pair(tty, o_tty);
 		tty_closing = tty->count <= 1;
 		o_tty_closing = o_tty &&
 			(o_tty->count <= (pty_master ? 1 : 0));
@@ -1840,7 +1837,6 @@ int tty_release(struct inode *inode, struct file *filp)
 			printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
 			       __func__, tty_name(tty, buf));
 		}
-		tty_unlock_pair(tty, o_tty);
 		schedule_timeout_killable(timeout);
 		if (timeout < 120 * HZ)
 			timeout = 2 * timeout + 1;
-- 
2.1.3

Re: [PATCH -next v2 11/26] tty: Don't release tty locks for wait queue sanity check

[Greg Kroah-Hartman]

On Wed, Nov 05, 2014 at 12:12:54PM -0500, Peter Hurley wrote:
> Releasing the tty locks while waiting for the tty wait queues to
> be empty is no longer necessary nor desirable. Prior to
> "tty: Don't take tty_mutex for tty count changes", dropping the
> tty locks was necessary to reestablish the correct lock order between
> tty_mutex and the tty locks. Dropping the global tty_mutex was necessary;
> otherwise new ttys could not have been opened while waiting.
> 
> However, without needing the global tty_mutex held, the tty locks for
> the releasing tty can now be held through the sleep. The sanity check
> is for abnormal conditions caused by kernel bugs, not for recoverable
> errors caused by misbehaving userspace; dropping the tty locks only
> allows the tty state to get more sideways.
> 
> Reviewed-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/tty_io.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index e59de81c39a9..b008e2b38d54 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1798,13 +1798,10 @@ int tty_release(struct inode *inode, struct file *filp)
>  	 * first, its count will be one, since the master side holds an open.
>  	 * Thus this test wouldn't be triggered at the time the slave closes,
>  	 * so we do it now.
> -	 *
> -	 * Note that it's possible for the tty to be opened again while we're
> -	 * flushing out waiters.  By recalculating the closing flags before
> -	 * each iteration we avoid any problems.
>  	 */
> +	tty_lock_pair(tty, o_tty);
> +
>  	while (1) {
> -		tty_lock_pair(tty, o_tty);
>  		tty_closing = tty->count <= 1;
>  		o_tty_closing = o_tty &&
>  			(o_tty->count <= (pty_master ? 1 : 0));
> @@ -1835,7 +1832,6 @@ int tty_release(struct inode *inode, struct file *filp)
>  
>  		printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
>  				__func__, tty_name(tty, buf));
> -		tty_unlock_pair(tty, o_tty);
>  		schedule();
>  	}
>  

This patch had the same type of fuzz as the previous one, the version I
used was:


diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e59de81c39a9..b008e2b38d54 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1798,13 +1798,10 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * first, its count will be one, since the master side holds an open.
 	 * Thus this test wouldn't be triggered at the time the slave closes,
 	 * so we do it now.
-	 *
-	 * Note that it's possible for the tty to be opened again while we're
-	 * flushing out waiters.  By recalculating the closing flags before
-	 * each iteration we avoid any problems.
 	 */
+	tty_lock_pair(tty, o_tty);
+
 	while (1) {
-		tty_lock_pair(tty, o_tty);
 		tty_closing = tty->count <= 1;
 		o_tty_closing = o_tty &&
 			(o_tty->count <= (pty_master ? 1 : 0));
@@ -1835,7 +1832,6 @@ int tty_release(struct inode *inode, struct file *filp)
 
 		printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
 				__func__, tty_name(tty, buf));
-		tty_unlock_pair(tty, o_tty);
 		schedule();
 	}
 

[PATCH -next v2 12/26] tty: Document check_tty_count() requires tty_lock held

[Peter Hurley]

Holding the tty_lock() is necessary to prevent concurrent changes
to the tty count that may cause it to differ from the open file
list count. The tty_lock() is already held at all call sites.

NB: Note that the check for the pty master tty count is safe because
the slave's tty_lock() is held while decrementing the pty master
tty count.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 276d939..41eeee0 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -275,6 +275,7 @@ int tty_paranoia_check(struct tty_struct *tty, struct inode *inode,
 	return 0;
 }
 
+/* Caller must hold tty_lock */
 static int check_tty_count(struct tty_struct *tty, const char *routine)
 {
 #ifdef CHECK_TTY_COUNT
-- 
2.1.3

[PATCH -next v2 13/26] tty: Simplify pty pair teardown logic

[Peter Hurley]

When the slave side closes and its tty count is 0, the pty
pair can be destroyed; the master side must have already
closed for the slave side tty count to be 0. Thus, only the
pty master close must check if the slave side has closed by
checking the slave tty count.

Remove the pre-computed closing flags and check the actual count(s).
Regular ttys are unaffected by this change.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 41eeee0..7b91007 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1755,7 +1755,7 @@ int tty_release(struct inode *inode, struct file *filp)
 {
 	struct tty_struct *tty = file_tty(filp);
 	struct tty_struct *o_tty;
-	int	pty_master, tty_closing, o_tty_closing, do_sleep;
+	int	pty_master, do_sleep, final;
 	int	idx;
 	char	buf[64];
 	long	timeout = 0;
@@ -1799,18 +1799,15 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * The test for the o_tty closing is necessary, since the master and
 	 * slave sides may close in any order.  If the slave side closes out
 	 * first, its count will be one, since the master side holds an open.
-	 * Thus this test wouldn't be triggered at the time the slave closes,
+	 * Thus this test wouldn't be triggered at the time the slave closed,
 	 * so we do it now.
 	 */
 	tty_lock_pair(tty, o_tty);
 
 	while (1) {
-		tty_closing = tty->count <= 1;
-		o_tty_closing = o_tty &&
-			(o_tty->count <= (pty_master ? 1 : 0));
 		do_sleep = 0;
 
-		if (tty_closing) {
+		if (tty->count <= 1) {
 			if (waitqueue_active(&tty->read_wait)) {
 				wake_up_poll(&tty->read_wait, POLLIN);
 				do_sleep++;
@@ -1820,7 +1817,7 @@ int tty_release(struct inode *inode, struct file *filp)
 				do_sleep++;
 			}
 		}
-		if (o_tty_closing) {
+		if (pty_master && o_tty->count <= 1) {
 			if (waitqueue_active(&o_tty->read_wait)) {
 				wake_up_poll(&o_tty->read_wait, POLLIN);
 				do_sleep++;
@@ -1845,14 +1842,6 @@ int tty_release(struct inode *inode, struct file *filp)
 			timeout = MAX_SCHEDULE_TIMEOUT;
 	}
 
-	/*
-	 * 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) {
 			printk(KERN_WARNING "%s: bad pty slave count (%d) for %s\n",
@@ -1884,20 +1873,22 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * processes that still think tty or o_tty is their controlling
 	 * tty.
 	 */
-	if (tty_closing || o_tty_closing) {
+	if (!tty->count) {
 		read_lock(&tasklist_lock);
 		session_clear_tty(tty->session);
-		if (o_tty)
+		if (pty_master)
 			session_clear_tty(o_tty->session);
 		read_unlock(&tasklist_lock);
 	}
 
+	/* check whether both sides are closing ... */
+	final = !tty->count && !(pty_master && o_tty->count);
+
 	tty_unlock_pair(tty, o_tty);
 	/* At this point, the tty->count == 0 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))
+	if (!final)
 		return 0;
 
 #ifdef TTY_DEBUG_HANGUP
-- 
2.1.3

[PATCH -next v2 14/26] tty: Fold pty pair handling into tty_flush_works()

[Peter Hurley]

Perform work flush for both ends of a pty pair within tty_flush_works(),
rather than calling twice.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 7b91007..563ab37 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1584,15 +1584,19 @@ void tty_free_termios(struct tty_struct *tty)
 EXPORT_SYMBOL(tty_free_termios);
 
 /**
- *	tty_flush_works		-	flush all works of a tty
- *	@tty: tty device to flush works for
+ *	tty_flush_works		-	flush all works of a tty/pty pair
+ *	@tty: tty device to flush works for (or either end of a pty pair)
  *
- *	Sync flush all works belonging to @tty.
+ *	Sync flush all works belonging to @tty (and the 'other' tty).
  */
 static void tty_flush_works(struct tty_struct *tty)
 {
 	flush_work(&tty->SAK_work);
 	flush_work(&tty->hangup_work);
+	if (tty->link) {
+		flush_work(&tty->link->SAK_work);
+		flush_work(&tty->link->hangup_work);
+	}
 }
 
 /**
@@ -1901,8 +1905,6 @@ int tty_release(struct inode *inode, struct file *filp)
 
 	/* Wait for pending work before tty destruction commmences */
 	tty_flush_works(tty);
-	if (o_tty)
-		tty_flush_works(o_tty);
 
 #ifdef TTY_DEBUG_HANGUP
 	printk(KERN_DEBUG "%s: %s: freeing structure...\n", __func__, tty_name(tty, buf));
-- 
2.1.3

[PATCH -next v2 15/26] tty: Simplify tty_ldisc_release() interface

[Peter Hurley]

Passing the 'other' tty to tty_ldisc_release() only makes sense
for a pty pair; make o_tty function local instead.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c    |  2 +-
 drivers/tty/tty_ldisc.c | 15 +++++++--------
 include/linux/tty.h     |  2 +-
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 563ab37..4034e5e 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1901,7 +1901,7 @@ int tty_release(struct inode *inode, struct file *filp)
 	/*
 	 * Ask the line discipline code to release its structures
 	 */
-	tty_ldisc_release(tty, o_tty);
+	tty_ldisc_release(tty);
 
 	/* Wait for pending work before tty destruction commmences */
 	tty_flush_works(tty);
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 49001fa..1c4d7b6 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -756,18 +756,17 @@ static void tty_ldisc_kill(struct tty_struct *tty)
 
 /**
  *	tty_ldisc_release		-	release line discipline
- *	@tty: tty being shut down
- *	@o_tty: pair tty for pty/tty pairs
- *
- *	Called during the final close of a tty/pty pair in order to shut down
- *	the line discpline layer. On exit the ldisc assigned is N_TTY and the
- *	ldisc has not been opened.
+ *	@tty: tty being shut down (or one end of pty pair)
  *
- *	Holding ldisc_sem write lock serializes tty->ldisc changes.
+ *	Called during the final close of a tty or a pty pair in order to shut
+ *	down the line discpline layer. On exit, each ldisc assigned is N_TTY and
+ *	each ldisc has not been opened.
  */
 
-void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
+void tty_ldisc_release(struct tty_struct *tty)
 {
+	struct tty_struct *o_tty = tty->link;
+
 	/*
 	 * Shutdown this line discipline. As this is the final close,
 	 * it does not race with the set_ldisc code path.
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 35b29f0..af1a7f3 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -557,7 +557,7 @@ extern int tty_register_ldisc(int disc, struct tty_ldisc_ops *new_ldisc);
 extern int tty_unregister_ldisc(int disc);
 extern int tty_set_ldisc(struct tty_struct *tty, int ldisc);
 extern int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty);
-extern void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty);
+extern void tty_ldisc_release(struct tty_struct *tty);
 extern void tty_ldisc_init(struct tty_struct *tty);
 extern void tty_ldisc_deinit(struct tty_struct *tty);
 extern void tty_ldisc_begin(void);
-- 
2.1.3

[PATCH -next v2 16/26] tty: Simplify tty_release_checks() interface

[Peter Hurley]

Passing the 'other' tty to tty_release_checks() only makes sense
for a pty pair; make o_tty scope local instead.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 4034e5e..c765260 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1702,8 +1702,7 @@ static void release_tty(struct tty_struct *tty, int idx)
  *	Performs some paranoid checking before true release of the @tty.
  *	This is a no-op unless TTY_PARANOIA_CHECK is defined.
  */
-static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty,
-		int idx)
+static int tty_release_checks(struct tty_struct *tty, int idx)
 {
 #ifdef TTY_PARANOIA_CHECK
 	if (idx < 0 || idx >= tty->driver->num) {
@@ -1722,6 +1721,8 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty,
 		return -1;
 	}
 	if (tty->driver->other) {
+		struct tty_struct *o_tty = tty->link;
+
 		if (o_tty != tty->driver->other->ttys[idx]) {
 			printk(KERN_DEBUG "%s: other->table[%d] not o_tty for (%s)\n",
 					__func__, idx, tty->name);
@@ -1779,7 +1780,7 @@ int tty_release(struct inode *inode, struct file *filp)
 	/* Review: parallel close */
 	o_tty = tty->link;
 
-	if (tty_release_checks(tty, o_tty, idx)) {
+	if (tty_release_checks(tty, idx)) {
 		tty_unlock(tty);
 		return 0;
 	}
-- 
2.1.3

[PATCH -next v2 17/26] tty: Simplify tty_release() state checks

[Peter Hurley]

The local o_tty variable in tty_release() is now accessed only
when closing the pty master.

Set o_tty to slave pty when closing pty master, otherwise NULL;
use o_tty != NULL as replacement for pty_master.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index c765260..bbc940d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1759,8 +1759,8 @@ static int tty_release_checks(struct tty_struct *tty, int idx)
 int tty_release(struct inode *inode, struct file *filp)
 {
 	struct tty_struct *tty = file_tty(filp);
-	struct tty_struct *o_tty;
-	int	pty_master, do_sleep, final;
+	struct tty_struct *o_tty = NULL;
+	int	do_sleep, final;
 	int	idx;
 	char	buf[64];
 	long	timeout = 0;
@@ -1775,10 +1775,9 @@ int tty_release(struct inode *inode, struct file *filp)
 	__tty_fasync(-1, filp, 0);
 
 	idx = tty->index;
-	pty_master = (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
-		      tty->driver->subtype == PTY_TYPE_MASTER);
-	/* Review: parallel close */
-	o_tty = tty->link;
+	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
+	    tty->driver->subtype == PTY_TYPE_MASTER)
+		o_tty = tty->link;
 
 	if (tty_release_checks(tty, idx)) {
 		tty_unlock(tty);
@@ -1822,7 +1821,7 @@ int tty_release(struct inode *inode, struct file *filp)
 				do_sleep++;
 			}
 		}
-		if (pty_master && o_tty->count <= 1) {
+		if (o_tty && o_tty->count <= 1) {
 			if (waitqueue_active(&o_tty->read_wait)) {
 				wake_up_poll(&o_tty->read_wait, POLLIN);
 				do_sleep++;
@@ -1847,7 +1846,7 @@ int tty_release(struct inode *inode, struct file *filp)
 			timeout = MAX_SCHEDULE_TIMEOUT;
 	}
 
-	if (pty_master) {
+	if (o_tty) {
 		if (--o_tty->count < 0) {
 			printk(KERN_WARNING "%s: bad pty slave count (%d) for %s\n",
 				__func__, o_tty->count, tty_name(o_tty, buf));
@@ -1881,13 +1880,13 @@ int tty_release(struct inode *inode, struct file *filp)
 	if (!tty->count) {
 		read_lock(&tasklist_lock);
 		session_clear_tty(tty->session);
-		if (pty_master)
+		if (o_tty)
 			session_clear_tty(o_tty->session);
 		read_unlock(&tasklist_lock);
 	}
 
 	/* check whether both sides are closing ... */
-	final = !tty->count && !(pty_master && o_tty->count);
+	final = !tty->count && !(o_tty && o_tty->count);
 
 	tty_unlock_pair(tty, o_tty);
 	/* At this point, the tty->count == 0 should ensure a dead tty
-- 
2.1.3

[PATCH -next v2 18/26] tty: Change tty lock order to master->slave

[Peter Hurley]

When releasing the master pty, the slave pty also needs to be locked
to prevent concurrent tty count changes for the slave pty and to
ensure that only one parallel master and slave release observe the
final close, and proceed to destruct the pty pair. Conversely, when
releasing the slave pty, locking the master pty is not necessary
(since the master's state can be inferred by the slave tty count).

Introduce tty_lock_slave()/tty_unlock_slave() which acquires/releases
the tty lock of the slave pty. Remove tty_lock_pair()/tty_unlock_pair().

Dropping the tty_lock is no longer required to re-establish a stable
lock order.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c    | 10 ++++++----
 drivers/tty/tty_mutex.c | 32 +++++++++++++-------------------
 include/linux/tty.h     |  7 ++-----
 3 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index bbc940d..fabf4c0 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1792,7 +1792,9 @@ int tty_release(struct inode *inode, struct file *filp)
 	if (tty->ops->close)
 		tty->ops->close(tty, filp);
 
-	tty_unlock(tty);
+	/* If tty is pty master, lock the slave pty (stable lock order) */
+	tty_lock_slave(o_tty);
+
 	/*
 	 * Sanity check: if tty->count is going to zero, there shouldn't be
 	 * any waiters on tty->read_wait or tty->write_wait.  We test the
@@ -1806,8 +1808,6 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * Thus this test wouldn't be triggered at the time the slave closed,
 	 * so we do it now.
 	 */
-	tty_lock_pair(tty, o_tty);
-
 	while (1) {
 		do_sleep = 0;
 
@@ -1888,7 +1888,9 @@ int tty_release(struct inode *inode, struct file *filp)
 	/* check whether both sides are closing ... */
 	final = !tty->count && !(o_tty && o_tty->count);
 
-	tty_unlock_pair(tty, o_tty);
+	tty_unlock_slave(o_tty);
+	tty_unlock(tty);
+
 	/* At this point, the tty->count == 0 should ensure a dead tty
 	   cannot be re-opened by a racing opener */
 
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 2e41abe..f43e995 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -4,6 +4,11 @@
 #include <linux/semaphore.h>
 #include <linux/sched.h>
 
+/*
+ * Nested tty locks are necessary for releasing pty pairs.
+ * The stable lock order is master pty first, then slave pty.
+ */
+
 /* Legacy tty mutex glue */
 
 enum {
@@ -45,29 +50,18 @@ void __lockfunc tty_unlock(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(tty_unlock);
 
-/*
- * Getting the big tty mutex for a pair of ttys with lock ordering
- * On a non pty/tty pair tty2 can be NULL which is just fine.
- */
-void __lockfunc tty_lock_pair(struct tty_struct *tty,
-					struct tty_struct *tty2)
+void __lockfunc tty_lock_slave(struct tty_struct *tty)
 {
-	if (tty < tty2) {
-		tty_lock(tty);
-		tty_lock_nested(tty2, TTY_MUTEX_NESTED);
-	} else {
-		if (tty2 && tty2 != tty)
-			tty_lock(tty2);
+	if (tty && tty != tty->link) {
+		WARN_ON(!mutex_is_locked(&tty->link->legacy_mutex) ||
+			!tty->driver->type == TTY_DRIVER_TYPE_PTY ||
+			!tty->driver->type == PTY_TYPE_SLAVE);
 		tty_lock_nested(tty, TTY_MUTEX_NESTED);
 	}
 }
-EXPORT_SYMBOL(tty_lock_pair);
 
-void __lockfunc tty_unlock_pair(struct tty_struct *tty,
-						struct tty_struct *tty2)
+void __lockfunc tty_unlock_slave(struct tty_struct *tty)
 {
-	tty_unlock(tty);
-	if (tty2 && tty2 != tty)
-		tty_unlock(tty2);
+	if (tty && tty != tty->link)
+		tty_unlock(tty);
 }
-EXPORT_SYMBOL(tty_unlock_pair);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index af1a7f3..a07b4b4 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -638,11 +638,8 @@ extern long vt_compat_ioctl(struct tty_struct *tty,
 /* functions for preparation of BKL removal */
 extern void __lockfunc tty_lock(struct tty_struct *tty);
 extern void __lockfunc tty_unlock(struct tty_struct *tty);
-extern void __lockfunc tty_lock_pair(struct tty_struct *tty,
-				struct tty_struct *tty2);
-extern void __lockfunc tty_unlock_pair(struct tty_struct *tty,
-				struct tty_struct *tty2);
-
+extern void __lockfunc tty_lock_slave(struct tty_struct *tty);
+extern void __lockfunc tty_unlock_slave(struct tty_struct *tty);
 /*
  * this shall be called only from where BTM is held (like close)
  *
-- 
2.1.3

[PATCH -next v2 19/26] tty: Preset lock subclass for nested tty locks

[Peter Hurley]

Eliminate the requirement of specifying the tty lock nesting at
lock time; instead, set the lock subclass for slave ptys at pty
install (normal ttys and master ptys use subclass 0).

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c       |  2 ++
 drivers/tty/tty_mutex.c | 19 +++++++++----------
 include/linux/tty.h     |  1 +
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index bdb8fd1..11db7dc 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -399,6 +399,8 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	if (!o_tty)
 		goto err_put_module;
 
+	tty_set_lock_subclass(o_tty);
+
 	if (legacy) {
 		/* We always use new tty termios data so we can do this
 		   the easy way .. */
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index f43e995..4486741 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -13,15 +13,14 @@
 
 enum {
 	TTY_MUTEX_NORMAL,
-	TTY_MUTEX_NESTED,
+	TTY_MUTEX_SLAVE,
 };
 
 /*
  * Getting the big tty mutex.
  */
 
-static void __lockfunc tty_lock_nested(struct tty_struct *tty,
-				       unsigned int subclass)
+void __lockfunc tty_lock(struct tty_struct *tty)
 {
 	if (tty->magic != TTY_MAGIC) {
 		pr_err("L Bad %p\n", tty);
@@ -29,12 +28,7 @@ static void __lockfunc tty_lock_nested(struct tty_struct *tty,
 		return;
 	}
 	tty_kref_get(tty);
-	mutex_lock_nested(&tty->legacy_mutex, subclass);
-}
-
-void __lockfunc tty_lock(struct tty_struct *tty)
-{
-	return tty_lock_nested(tty, TTY_MUTEX_NORMAL);
+	mutex_lock(&tty->legacy_mutex);
 }
 EXPORT_SYMBOL(tty_lock);
 
@@ -56,7 +50,7 @@ void __lockfunc tty_lock_slave(struct tty_struct *tty)
 		WARN_ON(!mutex_is_locked(&tty->link->legacy_mutex) ||
 			!tty->driver->type == TTY_DRIVER_TYPE_PTY ||
 			!tty->driver->type == PTY_TYPE_SLAVE);
-		tty_lock_nested(tty, TTY_MUTEX_NESTED);
+		tty_lock(tty);
 	}
 }
 
@@ -65,3 +59,8 @@ void __lockfunc tty_unlock_slave(struct tty_struct *tty)
 	if (tty && tty != tty->link)
 		tty_unlock(tty);
 }
+
+void tty_set_lock_subclass(struct tty_struct *tty)
+{
+	lockdep_set_subclass(&tty->legacy_mutex, TTY_MUTEX_SLAVE);
+}
diff --git a/include/linux/tty.h b/include/linux/tty.h
index a07b4b4..196c352 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -640,6 +640,7 @@ extern void __lockfunc tty_lock(struct tty_struct *tty);
 extern void __lockfunc tty_unlock(struct tty_struct *tty);
 extern void __lockfunc tty_lock_slave(struct tty_struct *tty);
 extern void __lockfunc tty_unlock_slave(struct tty_struct *tty);
+extern void tty_set_lock_subclass(struct tty_struct *tty);
 /*
  * this shall be called only from where BTM is held (like close)
  *
-- 
2.1.3

[PATCH -next v2 20/26] tty: Remove tty_unhangup() declaration

[Peter Hurley]

The tty_unhangup() function is not defined.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 include/linux/tty.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/tty.h b/include/linux/tty.h
index 196c352..e33ea1a 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -435,7 +435,6 @@ extern int is_ignored(int sig);
 extern int tty_signal(int sig, struct tty_struct *tty);
 extern void tty_hangup(struct tty_struct *tty);
 extern void tty_vhangup(struct tty_struct *tty);
-extern void tty_unhangup(struct file *filp);
 extern int tty_hung_up_p(struct file *filp);
 extern void do_SAK(struct tty_struct *tty);
 extern void __do_SAK(struct tty_struct *tty);
-- 
2.1.3

[PATCH -next v2 21/26] pty: Don't drop pty master tty lock to hangup slave

[Peter Hurley]

With the revised tty lock order and lockdep annotation, claiming
the pty slave lock is now safe while still holding the pty master lock.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 11db7dc..bee9776 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -66,9 +66,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 			mutex_unlock(&devpts_mutex);
 		}
 #endif
-		tty_unlock(tty);
 		tty_vhangup(tty->link);
-		tty_lock(tty);
 	}
 }
 
-- 
2.1.3

Re: [PATCH -next v2 21/26] pty: Don't drop pty master tty lock to hangup slave

[One Thousand Gnomes]

On Wed,  5 Nov 2014 12:13:04 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> With the revised tty lock order and lockdep annotation, claiming
> the pty slave lock is now safe while still holding the pty master lock.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>

Reviewed-by: Alan Cox <alan@linux.intel.com>

[PATCH -next v2 22/26] pty, n_tty: Simplify input processing on final close

[Peter Hurley]

When releasing one end of a pty pair, that end may just have written
to the other, which the input processing worker, flush_to_ldisc(), is
still working on but has not completed the copy to the other end's
read buffer. So input may not appear to be available to a waiting
reader but yet TTY_OTHER_CLOSED is now observed. The n_tty line
discipline has worked around this by waiting for input processing
to complete and then re-checking if input is available before
exiting with -EIO.

Since the tty/ldisc lock reordering, the wait for input processing
to complete can now occur during final close before setting
TTY_OTHER_CLOSED. In this way, a waiting reader is guaranteed to
see input available (if any) before observing TTY_OTHER_CLOSED.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 46 ++++++++++++++++++++--------------------------
 drivers/tty/pty.c   |  1 +
 2 files changed, 21 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 0cb0e12..112eda7 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2197,34 +2197,28 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 
 		if (!input_available_p(tty, 0)) {
 			if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
-				up_read(&tty->termios_rwsem);
-				tty_flush_to_ldisc(tty);
-				down_read(&tty->termios_rwsem);
-				if (!input_available_p(tty, 0)) {
-					retval = -EIO;
-					break;
-				}
-			} else {
-				if (tty_hung_up_p(file))
-					break;
-				if (!timeout)
-					break;
-				if (file->f_flags & O_NONBLOCK) {
-					retval = -EAGAIN;
-					break;
-				}
-				if (signal_pending(current)) {
-					retval = -ERESTARTSYS;
-					break;
-				}
-				n_tty_set_room(tty);
-				up_read(&tty->termios_rwsem);
+				retval = -EIO;
+				break;
+			}
+			if (tty_hung_up_p(file))
+				break;
+			if (!timeout)
+				break;
+			if (file->f_flags & O_NONBLOCK) {
+				retval = -EAGAIN;
+				break;
+			}
+			if (signal_pending(current)) {
+				retval = -ERESTARTSYS;
+				break;
+			}
+			n_tty_set_room(tty);
+			up_read(&tty->termios_rwsem);
 
-				timeout = schedule_timeout(timeout);
+			timeout = schedule_timeout(timeout);
 
-				down_read(&tty->termios_rwsem);
-				continue;
-			}
+			down_read(&tty->termios_rwsem);
+			continue;
 		}
 		__set_current_state(TASK_RUNNING);
 
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index bee9776..a9d256d 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -53,6 +53,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 	/* Review - krefs on tty_link ?? */
 	if (!tty->link)
 		return;
+	tty_flush_to_ldisc(tty->link);
 	set_bit(TTY_OTHER_CLOSED, &tty->link->flags);
 	wake_up_interruptible(&tty->link->read_wait);
 	wake_up_interruptible(&tty->link->write_wait);
-- 
2.1.3

[PATCH -next v2 23/26] tty: Prefix tty_ldisc_{lock,lock_nested,unlock} functions

[Peter Hurley]

tty_ldisc_lock(), tty_ldisc_unlock(), and tty_ldisc_lock_nested()
are low-level aliases for the underlying lock mechanism. Rename
with double underscore to allow for new, higher level functions
with those names.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 1c4d7b6..5bdc241 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -308,19 +308,19 @@ EXPORT_SYMBOL_GPL(tty_ldisc_deref);
 
 
 static inline int __lockfunc
-tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
+__tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
 {
 	return ldsem_down_write(&tty->ldisc_sem, timeout);
 }
 
 static inline int __lockfunc
-tty_ldisc_lock_nested(struct tty_struct *tty, unsigned long timeout)
+__tty_ldisc_lock_nested(struct tty_struct *tty, unsigned long timeout)
 {
 	return ldsem_down_write_nested(&tty->ldisc_sem,
 				       LDISC_SEM_OTHER, timeout);
 }
 
-static inline void tty_ldisc_unlock(struct tty_struct *tty)
+static inline void __tty_ldisc_unlock(struct tty_struct *tty)
 {
 	return ldsem_up_write(&tty->ldisc_sem);
 }
@@ -332,24 +332,24 @@ tty_ldisc_lock_pair_timeout(struct tty_struct *tty, struct tty_struct *tty2,
 	int ret;
 
 	if (tty < tty2) {
-		ret = tty_ldisc_lock(tty, timeout);
+		ret = __tty_ldisc_lock(tty, timeout);
 		if (ret) {
-			ret = tty_ldisc_lock_nested(tty2, timeout);
+			ret = __tty_ldisc_lock_nested(tty2, timeout);
 			if (!ret)
-				tty_ldisc_unlock(tty);
+				__tty_ldisc_unlock(tty);
 		}
 	} else {
 		/* if this is possible, it has lots of implications */
 		WARN_ON_ONCE(tty == tty2);
 		if (tty2 && tty != tty2) {
-			ret = tty_ldisc_lock(tty2, timeout);
+			ret = __tty_ldisc_lock(tty2, timeout);
 			if (ret) {
-				ret = tty_ldisc_lock_nested(tty, timeout);
+				ret = __tty_ldisc_lock_nested(tty, timeout);
 				if (!ret)
-					tty_ldisc_unlock(tty2);
+					__tty_ldisc_unlock(tty2);
 			}
 		} else
-			ret = tty_ldisc_lock(tty, timeout);
+			ret = __tty_ldisc_lock(tty, timeout);
 	}
 
 	if (!ret)
@@ -370,9 +370,9 @@ tty_ldisc_lock_pair(struct tty_struct *tty, struct tty_struct *tty2)
 static void __lockfunc tty_ldisc_unlock_pair(struct tty_struct *tty,
 					     struct tty_struct *tty2)
 {
-	tty_ldisc_unlock(tty);
+	__tty_ldisc_unlock(tty);
 	if (tty2)
-		tty_ldisc_unlock(tty2);
+		__tty_ldisc_unlock(tty2);
 }
 
 static void __lockfunc tty_ldisc_enable_pair(struct tty_struct *tty,
-- 
2.1.3

[PATCH -next v2 24/26] tty: Fix hung task on pty hangup

[Peter Hurley]

When hanging up one end of a pty pair, there may be waiting
readers/writers on the other end which may not exit, preventing
tty_ldisc_lock_pair() from acquiring the other side's ldisc lock.

Only acquire this side's ldisc lock; although this will no longer
prevent the other side from writing new input, that input will not
be processing until after the ldisc hangup is complete.

Reported-by: Sasha Levin <sasha.levin@oracle.com>
Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 5bdc241..1dbe278 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -326,6 +326,24 @@ static inline void __tty_ldisc_unlock(struct tty_struct *tty)
 }
 
 static int __lockfunc
+tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
+{
+	int ret;
+
+	ret = __tty_ldisc_lock(tty, timeout);
+	if (!ret)
+		return -EBUSY;
+	set_bit(TTY_LDISC_HALTED, &tty->flags);
+	return 0;
+}
+
+static void tty_ldisc_unlock(struct tty_struct *tty)
+{
+	clear_bit(TTY_LDISC_HALTED, &tty->flags);
+	__tty_ldisc_unlock(tty);
+}
+
+static int __lockfunc
 tty_ldisc_lock_pair_timeout(struct tty_struct *tty, struct tty_struct *tty2,
 			    unsigned long timeout)
 {
@@ -682,7 +700,7 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 	 *
 	 * Avoid racing set_ldisc or tty_ldisc_release
 	 */
-	tty_ldisc_lock_pair(tty, tty->link);
+	tty_ldisc_lock(tty, MAX_SCHEDULE_TIMEOUT);
 
 	if (tty->ldisc) {
 
@@ -704,7 +722,7 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 			WARN_ON(tty_ldisc_open(tty, tty->ldisc));
 		}
 	}
-	tty_ldisc_enable_pair(tty, tty->link);
+	tty_ldisc_unlock(tty);
 	if (reset)
 		tty_reset_termios(tty);
 
-- 
2.1.3

[PATCH -next v2 25/26] tty: Fix timeout on pty set ldisc

[Peter Hurley]

When changing the ldisc on one end of a pty pair, there may be
waiting readers/writers on the other end which may not exit from
the ldisc i/o loop, preventing tty_ldisc_lock_pair_timeout() from
acquiring the other side's ldisc lock.

Only acquire this side's ldisc lock; although this will no longer
prevent the other side from writing new input, that input will not
be processed until after the ldisc change completes. This has no
effect on normal ttys; new input from the driver was never disabled.

Remove tty_ldisc_enable_pair().

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ldisc.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 1dbe278..6368dd9 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -393,16 +393,6 @@ static void __lockfunc tty_ldisc_unlock_pair(struct tty_struct *tty,
 		__tty_ldisc_unlock(tty2);
 }
 
-static void __lockfunc tty_ldisc_enable_pair(struct tty_struct *tty,
-					     struct tty_struct *tty2)
-{
-	clear_bit(TTY_LDISC_HALTED, &tty->flags);
-	if (tty2)
-		clear_bit(TTY_LDISC_HALTED, &tty2->flags);
-
-	tty_ldisc_unlock_pair(tty, tty2);
-}
-
 /**
  *	tty_ldisc_flush	-	flush line discipline queue
  *	@tty: tty
@@ -535,14 +525,13 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 {
 	int retval;
 	struct tty_ldisc *old_ldisc, *new_ldisc;
-	struct tty_struct *o_tty = tty->link;
 
 	new_ldisc = tty_ldisc_get(tty, ldisc);
 	if (IS_ERR(new_ldisc))
 		return PTR_ERR(new_ldisc);
 
 	tty_lock(tty);
-	retval = tty_ldisc_lock_pair_timeout(tty, o_tty, 5 * HZ);
+	retval = tty_ldisc_lock(tty, 5 * HZ);
 	if (retval) {
 		tty_ldisc_put(new_ldisc);
 		tty_unlock(tty);
@@ -554,7 +543,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	 */
 
 	if (tty->ldisc->ops->num == ldisc) {
-		tty_ldisc_enable_pair(tty, o_tty);
+		tty_ldisc_unlock(tty);
 		tty_ldisc_put(new_ldisc);
 		tty_unlock(tty);
 		return 0;
@@ -565,7 +554,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	if (test_bit(TTY_HUPPED, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
 		   the ldisc data and closed the ldisc down */
-		tty_ldisc_enable_pair(tty, o_tty);
+		tty_ldisc_unlock(tty);
 		tty_ldisc_put(new_ldisc);
 		tty_unlock(tty);
 		return -EIO;
@@ -599,13 +588,11 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	/*
 	 *	Allow ldisc referencing to occur again
 	 */
-	tty_ldisc_enable_pair(tty, o_tty);
+	tty_ldisc_unlock(tty);
 
 	/* Restart the work queue in case no characters kick it off. Safe if
 	   already running */
 	schedule_work(&tty->port->buf.work);
-	if (o_tty)
-		schedule_work(&o_tty->port->buf.work);
 
 	tty_unlock(tty);
 	return retval;
-- 
2.1.3

[PATCH -next v2 26/26] tty: Flush ldisc buffer atomically with tty flip buffers

[Peter Hurley]

tty_ldisc_flush() first clears the line discipline input buffer,
then clears the tty flip buffers. However, this allows for existing
data in the tty flip buffers to be added after the ldisc input
buffer has been cleared, but before the flip buffers have been cleared.

Add an optional ldisc parameter to tty_buffer_flush() to allow
tty_ldisc_flush() to pass the ldisc to clear.

NB: Initially, the plan was to do this automatically in
tty_buffer_flush(). However, an audit of the behavior of existing
line disciplines showed that performing a ldisc buffer flush on
ioctl(TCFLSH) was not always the outcome. For example, some line
disciplines have flush_buffer() methods but not ioctl() methods,
so a ->flush_buffer() command would be unexpected.

Reviewed-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_buffer.c | 10 ++++++++--
 drivers/tty/tty_io.c     |  2 +-
 drivers/tty/tty_ldisc.c  | 12 +++++-------
 include/linux/tty.h      |  2 +-
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 143deb6..3605103 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -202,14 +202,16 @@ static void tty_buffer_free(struct tty_port *port, struct tty_buffer *b)
 /**
  *	tty_buffer_flush		-	flush full tty buffers
  *	@tty: tty to flush
+ *	@ld:  optional ldisc ptr (must be referenced)
  *
- *	flush all the buffers containing receive data.
+ *	flush all the buffers containing receive data. If ld != NULL,
+ *	flush the ldisc input buffer.
  *
  *	Locking: takes buffer lock to ensure single-threaded flip buffer
  *		 'consumer'
  */
 
-void tty_buffer_flush(struct tty_struct *tty)
+void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
 {
 	struct tty_port *port = tty->port;
 	struct tty_bufhead *buf = &port->buf;
@@ -223,6 +225,10 @@ void tty_buffer_flush(struct tty_struct *tty)
 		buf->head = next;
 	}
 	buf->head->read = buf->head->commit;
+
+	if (ld && ld->ops->flush_buffer)
+		ld->ops->flush_buffer(tty);
+
 	atomic_dec(&buf->priority);
 	mutex_unlock(&buf->lock);
 }
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index fabf4c0..4bd48f7 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2899,7 +2899,7 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		case TCIFLUSH:
 		case TCIOFLUSH:
 		/* flush tty buffer and allow ldisc to process ioctl */
-			tty_buffer_flush(tty);
+			tty_buffer_flush(tty, NULL);
 			break;
 		}
 		break;
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 6368dd9..b66a81d 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -397,19 +397,17 @@ static void __lockfunc tty_ldisc_unlock_pair(struct tty_struct *tty,
  *	tty_ldisc_flush	-	flush line discipline queue
  *	@tty: tty
  *
- *	Flush the line discipline queue (if any) for this tty. If there
- *	is no line discipline active this is a no-op.
+ *	Flush the line discipline queue (if any) and the tty flip buffers
+ *	for this tty.
  */
 
 void tty_ldisc_flush(struct tty_struct *tty)
 {
 	struct tty_ldisc *ld = tty_ldisc_ref(tty);
-	if (ld) {
-		if (ld->ops->flush_buffer)
-			ld->ops->flush_buffer(tty);
+
+	tty_buffer_flush(tty, ld);
+	if (ld)
 		tty_ldisc_deref(ld);
-	}
-	tty_buffer_flush(tty);
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_flush);
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index e33ea1a..f6835ea 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -441,7 +441,7 @@ extern void __do_SAK(struct tty_struct *tty);
 extern void no_tty(void);
 extern void tty_flush_to_ldisc(struct tty_struct *tty);
 extern void tty_buffer_free_all(struct tty_port *port);
-extern void tty_buffer_flush(struct tty_struct *tty);
+extern void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld);
 extern void tty_buffer_init(struct tty_port *port);
 extern speed_t tty_termios_baud_rate(struct ktermios *termios);
 extern speed_t tty_termios_input_baud_rate(struct ktermios *termios);
-- 
2.1.3