tty locking changes

PATCH Superseded Peter Hurley 3 participants 66 messages Oct 16, 2014 – Nov 11, 2014 TTY layer and serial drivers

lore

Share & tools

lore

Peter Hurley

author

2014-10-16 20:25 UTC #0 lore raw

cover tty locking changes

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(-)

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:24 UTC #1 lore raw

PATCH 1/27 tty: Don't hold tty_lock for ldisc release

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 <[email protected]>

---
 drivers/tty/tty_ldisc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/tty/tty_ldisc.c +2 -4

index 2d822aa..332a622 100644
@@ -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

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #2 lore raw

PATCH 2/27 tty: Invert tty_lock/ldisc_sem lock order

In reply to 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 <[email protected]>

---
 drivers/tty/tty_ldisc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/tty/tty_ldisc.c +3 -4

index 332a622..28858eb 100644
@@ -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) {

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #3 lore raw

PATCH 3/27 tty: Remove TTY_HUPPING

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c    | 12 +-----------
 drivers/tty/tty_ldisc.c |  3 +--
 include/linux/tty.h     |  1 -
 3 files changed, 2 insertions(+), 14 deletions(-)

drivers/tty/tty_io.c +1 ~~-11~~

index e18491a..fd06512 100644
@@ -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 &&

drivers/tty/tty_ldisc.c +1 -2

index 28858eb..49001fa 100644
@@ -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);

include/linux/tty.h +0 -1

index b36b0b4..ff0dd7a 100644
@@ -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))

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #4 lore raw

PATCH 4/27 tty: Clarify re-open behavior of master ptys

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

drivers/tty/tty_io.c +4 ~~-11~~

index fd06512..5b6253f 100644
@@ -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);

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #5 lore raw

PATCH 5/27 tty: Check tty->count instead of TTY_CLOSING in tty_reopen()

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/tty/tty_io.c +1 -1

index 5b6253f..52bcdd3 100644
@@ -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 &&

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #6 lore raw

PATCH 6/27 pty: Always return -EIO if slave BSD pty opened first

In reply to 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 <[email protected]>

---
 drivers/tty/pty.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/tty/pty.c +5 -2

index 7a1a538..bdb8fd1 100644
@@ -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)

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #7 lore raw

PATCH 7/27 tty: Re-open /dev/tty without tty_mutex

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 53 ++++++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

drivers/tty/tty_io.c +27 ~~-26~~

index 52bcdd3..1b6f38c 100644
@@ -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;

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #8 lore raw

PATCH 8/27 tty: Drop tty_mutex before tty reopen

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

drivers/tty/tty_io.c +17 -8

index 1b6f38c..ded0445 100644
@@ -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);
 	}

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #9 lore raw

PATCH 9/27 tty: Remove TTY_CLOSING

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 14 ++------------
 include/linux/tty.h  |  1 -
 2 files changed, 2 insertions(+), 13 deletions(-)

drivers/tty/tty_io.c +2 ~~-12~~

index ded0445..55f9931 100644
@@ -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 ... */

include/linux/tty.h +0 -1

index ff0dd7a..35b29f0 100644
@@ -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 */

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 21:01 UTC #10 lore raw

PATCH 10/27 tty: Don't take tty_mutex for tty count changes

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 6 ------
 1 file changed, 6 deletions(-)

drivers/tty/tty_io.c +0 -6

index 55f9931..7b40247 100644
@@ -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 */

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #11 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/tty/tty_io.c +2 -6

index 7b40247..50118ce 100644
@@ -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;

Signature · 1 line

2.1.1

One Thousand Gnomes

2014-10-22 15:29 UTC #12 lore raw

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

Quoted 5 lines

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

Peter Hurley

author

2014-10-22 17:34 UTC #13 lore raw

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

Quoted 10 lines · One Thousand Gnomes

Quoted 5 lines
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.

Quoted 6 lines

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

One Thousand Gnomes

2014-10-23 11:30 UTC #14 lore raw

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

Quoted 2 lines

(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

Peter Hurley

author

2014-10-16 20:25 UTC #15 lore raw

PATCH 12/27 tty: Document check_tty_count() requires tty_lock held

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 1 +
 1 file changed, 1 insertion(+)

drivers/tty/tty_io.c +1 -0

index 50118ce..5c09cc4 100644
@@ -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

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #16 lore raw

PATCH 13/27 tty: Simplify pty pair teardown logic

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

drivers/tty/tty_io.c +10 ~~-19~~

index 5c09cc4..978896d 100644
@@ -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

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #17 lore raw

PATCH 14/27 tty: Fold pty pair handling into tty_flush_works()

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/tty/tty_io.c +7 -5

index 978896d..6a3be01 100644
@@ -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));

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #18 lore raw

PATCH 15/27 tty: Simplify tty_ldisc_release() interface

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c    |  2 +-
 drivers/tty/tty_ldisc.c | 15 +++++++--------
 include/linux/tty.h     |  2 +-
 3 files changed, 9 insertions(+), 10 deletions(-)

drivers/tty/tty_io.c +1 -1

index 6a3be01..5c782dc 100644
@@ -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);

drivers/tty/tty_ldisc.c +7 -8

index 49001fa..1c4d7b6 100644
@@ -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.

include/linux/tty.h +1 -1

index 35b29f0..af1a7f3 100644
@@ -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);

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #19 lore raw

PATCH 16/27 tty: Simplify tty_release_checks() interface

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/tty/tty_io.c +4 -3

index 5c782dc..8fedd22 100644
@@ -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;
 	}

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #20 lore raw

PATCH 17/27 tty: Simplify tty_release() state checks

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

drivers/tty/tty_io.c +9 ~~-10~~

index 8fedd22..fb043b9 100644
@@ -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

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:49 UTC #21 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c    | 10 ++++++----
 drivers/tty/tty_mutex.c | 32 +++++++++++++-------------------
 include/linux/tty.h     |  7 ++-----
 3 files changed, 21 insertions(+), 28 deletions(-)

drivers/tty/tty_io.c +6 -4

index fb043b9..58015b3 100644
@@ -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 */

drivers/tty/tty_mutex.c +13 ~~-19~~

index 2e41abe..f43e995 100644
@@ -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);

include/linux/tty.h +2 -5

index af1a7f3..a07b4b4 100644
@@ -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)
  *

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #22 lore raw

PATCH 19/27 tty: Remove tty_unhangup() declaration

In reply to Peter Hurley

The tty_unhangup() function is not defined.

Signed-off-by Peter Hurley <[email protected]>

---
 include/linux/tty.h | 1 -
 1 file changed, 1 deletion(-)

include/linux/tty.h +0 -1

index a07b4b4..d470a86 100644
@@ -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);

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 21:02 UTC #23 lore raw

PATCH 20/27 tty: Refactor __tty_hangup to enable lockdep annotation

In reply to 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 <[email protected]>

---
 drivers/tty/tty_io.c | 51 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

drivers/tty/tty_io.c +30 ~~-21~~

index 58015b3..48c1def 100644
@@ -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);
 }
 
 /**

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-27 22:13 UTC #24 lore raw

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

Quoted 9 lines · 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.

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

Peter Hurley

author

2014-10-16 21:02 UTC #25 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/tty/pty.c    |  4 +---
 drivers/tty/tty_io.c | 20 ++++++++++++++++++++
 include/linux/tty.h  |  1 +
 3 files changed, 22 insertions(+), 3 deletions(-)

drivers/tty/pty.c +1 -3

index bdb8fd1..09fb513 100644
@@ -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);
 	}
 }

drivers/tty/tty_io.c +20 -0

index 48c1def..25e85b0 100644
@@ -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

include/linux/tty.h +1 -0

index d470a86..8a90253 100644
@@ -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);

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 21:02 UTC #26 lore raw

PATCH 22/27 tty: Document hangup call tree

In reply to Peter Hurley

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

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/tty/tty_io.c +13 -0

index 25e85b0..8effd44 100644
@@ -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)
 {

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 21:00 UTC #27 lore raw

PATCH 23/27 pty, n_tty: Simplify input processing on final close

In reply to 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 <[email protected]>

---
 drivers/tty/n_tty.c | 46 ++++++++++++++++++++--------------------------
 drivers/tty/pty.c   |  1 +
 2 files changed, 21 insertions(+), 26 deletions(-)

drivers/tty/n_tty.c +20 ~~-26~~

index 38ac7b9..a8ec82d 100644
@@ -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);

drivers/tty/pty.c +1 -0

index 09fb513..461ca53 100644
@@ -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);

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #28 lore raw

PATCH 24/27 tty: Prefix tty_ldisc_{lock,lock_nested,unlock} functions

In reply to 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 <[email protected]>

---
 drivers/tty/tty_ldisc.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

drivers/tty/tty_ldisc.c +12 ~~-12~~

index 1c4d7b6..5bdc241 100644
@@ -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,

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:25 UTC #29 lore raw

PATCH 25/27 tty: Fix hung task on pty hangup

In reply to 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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_ldisc.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

drivers/tty/tty_ldisc.c +20 -2

index 5bdc241..1dbe278 100644
@@ -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);

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:49 UTC #30 lore raw

PATCH 26/27 tty: Fix timeout on pty set ldisc

In reply to 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 <[email protected]>

---
 drivers/tty/tty_ldisc.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

drivers/tty/tty_ldisc.c +4 ~~-17~~

index 1dbe278..6368dd9 100644
@@ -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;

Signature · 1 line

2.1.1

Peter Hurley

author

2014-10-16 20:49 UTC #31 lore raw

PATCH 27/27 tty: Flush ldisc buffer atomically with tty flip buffers

In reply to 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 <[email protected]>

---
 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(-)

drivers/tty/tty_buffer.c +8 -2

index 143deb6..3605103 100644
@@ -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);
 }

drivers/tty/tty_io.c +1 -1

index 8effd44..2931f42 100644
@@ -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;

drivers/tty/tty_ldisc.c +5 -7

index 6368dd9..b66a81d 100644
@@ -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);

include/linux/tty.h +1 -1

index 8a90253..90256ee 100644
@@ -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);

Signature · 1 line

2.1.1

One Thousand Gnomes

Reviewed-by Alan Cox on the v1 series

2014-10-22 15:32 UTC #32 lore raw

In reply to Peter Hurley

Quoted 20 lines · On Thu, 16 Oct 2014 16:24:58 -0400 Peter Hurley <[email protected]>…

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.

Quoted 4 lines

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 <[email protected]>

Peter Hurley

author

2014-11-05 17:13 UTC #33 lore raw

v2 cover tty locking changes

In reply to 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,

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(-)

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:13 UTC #34 lore raw

PATCH v2 1/26 tty: Don't hold tty_lock for ldisc release

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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_ldisc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/tty/tty_ldisc.c +2 -4

index 2d822aa..332a622 100644
@@ -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

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:12 UTC #35 lore raw

PATCH v2 2/26 tty: Invert tty_lock/ldisc_sem lock order

In reply to Peter Hurley

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_ldisc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/tty/tty_ldisc.c +3 -4

index 332a622..28858eb 100644
@@ -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) {

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:12 UTC #36 lore raw

PATCH v2 3/26 tty: Remove TTY_HUPPING

In reply to 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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c    | 12 +-----------
 drivers/tty/tty_ldisc.c |  3 +--
 include/linux/tty.h     |  1 -
 3 files changed, 2 insertions(+), 14 deletions(-)

drivers/tty/tty_io.c +1 ~~-11~~

index 45cc09e..bb4d2f2 100644
@@ -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 &&

drivers/tty/tty_ldisc.c +1 -2

index 28858eb..49001fa 100644
@@ -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);

include/linux/tty.h +0 -1

index b36b0b4..ff0dd7a 100644
@@ -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))

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:12 UTC #37 lore raw

PATCH v2 4/26 tty: Clarify re-open behavior of master ptys

In reply to Peter Hurley

      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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

drivers/tty/tty_io.c +4 ~~-11~~

index bb4d2f2..6dc3fa0 100644
@@ -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);

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:12 UTC #38 lore raw

PATCH v2 5/26 tty: Check tty->count instead of TTY_CLOSING in tty_reopen()

In reply to Peter Hurley

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

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/tty/tty_io.c +1 -1

index 6dc3fa0..97445c2 100644
@@ -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 &&

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:12 UTC #39 lore raw

PATCH v2 6/26 pty: Always return -EIO if slave BSD pty opened first

In reply to Peter Hurley

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

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/pty.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/tty/pty.c +5 -2

index 7a1a538..bdb8fd1 100644
@@ -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)

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:12 UTC #40 lore raw

PATCH v2 7/26 tty: Re-open /dev/tty without tty_mutex

In reply to Peter Hurley

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

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.

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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 53 ++++++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

drivers/tty/tty_io.c +27 ~~-26~~

index 97445c2..fb4583c 100644
@@ -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;

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:13 UTC #41 lore raw

PATCH v2 8/26 tty: Drop tty_mutex before tty reopen

In reply to Peter Hurley

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

drivers/tty/tty_io.c +17 -8

index fb4583c..fb74107 100644
@@ -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);
 	}

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:13 UTC #42 lore raw

PATCH v2 9/26 tty: Remove TTY_CLOSING

In reply to 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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 14 ++------------
 include/linux/tty.h  |  1 -
 2 files changed, 2 insertions(+), 13 deletions(-)

drivers/tty/tty_io.c +2 ~~-12~~

index fb74107..49167ed 100644
@@ -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 ... */

include/linux/tty.h +0 -1

index ff0dd7a..35b29f0 100644
@@ -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 */

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:49 UTC #43 lore raw

PATCH v2 10/26 tty: Don't take tty_mutex for tty count changes

In reply to 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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 6 ------
 1 file changed, 6 deletions(-)

drivers/tty/tty_io.c +0 -6

index 49167ed..f1c4b07 100644
@@ -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 */

Signature · 1 line

2.1.3

Greg Kroah-Hartman

maintainer

2014-11-06 02:33 UTC #44 lore raw

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

Quoted 26 lines · 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 <[email protected]>
Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/tty_io.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/tty/tty_io.c +0 -4
index ea8c6cae8d12..e59de81c39a9 100644
@@ -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 &&

drivers/tty/tty_io.c int tty_release(struct inode *inode, struct file *filp) · line 1841

Quoted 7 lines

drivers/tty/tty_io.c +0 -1

@@ -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

drivers/tty/tty_io.c +0 -6

index ea8c6cae8d12..e59de81c39a9 100644
@@ -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 */

Peter Hurley

author

2014-11-06 02:39 UTC #45 lore raw

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

Quoted 29 lines · Greg Kroah-Hartman

Quoted 28 lines · 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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 6 ------
 1 file changed, 6 deletions(-)

drivers/tty/tty_io.c +0 -4

index ea8c6cae8d12..e59de81c39a9 100644
@@ -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));

Quoted 8 lines

drivers/tty/tty_io.c int tty_release(struct inode *inode, struct file *filp) · line 1841
Quoted 5 lines
drivers/tty/tty_io.c +0 -1
 		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

Quoted 41 lines

I've included the version
I used below, please verify I didn't mess it up.

thanks,

greg k-h

drivers/tty/tty_io.c +0 -6

index ea8c6cae8d12..e59de81c39a9 100644
@@ -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 */

Peter Hurley

author

2014-11-06 02:50 UTC #46 lore raw

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

Quoted 39 lines · Peter Hurley

Quoted 29 lines · Greg Kroah-Hartman
Quoted 28 lines · 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 <[email protected]>
Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/tty_io.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/tty/tty_io.c +0 -4
index ea8c6cae8d12..e59de81c39a9 100644
@@ -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));
Quoted 8 lines
drivers/tty/tty_io.c int tty_release(struct inode *inode, struct file *filp) · line 1841
Quoted 5 lines
drivers/tty/tty_io.c +0 -1
 		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.

Quoted 7 lines

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

Quoted 47 lines

Quoted 34 lines

I've included the version
I used below, please verify I didn't mess it up.

thanks,

greg k-h

drivers/tty/tty_io.c +0 -5

index ea8c6cae8d12..e59de81c39a9 100644
@@ -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();
 	}

drivers/tty/tty_io.c int tty_release(struct inode *inode, struct file *filp) · line 1891

Quoted 7 lines

drivers/tty/tty_io.c +0 -1

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Greg Kroah-Hartman

maintainer

2014-11-06 03:46 UTC #47 lore raw

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

Quoted 48 lines · Peter Hurley

Quoted 39 lines · Peter Hurley
Quoted 29 lines · Greg Kroah-Hartman
Quoted 28 lines · 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 <[email protected]>
Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/tty_io.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/tty/tty_io.c +0 -4
index ea8c6cae8d12..e59de81c39a9 100644
@@ -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));
Quoted 8 lines
drivers/tty/tty_io.c int tty_release(struct inode *inode, struct file *filp) · line 1841
Quoted 5 lines
drivers/tty/tty_io.c +0 -1
 		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.
Quoted 7 lines
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'?

Quoted 8 lines

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

Peter Hurley

author

2014-11-05 17:12 UTC #48 lore raw

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

In reply to Peter Hurley

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/tty/tty_io.c +2 -6

index f1c4b07..276d939 100644
@@ -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;

Signature · 1 line

2.1.3

Greg Kroah-Hartman

maintainer

2014-11-06 02:40 UTC #49 lore raw

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

Quoted 40 lines · 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 <[email protected]>
Signed-off-by Peter Hurley <[email protected]>
---
 drivers/tty/tty_io.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/tty/tty_io.c +2 -5
index e59de81c39a9..b008e2b38d54 100644
@@ -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));

drivers/tty/tty_io.c int tty_release(struct inode *inode, struct file *filp) · line 1837

Quoted 7 lines

drivers/tty/tty_io.c +0 -1

@@ -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:

drivers/tty/tty_io.c +2 -6

index e59de81c39a9..b008e2b38d54 100644
@@ -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();
 	}

Peter Hurley

author

2014-11-05 17:12 UTC #50 lore raw

PATCH v2 12/26 tty: Document check_tty_count() requires tty_lock held

In reply to 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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 1 +
 1 file changed, 1 insertion(+)

drivers/tty/tty_io.c +1 -0

index 276d939..41eeee0 100644
@@ -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

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:12 UTC #51 lore raw

PATCH v2 13/26 tty: Simplify pty pair teardown logic

In reply to Peter Hurley

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

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

drivers/tty/tty_io.c +10 ~~-19~~

index 41eeee0..7b91007 100644
@@ -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

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:12 UTC #52 lore raw

PATCH v2 14/26 tty: Fold pty pair handling into tty_flush_works()

In reply to Peter Hurley

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

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/tty/tty_io.c +7 -5

index 7b91007..563ab37 100644
@@ -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));

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:12 UTC #53 lore raw

PATCH v2 15/26 tty: Simplify tty_ldisc_release() interface

In reply to 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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c    |  2 +-
 drivers/tty/tty_ldisc.c | 15 +++++++--------
 include/linux/tty.h     |  2 +-
 3 files changed, 9 insertions(+), 10 deletions(-)

drivers/tty/tty_io.c +1 -1

index 563ab37..4034e5e 100644
@@ -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);

drivers/tty/tty_ldisc.c +7 -8

index 49001fa..1c4d7b6 100644
@@ -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.

include/linux/tty.h +1 -1

index 35b29f0..af1a7f3 100644
@@ -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);

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:49 UTC #54 lore raw

PATCH v2 16/26 tty: Simplify tty_release_checks() interface

In reply to 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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/tty/tty_io.c +4 -3

index 4034e5e..c765260 100644
@@ -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;
 	}

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:13 UTC #55 lore raw

PATCH v2 17/26 tty: Simplify tty_release() state checks

In reply to 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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

drivers/tty/tty_io.c +9 ~~-10~~

index c765260..bbc940d 100644
@@ -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

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:13 UTC #56 lore raw

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

In reply to Peter Hurley

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 <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_io.c    | 10 ++++++----
 drivers/tty/tty_mutex.c | 32 +++++++++++++-------------------
 include/linux/tty.h     |  7 ++-----
 3 files changed, 21 insertions(+), 28 deletions(-)

drivers/tty/tty_io.c +6 -4

index bbc940d..fabf4c0 100644
@@ -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 */

drivers/tty/tty_mutex.c +13 ~~-19~~

index 2e41abe..f43e995 100644
@@ -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);

include/linux/tty.h +2 -5

index af1a7f3..a07b4b4 100644
@@ -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)
  *

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:49 UTC #57 lore raw

PATCH v2 19/26 tty: Preset lock subclass for nested tty locks

In reply to 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 <[email protected]>

---
 drivers/tty/pty.c       |  2 ++
 drivers/tty/tty_mutex.c | 19 +++++++++----------
 include/linux/tty.h     |  1 +
 3 files changed, 12 insertions(+), 10 deletions(-)

drivers/tty/pty.c +2 -0

index bdb8fd1..11db7dc 100644
@@ -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 .. */

drivers/tty/tty_mutex.c +9 ~~-10~~

index f43e995..4486741 100644
@@ -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);
+}

include/linux/tty.h +1 -0

index a07b4b4..196c352 100644
@@ -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)
  *

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:50 UTC #58 lore raw

PATCH v2 20/26 tty: Remove tty_unhangup() declaration

In reply to Peter Hurley

The tty_unhangup() function is not defined.

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 include/linux/tty.h | 1 -
 1 file changed, 1 deletion(-)

include/linux/tty.h +0 -1

index 196c352..e33ea1a 100644
@@ -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);

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:13 UTC #59 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/tty/pty.c | 2 --
 1 file changed, 2 deletions(-)

drivers/tty/pty.c +0 -2

index 11db7dc..bee9776 100644
@@ -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);
 	}
 }

Signature · 1 line

2.1.3

One Thousand Gnomes

Reviewed-by Alan Cox on v2 21/26

2014-11-11 15:49 UTC #60 lore raw

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

Quoted 4 lines · On Wed, 5 Nov 2014 12:13:04 -0500 Peter Hurley <[email protected]>…

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 <[email protected]>

Reviewed-by Alan Cox <[email protected]>

Peter Hurley

author

2014-11-05 17:13 UTC #61 lore raw

PATCH v2 22/26 pty, n_tty: Simplify input processing on final close

In reply to Peter Hurley

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 46 ++++++++++++++++++++--------------------------
 drivers/tty/pty.c   |  1 +
 2 files changed, 21 insertions(+), 26 deletions(-)

drivers/tty/n_tty.c +20 ~~-26~~

index 0cb0e12..112eda7 100644
@@ -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);

drivers/tty/pty.c +1 -0

index bee9776..a9d256d 100644
@@ -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);

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:13 UTC #62 lore raw

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

In reply to Peter Hurley

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_ldisc.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

drivers/tty/tty_ldisc.c +12 ~~-12~~

index 1c4d7b6..5bdc241 100644
@@ -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,

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:13 UTC #63 lore raw

PATCH v2 24/26 tty: Fix hung task on pty hangup

In reply to 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 <[email protected]>

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_ldisc.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

drivers/tty/tty_ldisc.c +20 -2

index 5bdc241..1dbe278 100644
@@ -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);

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:13 UTC #64 lore raw

PATCH v2 25/26 tty: Fix timeout on pty set ldisc

In reply to Peter Hurley

Remove tty_ldisc_enable_pair().

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_ldisc.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

drivers/tty/tty_ldisc.c +4 ~~-17~~

index 1dbe278..6368dd9 100644
@@ -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;

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 17:13 UTC #65 lore raw

PATCH v2 26/26 tty: Flush ldisc buffer atomically with tty flip buffers

In reply to Peter Hurley

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

Reviewed-by Alan Cox <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 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(-)

drivers/tty/tty_buffer.c +8 -2

index 143deb6..3605103 100644
@@ -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);
 }

drivers/tty/tty_io.c +1 -1

index fabf4c0..4bd48f7 100644
@@ -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;

drivers/tty/tty_ldisc.c +5 -7

index 6368dd9..b66a81d 100644
@@ -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);

include/linux/tty.h +1 -1

index e33ea1a..f6835ea 100644
@@ -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);

Signature · 1 line

2.1.3