[PATCH 00/12] Rework tty_reopen()

[PATCH 00/12] Rework tty_reopen()

[Peter Hurley]

Hi Greg,

This patch series implements two important improvements to tty open()
behavior: interruptible open() and automatic retry when tty teardown
has already commenced.

Interruptible open() allows signals to cancel the open wait if stalled
waiting for tty teardown to complete.

Automatic retry of tty open() when racing a tty teardown now makes tty
open() fully POSIX compliant. For some time, the Linux kernel has
returned EIO from open() under certain circumstances. This happens when
tty_open() observes a valid tty from driver lookup but the tty is
being released (in final close) and teardown is about to commence.

The observable userspace change is that userspace will no longer need
to retry open() on EIO error.

This series also continues the ongoing effort to cleanup and reduce the
kernel tty interface.

Lastly, this series lays important groundwork for implementing ptmx_open()
in tty_open(), trivially with driver lookup (still a work-in-progress).

Regards,

Peter Hurley (12):
  tty: Fix ldisc leak in failed tty_init_dev()
  tty: Remove !tty check from free_tty_struct()
  tty: Fix tty_init_termios() declaration
  tty: Re-declare tty_driver_remove_tty() file scope
  pty: Remove pty_unix98_shutdown()
  tty: Remove __lockfunc annotation from tty lock functions
  tty: Wait interruptibly for tty lock on reopen
  pty: Prepare to redefine tty driver remove() interface
  tty: Re-define tty driver remove() interface
  tty: Consolidate noctty checks in tty_open()
  tty: Refactor tty_open()
  tty: Retry failed reopen if tty teardown in-progress

 drivers/tty/pty.c            |  40 +++-------
 drivers/tty/tty_io.c         | 180 +++++++++++++++++++++----------------------
 drivers/tty/tty_ldisc.c      |  21 +++--
 drivers/tty/tty_mutex.c      |  16 +++-
 drivers/usb/serial/console.c |   6 +-
 include/linux/tty.h          |  19 ++---
 include/linux/tty_driver.h   |   4 +-
 7 files changed, 128 insertions(+), 158 deletions(-)

-- 
2.6.3

[PATCH 01/12] tty: Fix ldisc leak in failed tty_init_dev()

[Peter Hurley]

release_tty() leaks the ldisc instance when called directly (rather
than when releasing the file descriptor from tty_release()).

Since tty_ldisc_release() clears tty->ldisc, releasing the ldisc
instance at tty teardown if tty->ldisc is non-null is not in danger
of double-releasing the ldisc.

Remove deinitialize_tty_struct() now that free_tty_struct() always
performs the tty_ldisc_deinit().

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

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index b311004..8cbe802 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -408,7 +408,7 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 		   the easy way .. */
 		retval = tty_init_termios(tty);
 		if (retval)
-			goto err_deinit_tty;
+			goto err_free_tty;
 
 		retval = tty_init_termios(o_tty);
 		if (retval)
@@ -447,8 +447,7 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 err_free_termios:
 	if (legacy)
 		tty_free_termios(tty);
-err_deinit_tty:
-	deinitialize_tty_struct(o_tty);
+err_free_tty:
 	free_tty_struct(o_tty);
 err_put_module:
 	module_put(driver->other->owner);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index eda8715..153b7b8 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -173,6 +173,7 @@ void free_tty_struct(struct tty_struct *tty)
 {
 	if (!tty)
 		return;
+	tty_ldisc_deinit(tty);
 	put_device(tty->dev);
 	kfree(tty->write_buf);
 	tty->magic = 0xDEADDEAD;
@@ -1535,7 +1536,7 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
 	tty_lock(tty);
 	retval = tty_driver_install_tty(driver, tty);
 	if (retval < 0)
-		goto err_deinit_tty;
+		goto err_free_tty;
 
 	if (!tty->port)
 		tty->port = driver->ports[idx];
@@ -1557,9 +1558,8 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
 	/* Return the tty locked so that it cannot vanish under the caller */
 	return tty;
 
-err_deinit_tty:
+err_free_tty:
 	tty_unlock(tty);
-	deinitialize_tty_struct(tty);
 	free_tty_struct(tty);
 err_module_put:
 	module_put(driver->owner);
@@ -3169,20 +3169,6 @@ struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx)
 }
 
 /**
- *	deinitialize_tty_struct
- *	@tty: tty to deinitialize
- *
- *	This subroutine deinitializes a tty structure that has been newly
- *	allocated but tty_release cannot be called on that yet.
- *
- *	Locking: none - tty in question must not be exposed at this point
- */
-void deinitialize_tty_struct(struct tty_struct *tty)
-{
-	tty_ldisc_deinit(tty);
-}
-
-/**
  *	tty_put_char	-	write one character to a tty
  *	@tty: tty
  *	@ch: character
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 3455908..9b3c11a 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -830,7 +830,7 @@ void tty_ldisc_init(struct tty_struct *tty)
 }
 
 /**
- *	tty_ldisc_init		-	ldisc cleanup for new tty
+ *	tty_ldisc_deinit	-	ldisc cleanup for new tty
  *	@tty: tty that was allocated recently
  *
  *	The tty structure must not becompletely set up (tty_ldisc_setup) when
@@ -838,7 +838,8 @@ void tty_ldisc_init(struct tty_struct *tty)
  */
 void tty_ldisc_deinit(struct tty_struct *tty)
 {
-	tty_ldisc_put(tty->ldisc);
+	if (tty->ldisc)
+		tty_ldisc_put(tty->ldisc);
 	tty->ldisc = NULL;
 }
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 8c8050d..9656c5d 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -510,7 +510,6 @@ extern int tty_alloc_file(struct file *file);
 extern void tty_add_file(struct tty_struct *tty, struct file *file);
 extern void tty_free_file(struct file *file);
 extern void free_tty_struct(struct tty_struct *tty);
-extern void deinitialize_tty_struct(struct tty_struct *tty);
 extern struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx);
 extern int tty_release(struct inode *inode, struct file *filp);
 extern int tty_init_termios(struct tty_struct *tty);
-- 
2.6.3

[PATCH 02/12] tty: Remove !tty check from free_tty_struct()

[Peter Hurley]

free_tty_struct() is never called with NULL tty; the two call sites
would already have faulted on earlier access.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 153b7b8..76e4ae0 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -171,8 +171,6 @@ static void release_tty(struct tty_struct *tty, int idx);
 
 void free_tty_struct(struct tty_struct *tty)
 {
-	if (!tty)
-		return;
 	tty_ldisc_deinit(tty);
 	put_device(tty->dev);
 	kfree(tty->write_buf);
-- 
2.6.3

[PATCH 03/12] tty: Fix tty_init_termios() declaration

[Peter Hurley]

tty_init_termios() never returns an error; re-declare as void. Remove
unnecessary error handling from callers. Remove extern declarations
of tty_free_termios() and free_tty_struct() and re-declare in file
scope.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c            | 15 +++------------
 drivers/tty/tty_io.c         | 13 ++++---------
 drivers/usb/serial/console.c |  6 +-----
 include/linux/tty.h          |  4 +---
 4 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 8cbe802..7e885a2 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -406,13 +406,8 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	if (legacy) {
 		/* We always use new tty termios data so we can do this
 		   the easy way .. */
-		retval = tty_init_termios(tty);
-		if (retval)
-			goto err_free_tty;
-
-		retval = tty_init_termios(o_tty);
-		if (retval)
-			goto err_free_termios;
+		tty_init_termios(tty);
+		tty_init_termios(o_tty);
 
 		driver->other->ttys[idx] = o_tty;
 		driver->ttys[idx] = tty;
@@ -444,11 +439,7 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	tty->count++;
 	o_tty->count++;
 	return 0;
-err_free_termios:
-	if (legacy)
-		tty_free_termios(tty);
-err_free_tty:
-	free_tty_struct(o_tty);
+
 err_put_module:
 	module_put(driver->other->owner);
 err:
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 76e4ae0..fe2ed1a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -169,7 +169,7 @@ static void release_tty(struct tty_struct *tty, int idx);
  *	Locking: none. Must be called after tty is definitely unused
  */
 
-void free_tty_struct(struct tty_struct *tty)
+static void free_tty_struct(struct tty_struct *tty)
 {
 	tty_ldisc_deinit(tty);
 	put_device(tty->dev);
@@ -1381,7 +1381,7 @@ static struct tty_struct *tty_driver_lookup_tty(struct tty_driver *driver,
  *	the tty_mutex currently so we can be relaxed about ordering.
  */
 
-int tty_init_termios(struct tty_struct *tty)
+void tty_init_termios(struct tty_struct *tty)
 {
 	struct ktermios *tp;
 	int idx = tty->index;
@@ -1400,16 +1400,12 @@ int tty_init_termios(struct tty_struct *tty)
 	/* Compatibility until drivers always set this */
 	tty->termios.c_ispeed = tty_termios_input_baud_rate(&tty->termios);
 	tty->termios.c_ospeed = tty_termios_baud_rate(&tty->termios);
-	return 0;
 }
 EXPORT_SYMBOL_GPL(tty_init_termios);
 
 int tty_standard_install(struct tty_driver *driver, struct tty_struct *tty)
 {
-	int ret = tty_init_termios(tty);
-	if (ret)
-		return ret;
-
+	tty_init_termios(tty);
 	tty_driver_kref_get(driver);
 	tty->count++;
 	driver->ttys[tty->index] = tty;
@@ -1572,7 +1568,7 @@ err_release_tty:
 	return ERR_PTR(retval);
 }
 
-void tty_free_termios(struct tty_struct *tty)
+static void tty_free_termios(struct tty_struct *tty)
 {
 	struct ktermios *tp;
 	int idx = tty->index;
@@ -1591,7 +1587,6 @@ void tty_free_termios(struct tty_struct *tty)
 	}
 	*tp = tty->termios;
 }
-EXPORT_SYMBOL(tty_free_termios);
 
 /**
  *	tty_flush_works		-	flush all works of a tty/pty pair
diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index 3806e70..a66b01b 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -147,10 +147,7 @@ static int usb_console_setup(struct console *co, char *options)
 			kref_get(&tty->driver->kref);
 			__module_get(tty->driver->owner);
 			tty->ops = &usb_console_fake_tty_ops;
-			if (tty_init_termios(tty)) {
-				retval = -ENOMEM;
-				goto put_tty;
-			}
+			tty_init_termios(tty);
 			tty_port_tty_set(&port->port, tty);
 		}
 
@@ -185,7 +182,6 @@ static int usb_console_setup(struct console *co, char *options)
 
  fail:
 	tty_port_tty_set(&port->port, NULL);
- put_tty:
 	tty_kref_put(tty);
  reset_open_count:
 	port->port.count = 0;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9656c5d..301d0e9 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -447,7 +447,6 @@ extern int tty_unthrottle_safe(struct tty_struct *tty);
 extern int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
 extern void tty_driver_remove_tty(struct tty_driver *driver,
 				  struct tty_struct *tty);
-extern void tty_free_termios(struct tty_struct *tty);
 extern int is_current_pgrp_orphaned(void);
 extern int is_ignored(int sig);
 extern int tty_signal(int sig, struct tty_struct *tty);
@@ -509,10 +508,9 @@ extern struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx);
 extern int tty_alloc_file(struct file *file);
 extern void tty_add_file(struct tty_struct *tty, struct file *file);
 extern void tty_free_file(struct file *file);
-extern void free_tty_struct(struct tty_struct *tty);
 extern struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx);
 extern int tty_release(struct inode *inode, struct file *filp);
-extern int tty_init_termios(struct tty_struct *tty);
+extern void tty_init_termios(struct tty_struct *tty);
 extern int tty_standard_install(struct tty_driver *driver,
 		struct tty_struct *tty);
 
-- 
2.6.3

[PATCH 04/12] tty: Re-declare tty_driver_remove_tty() file scope

[Peter Hurley]

tty_driver_remove_tty() is only local-scope; declare as static.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index fe2ed1a..64978ca 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1442,7 +1442,7 @@ static int tty_driver_install_tty(struct tty_driver *driver,
  *
  *	Locking: tty_mutex for now
  */
-void tty_driver_remove_tty(struct tty_driver *driver, struct tty_struct *tty)
+static void tty_driver_remove_tty(struct tty_driver *driver, struct tty_struct *tty)
 {
 	if (driver->ops->remove)
 		driver->ops->remove(driver, tty);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 301d0e9..f8915f0 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -445,8 +445,6 @@ extern void tty_unthrottle(struct tty_struct *tty);
 extern int tty_throttle_safe(struct tty_struct *tty);
 extern int tty_unthrottle_safe(struct tty_struct *tty);
 extern int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
-extern void tty_driver_remove_tty(struct tty_driver *driver,
-				  struct tty_struct *tty);
 extern int is_current_pgrp_orphaned(void);
 extern int is_ignored(int sig);
 extern int tty_signal(int sig, struct tty_struct *tty);
-- 
2.6.3

[PATCH 05/12] pty: Remove pty_unix98_shutdown()

[Peter Hurley]

The tty core invokes the optional driver shutdown() just before
the optional driver remove() (shutdown() has access to the termios
and remove() does not). Because pty drivers must prevent the default
remove() action, the Unix98 pty drivers define a dummy remove() function.

Instead, release the slave index in the remove() method and delete the
optional shutdown() method.

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

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 7e885a2..be5020d 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -656,20 +656,13 @@ static struct tty_struct *pts_unix98_lookup(struct tty_driver *driver,
 	return tty;
 }
 
-/* We have no need to install and remove our tty objects as devpts does all
-   the work for us */
-
 static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
 {
 	return pty_common_install(driver, tty, false);
 }
 
-static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
-{
-}
-
 /* this is called once with whichever end is closed last */
-static void pty_unix98_shutdown(struct tty_struct *tty)
+static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
 {
 	devpts_kill_index(tty->driver_data, tty->index);
 }
@@ -687,7 +680,6 @@ static const struct tty_operations ptm_unix98_ops = {
 	.unthrottle = pty_unthrottle,
 	.ioctl = pty_unix98_ioctl,
 	.resize = pty_resize,
-	.shutdown = pty_unix98_shutdown,
 	.cleanup = pty_cleanup
 };
 
@@ -705,7 +697,6 @@ static const struct tty_operations pty_unix98_ops = {
 	.set_termios = pty_set_termios,
 	.start = pty_start,
 	.stop = pty_stop,
-	.shutdown = pty_unix98_shutdown,
 	.cleanup = pty_cleanup,
 };
 
-- 
2.6.3

[PATCH 06/12] tty: Remove __lockfunc annotation from tty lock functions

[Peter Hurley]

The tty lock/unlock code does not belong in the special lockfunc section
which is treated specially by stack backtraces.

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

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 9b3c11a..8a960fd 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -314,13 +314,13 @@ void tty_ldisc_deref(struct tty_ldisc *ld)
 EXPORT_SYMBOL_GPL(tty_ldisc_deref);
 
 
-static inline int __lockfunc
+static inline int
 __tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
 {
 	return ldsem_down_write(&tty->ldisc_sem, timeout);
 }
 
-static inline int __lockfunc
+static inline int
 __tty_ldisc_lock_nested(struct tty_struct *tty, unsigned long timeout)
 {
 	return ldsem_down_write_nested(&tty->ldisc_sem,
@@ -332,8 +332,7 @@ static inline void __tty_ldisc_unlock(struct tty_struct *tty)
 	ldsem_up_write(&tty->ldisc_sem);
 }
 
-static int __lockfunc
-tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
+static int tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
 {
 	int ret;
 
@@ -350,7 +349,7 @@ static void tty_ldisc_unlock(struct tty_struct *tty)
 	__tty_ldisc_unlock(tty);
 }
 
-static int __lockfunc
+static int
 tty_ldisc_lock_pair_timeout(struct tty_struct *tty, struct tty_struct *tty2,
 			    unsigned long timeout)
 {
@@ -386,14 +385,13 @@ tty_ldisc_lock_pair_timeout(struct tty_struct *tty, struct tty_struct *tty2,
 	return 0;
 }
 
-static void __lockfunc
-tty_ldisc_lock_pair(struct tty_struct *tty, struct tty_struct *tty2)
+static void tty_ldisc_lock_pair(struct tty_struct *tty, struct tty_struct *tty2)
 {
 	tty_ldisc_lock_pair_timeout(tty, tty2, MAX_SCHEDULE_TIMEOUT);
 }
 
-static void __lockfunc tty_ldisc_unlock_pair(struct tty_struct *tty,
-					     struct tty_struct *tty2)
+static void tty_ldisc_unlock_pair(struct tty_struct *tty,
+				  struct tty_struct *tty2)
 {
 	__tty_ldisc_unlock(tty);
 	if (tty2)
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 77703a3..79dac6f 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -10,7 +10,7 @@
  * Getting the big tty mutex.
  */
 
-void __lockfunc tty_lock(struct tty_struct *tty)
+void tty_lock(struct tty_struct *tty)
 {
 	if (WARN(tty->magic != TTY_MAGIC, "L Bad %p\n", tty))
 		return;
@@ -19,7 +19,7 @@ void __lockfunc tty_lock(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(tty_lock);
 
-void __lockfunc tty_unlock(struct tty_struct *tty)
+void tty_unlock(struct tty_struct *tty)
 {
 	if (WARN(tty->magic != TTY_MAGIC, "U Bad %p\n", tty))
 		return;
@@ -28,13 +28,13 @@ void __lockfunc tty_unlock(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(tty_unlock);
 
-void __lockfunc tty_lock_slave(struct tty_struct *tty)
+void tty_lock_slave(struct tty_struct *tty)
 {
 	if (tty && tty != tty->link)
 		tty_lock(tty);
 }
 
-void __lockfunc tty_unlock_slave(struct tty_struct *tty)
+void tty_unlock_slave(struct tty_struct *tty)
 {
 	if (tty && tty != tty->link)
 		tty_unlock(tty);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index f8915f0..c3ea90a 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -644,10 +644,10 @@ extern long vt_compat_ioctl(struct tty_struct *tty,
 
 /* tty_mutex.c */
 /* 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_slave(struct tty_struct *tty);
-extern void __lockfunc tty_unlock_slave(struct tty_struct *tty);
+extern void tty_lock(struct tty_struct *tty);
+extern void tty_unlock(struct tty_struct *tty);
+extern void tty_lock_slave(struct tty_struct *tty);
+extern void tty_unlock_slave(struct tty_struct *tty);
 extern void tty_set_lock_subclass(struct tty_struct *tty);
 
 #ifdef CONFIG_PROC_FS
-- 
2.6.3

[PATCH 07/12] tty: Wait interruptibly for tty lock on reopen

[Peter Hurley]

Allow a signal to interrupt the wait for a tty reopen; eg., if
the tty has starting final close and is waiting for the device to
drain.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c    | 8 +++++++-
 drivers/tty/tty_mutex.c | 8 ++++++++
 include/linux/tty.h     | 1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 64978ca..afd378e 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2064,7 +2064,12 @@ retry_open:
 
 		if (tty) {
 			mutex_unlock(&tty_mutex);
-			tty_lock(tty);
+			retval = tty_lock_interruptible(tty);
+			if (retval) {
+				if (retval == -EINTR)
+					retval = -ERESTARTSYS;
+				goto err_unref;
+			}
 			/* safe to drop the kref from tty_driver_lookup_tty() */
 			tty_kref_put(tty);
 			retval = tty_reopen(tty);
@@ -2151,6 +2156,7 @@ retry_open:
 	return 0;
 err_unlock:
 	mutex_unlock(&tty_mutex);
+err_unref:
 	/* after locks to avoid deadlock */
 	if (!IS_ERR_OR_NULL(driver))
 		tty_driver_kref_put(driver);
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 79dac6f..75351e4 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -19,6 +19,14 @@ void tty_lock(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(tty_lock);
 
+int tty_lock_interruptible(struct tty_struct *tty)
+{
+	if (WARN(tty->magic != TTY_MAGIC, "L Bad %p\n", tty))
+		return -EIO;
+	tty_kref_get(tty);
+	return mutex_lock_interruptible(&tty->legacy_mutex);
+}
+
 void tty_unlock(struct tty_struct *tty)
 {
 	if (WARN(tty->magic != TTY_MAGIC, "U Bad %p\n", tty))
diff --git a/include/linux/tty.h b/include/linux/tty.h
index c3ea90a..c7c5d3c 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -645,6 +645,7 @@ extern long vt_compat_ioctl(struct tty_struct *tty,
 /* tty_mutex.c */
 /* functions for preparation of BKL removal */
 extern void tty_lock(struct tty_struct *tty);
+extern int  tty_lock_interruptible(struct tty_struct *tty);
 extern void tty_unlock(struct tty_struct *tty);
 extern void tty_lock_slave(struct tty_struct *tty);
 extern void tty_unlock_slave(struct tty_struct *tty);
-- 
2.6.3

[PATCH 08/12] pty: Prepare to redefine tty driver remove() interface

[Peter Hurley]

BSD pty drivers are cross-linked at driver initialization and linked pairs
must have the same tty index; use this information to simplify clearing
the driver tables.

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

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index be5020d..2680044 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -463,10 +463,8 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
 
 static void pty_remove(struct tty_driver *driver, struct tty_struct *tty)
 {
-	struct tty_struct *pair = tty->link;
 	driver->ttys[tty->index] = NULL;
-	if (pair)
-		pair->driver->ttys[pair->index] = NULL;
+	driver->other->ttys[tty->index] = NULL;
 }
 
 static int pty_bsd_ioctl(struct tty_struct *tty,
-- 
2.6.3

[PATCH 09/12] tty: Re-define tty driver remove() interface

[Peter Hurley]

The tty driver remove() method is used exclusively by the BSD and Unix98
pty drivers to release the tty index; the BSD drivers clear the tty tables
for the pty pair @index, while the Unix98 drivers free the devpts index.

Although the remove() method is intended as the symmetric operation to the
lookup() method (ie., the index obtained via the lookup() is released by
the remove() method). the actual interface isn't defined reflexively.

This causes cleanup problems for ptmx_open() if initialization fails in
tty_init_dev(): ptmx_open() releases the devpts index, but the index may
have been already been released by release_tty() during cleanup in
tty_init_dev() instead. This will cause mismatched devpts counts and
duplicate removal of the devpts index.

Re-define the driver remove() method as the symmetric operation to the
lookup() method. Propagate the inode parameter dependency from the
file operation to the tty driver method:

 tty_open()  -+
              +-> tty_init_dev() +-> release_tty() -> tty_driver_remove_tty()
 ptmx_open() -+                  |
                                 |
 tty_release() ------------------+

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

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 2680044..c71a1ab 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -461,10 +461,10 @@ static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
 	return pty_common_install(driver, tty, true);
 }
 
-static void pty_remove(struct tty_driver *driver, struct tty_struct *tty)
+static void pty_remove(struct tty_driver *driver, struct inode *inode, int idx)
 {
-	driver->ttys[tty->index] = NULL;
-	driver->other->ttys[tty->index] = NULL;
+	driver->ttys[idx] = NULL;
+	driver->other->ttys[idx] = NULL;
 }
 
 static int pty_bsd_ioctl(struct tty_struct *tty,
@@ -660,9 +660,9 @@ static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
 }
 
 /* this is called once with whichever end is closed last */
-static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
+static void pty_unix98_remove(struct tty_driver *driver, struct inode *inode, int idx)
 {
-	devpts_kill_index(tty->driver_data, tty->index);
+	devpts_kill_index(inode, idx);
 }
 
 static const struct tty_operations ptm_unix98_ops = {
@@ -738,7 +738,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 	mutex_unlock(&devpts_mutex);
 
 	mutex_lock(&tty_mutex);
-	tty = tty_init_dev(ptm_driver, index);
+	tty = tty_init_dev(ptm_driver, inode, index);
 
 	if (IS_ERR(tty)) {
 		retval = PTR_ERR(tty);
@@ -777,7 +777,6 @@ err_release:
 	return retval;
 out:
 	mutex_unlock(&tty_mutex);
-	devpts_kill_index(inode, index);
 err_file:
 	tty_free_file(filp);
 	return retval;
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index afd378e..2a6d40c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -158,7 +158,7 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
 #endif
 static int __tty_fasync(int fd, struct file *filp, int on);
 static int tty_fasync(int fd, struct file *filp, int on);
-static void release_tty(struct tty_struct *tty, int idx);
+static void release_tty(struct tty_struct *tty, struct inode *inode, int idx);
 
 /**
  *	free_tty_struct		-	free a disused tty
@@ -1435,6 +1435,7 @@ static int tty_driver_install_tty(struct tty_driver *driver,
 /**
  *	tty_driver_remove_tty() - remove a tty from the driver tables
  *	@driver: the driver for the tty
+ *	@inode:	 device file inode
  *	@idx:	 the minor number
  *
  *	Remvoe a tty object from the driver tables. The tty->index field
@@ -1442,12 +1443,13 @@ static int tty_driver_install_tty(struct tty_driver *driver,
  *
  *	Locking: tty_mutex for now
  */
-static void tty_driver_remove_tty(struct tty_driver *driver, struct tty_struct *tty)
+static void tty_driver_remove_tty(struct tty_driver *driver,
+				  struct inode *inode, int idx)
 {
 	if (driver->ops->remove)
-		driver->ops->remove(driver, tty);
+		driver->ops->remove(driver, inode, idx);
 	else
-		driver->ttys[tty->index] = NULL;
+		driver->ttys[idx] = NULL;
 }
 
 /*
@@ -1505,7 +1507,8 @@ static int tty_reopen(struct tty_struct *tty)
  * relaxed for the (most common) case of reopening a tty.
  */
 
-struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
+struct tty_struct *tty_init_dev(struct tty_driver *driver,
+				struct inode *inode, int idx)
 {
 	struct tty_struct *tty;
 	int retval;
@@ -1557,6 +1560,7 @@ err_free_tty:
 	free_tty_struct(tty);
 err_module_put:
 	module_put(driver->owner);
+	tty_driver_remove_tty(driver, inode, idx);
 	return ERR_PTR(retval);
 
 	/* call the tty release_tty routine to clean out this slot */
@@ -1564,7 +1568,7 @@ err_release_tty:
 	tty_unlock(tty);
 	tty_info_ratelimited(tty, "ldisc open failed (%d), clearing slot %d\n",
 			     retval, idx);
-	release_tty(tty, idx);
+	release_tty(tty, inode, idx);
 	return ERR_PTR(retval);
 }
 
@@ -1679,7 +1683,7 @@ EXPORT_SYMBOL(tty_kref_put);
  *	of ttys that the driver keeps.
  *
  */
-static void release_tty(struct tty_struct *tty, int idx)
+static void release_tty(struct tty_struct *tty, struct inode *inode, int idx)
 {
 	/* This should always be true but check for the moment */
 	WARN_ON(tty->index != idx);
@@ -1687,7 +1691,7 @@ static void release_tty(struct tty_struct *tty, int idx)
 	if (tty->ops->shutdown)
 		tty->ops->shutdown(tty);
 	tty_free_termios(tty);
-	tty_driver_remove_tty(tty->driver, tty);
+	tty_driver_remove_tty(tty->driver, inode, idx);
 	tty->port->itty = NULL;
 	if (tty->link)
 		tty->link->port->itty = NULL;
@@ -1910,7 +1914,7 @@ int tty_release(struct inode *inode, struct file *filp)
 	 * unlock never unlocks a freed tty).
 	 */
 	mutex_lock(&tty_mutex);
-	release_tty(tty, idx);
+	release_tty(tty, inode, idx);
 	mutex_unlock(&tty_mutex);
 
 	return 0;
@@ -2078,7 +2082,7 @@ retry_open:
 				tty = ERR_PTR(retval);
 			}
 		} else { /* Returns with the tty_lock held for now */
-			tty = tty_init_dev(driver, index);
+			tty = tty_init_dev(driver, inode, index);
 			mutex_unlock(&tty_mutex);
 		}
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index c7c5d3c..94c3983 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -506,7 +506,8 @@ extern struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx);
 extern int tty_alloc_file(struct file *file);
 extern void tty_add_file(struct tty_struct *tty, struct file *file);
 extern void tty_free_file(struct file *file);
-extern struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx);
+extern struct tty_struct *tty_init_dev(struct tty_driver *driver,
+				       struct inode *inode, int idx);
 extern int tty_release(struct inode *inode, struct file *filp);
 extern void tty_init_termios(struct tty_struct *tty);
 extern int tty_standard_install(struct tty_driver *driver,
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index 1610524..c7aa71c 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -22,7 +22,7 @@
  *
  *	Optional method. Default behaviour is to use the ttys array
  *
- * void (*remove)(struct tty_driver *self, struct tty_struct *tty)
+ * void (*remove)(struct tty_driver *self, struct inode *inode, int idx)
  *
  *	Remove a closed tty from the tty driver internal tables. Used in
  *	conjunction with lookup and remove methods.
@@ -252,7 +252,7 @@ struct tty_operations {
 	struct tty_struct * (*lookup)(struct tty_driver *driver,
 			struct inode *inode, int idx);
 	int  (*install)(struct tty_driver *driver, struct tty_struct *tty);
-	void (*remove)(struct tty_driver *driver, struct tty_struct *tty);
+	void (*remove)(struct tty_driver *driver, struct inode *inode, int idx);
 	int  (*open)(struct tty_struct * tty, struct file * filp);
 	void (*close)(struct tty_struct * tty, struct file * filp);
 	void (*shutdown)(struct tty_struct *tty);
-- 
2.6.3

[PATCH 10/12] tty: Consolidate noctty checks in tty_open()

[Peter Hurley]

Evaluate the conditions which prevent this tty being the controlling
terminal in one place, just before setting the controlling terminal.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 2a6d40c..1792a20 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1970,7 +1970,7 @@ static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
  *	Locking: tty_mutex protects get_tty_driver
  */
 static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
-		int *noctty, int *index)
+		int *index)
 {
 	struct tty_driver *driver;
 
@@ -1980,7 +1980,6 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
 		extern struct tty_driver *console_driver;
 		driver = tty_driver_kref_get(console_driver);
 		*index = fg_console;
-		*noctty = 1;
 		break;
 	}
 #endif
@@ -1991,7 +1990,6 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
 			if (driver) {
 				/* Don't let /dev/console block */
 				filp->f_flags |= O_NONBLOCK;
-				*noctty = 1;
 				break;
 			}
 		}
@@ -2046,14 +2044,13 @@ retry_open:
 	if (retval)
 		return -ENOMEM;
 
-	noctty = filp->f_flags & O_NOCTTY;
 	index  = -1;
 	retval = 0;
 
 	tty = tty_open_current_tty(device, filp);
 	if (!tty) {
 		mutex_lock(&tty_mutex);
-		driver = tty_lookup_driver(device, filp, &noctty, &index);
+		driver = tty_lookup_driver(device, filp, &index);
 		if (IS_ERR(driver)) {
 			retval = PTR_ERR(driver);
 			goto err_unlock;
@@ -2097,10 +2094,6 @@ retry_open:
 	tty_add_file(tty, filp);
 
 	check_tty_count(tty, __func__);
-	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
-	    tty->driver->subtype == PTY_TYPE_MASTER)
-		noctty = 1;
-
 	tty_debug_hangup(tty, "opening (count=%d)\n", tty->count);
 
 	if (tty->ops->open)
@@ -2133,6 +2126,12 @@ retry_open:
 
 	read_lock(&tasklist_lock);
 	spin_lock_irq(&current->sighand->siglock);
+	noctty = (filp->f_flags & O_NOCTTY) ||
+			device == MKDEV(TTY_MAJOR, 0) ||
+			device == MKDEV(TTYAUX_MAJOR, 1) ||
+			(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
+			 tty->driver->subtype == PTY_TYPE_MASTER);
+
 	if (!noctty &&
 	    current->signal->leader &&
 	    !current->signal->tty &&
-- 
2.6.3

[PATCH 11/12] tty: Refactor tty_open()

[Peter Hurley]

Extract the driver lookup and reopen-or-initialize logic into helper
function tty_open_by_driver(). No functional change.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 1792a20..eb391d4 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2004,6 +2004,54 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
 	return driver;
 }
 
+static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
+					     struct file *filp)
+{
+	struct tty_struct *tty;
+	struct tty_driver *driver = NULL;
+	int index = -1;
+	int retval;
+
+	mutex_lock(&tty_mutex);
+	driver = tty_lookup_driver(device, filp, &index);
+	if (IS_ERR(driver)) {
+		mutex_unlock(&tty_mutex);
+		return ERR_CAST(driver);
+	}
+
+	/* check whether we're reopening an existing tty */
+	tty = tty_driver_lookup_tty(driver, inode, index);
+	if (IS_ERR(tty)) {
+		mutex_unlock(&tty_mutex);
+		goto out;
+	}
+
+	if (tty) {
+		mutex_unlock(&tty_mutex);
+		retval = tty_lock_interruptible(tty);
+		if (retval) {
+			if (retval == -EINTR)
+				retval = -ERESTARTSYS;
+			tty = ERR_PTR(retval);
+			goto out;
+		}
+		/* 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 */
+		tty = tty_init_dev(driver, inode, index);
+		mutex_unlock(&tty_mutex);
+	}
+
+out:
+	tty_driver_kref_put(driver);
+	return tty;
+}
+
 /**
  *	tty_open		-	open a tty device
  *	@inode: inode of device file
@@ -2032,8 +2080,6 @@ static int tty_open(struct inode *inode, struct file *filp)
 {
 	struct tty_struct *tty;
 	int noctty, retval;
-	struct tty_driver *driver = NULL;
-	int index;
 	dev_t device = inode->i_rdev;
 	unsigned saved_flags = filp->f_flags;
 
@@ -2044,47 +2090,9 @@ retry_open:
 	if (retval)
 		return -ENOMEM;
 
-	index  = -1;
-	retval = 0;
-
 	tty = tty_open_current_tty(device, filp);
-	if (!tty) {
-		mutex_lock(&tty_mutex);
-		driver = tty_lookup_driver(device, filp, &index);
-		if (IS_ERR(driver)) {
-			retval = PTR_ERR(driver);
-			goto err_unlock;
-		}
-
-		/* check whether we're reopening an existing tty */
-		tty = tty_driver_lookup_tty(driver, inode, index);
-		if (IS_ERR(tty)) {
-			retval = PTR_ERR(tty);
-			goto err_unlock;
-		}
-
-		if (tty) {
-			mutex_unlock(&tty_mutex);
-			retval = tty_lock_interruptible(tty);
-			if (retval) {
-				if (retval == -EINTR)
-					retval = -ERESTARTSYS;
-				goto err_unref;
-			}
-			/* 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 */
-			tty = tty_init_dev(driver, inode, index);
-			mutex_unlock(&tty_mutex);
-		}
-
-		tty_driver_kref_put(driver);
-	}
+	if (!tty)
+		tty = tty_open_by_driver(device, inode, filp);
 
 	if (IS_ERR(tty)) {
 		retval = PTR_ERR(tty);
@@ -2157,12 +2165,6 @@ retry_open:
 	read_unlock(&tasklist_lock);
 	tty_unlock(tty);
 	return 0;
-err_unlock:
-	mutex_unlock(&tty_mutex);
-err_unref:
-	/* after locks to avoid deadlock */
-	if (!IS_ERR_OR_NULL(driver))
-		tty_driver_kref_put(driver);
 err_file:
 	tty_free_file(filp);
 	return retval;
-- 
2.6.3

[PATCH 12/12] tty: Retry failed reopen if tty teardown in-progress

[Peter Hurley]

A small window exists where a tty reopen will observe the tty
just prior to imminent teardown (tty->count == 0); in this case, open()
returns EIO to userspace.

Instead, retry the open after checking for signals and yielding;
this interruptible retry loop allows teardown to commence and initialize
a new tty on retry. Never retry the BSD master pty reopen; there is no
guarantee the pty pair teardown is imminent since the slave file
descriptors may remain open indefinitely.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index eb391d4..09e8d4f 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1465,13 +1465,13 @@ static int tty_reopen(struct tty_struct *tty)
 {
 	struct tty_driver *driver = tty->driver;
 
-	if (!tty->count)
-		return -EIO;
-
 	if (driver->type == TTY_DRIVER_TYPE_PTY &&
 	    driver->subtype == PTY_TYPE_MASTER)
 		return -EIO;
 
+	if (!tty->count)
+		return -EAGAIN;
+
 	if (test_bit(TTY_EXCLUSIVE, &tty->flags) && !capable(CAP_SYS_ADMIN))
 		return -EBUSY;
 
@@ -2095,8 +2095,13 @@ retry_open:
 		tty = tty_open_by_driver(device, inode, filp);
 
 	if (IS_ERR(tty)) {
+		tty_free_file(filp);
 		retval = PTR_ERR(tty);
-		goto err_file;
+		if (retval != -EAGAIN || signal_pending(current))
+			return retval;
+
+		schedule();
+		goto retry_open;
 	}
 
 	tty_add_file(tty, filp);
@@ -2165,9 +2170,6 @@ retry_open:
 	read_unlock(&tasklist_lock);
 	tty_unlock(tty);
 	return 0;
-err_file:
-	tty_free_file(filp);
-	return retval;
 }
 
 
-- 
2.6.3

Re: [PATCH 00/12] Rework tty_reopen()

[Peter Hurley]

Hi Greg,

This series has been reported to fix a regression with Redhat's kdump
systemd service redirecting to /dev/console, when /dev/console is a
serial port.

The redirection consistently fails with EIO since
"tty: Remove tty_wait_until_sent_from_close", which is new to 4.4-rc
Prior to that patch, redirection would only occasionally fail with EIO. :)

[  The systemd repeated hangup of /dev/console also seems to be the
[  trigger for the serial driver crashes on hangup as well, which is
[  fixed by the 19-patch "Fix driver crashes on hangup" series.
[  That problem goes back to 3.10, but has only been reported recently,
[  which leads me to believe recent changes in systemd /dev/console
[  handling is a contributing factor (which I'm checking right now)

Here are what I think are the options to resolve the regression:

#1. Respin this series w/o the tty-next dependencies
#2. Split this series into the minimum necessary to fix the regression
#3. Revert from 4.4-rc (in revert order)
      "tty: Remove wait_event_interruptible_tty()"
      "tty: r3964: Replace/remove bogus tty lock use"
      "tty: r3964: Use tty->read_wait waitqueue"
      "tty: Remove tty_port::close_wait"
      "usb: gadget: gserial: Privatize close_wait"
      "tty: Remove ASYNC_CLOSING check in open()/hangup() methods"
      "tty: Remove tty_wait_until_sent_from_close()"

Let me know how you'd like me to handle this.

Sorry,
Peter Hurley


On 11/27/2015 06:25 PM, Peter Hurley wrote:
> This patch series implements two important improvements to tty open()
> behavior: interruptible open() and automatic retry when tty teardown
> has already commenced.
> 
> Interruptible open() allows signals to cancel the open wait if stalled
> waiting for tty teardown to complete.
> 
> Automatic retry of tty open() when racing a tty teardown now makes tty
> open() fully POSIX compliant. For some time, the Linux kernel has
> returned EIO from open() under certain circumstances. This happens when
> tty_open() observes a valid tty from driver lookup but the tty is
> being released (in final close) and teardown is about to commence.
> 
> The observable userspace change is that userspace will no longer need
> to retry open() on EIO error.
> 
> This series also continues the ongoing effort to cleanup and reduce the
> kernel tty interface.
> 
> Lastly, this series lays important groundwork for implementing ptmx_open()
> in tty_open(), trivially with driver lookup (still a work-in-progress).
> 
> Regards,
> 
> Peter Hurley (12):
>   tty: Fix ldisc leak in failed tty_init_dev()
>   tty: Remove !tty check from free_tty_struct()
>   tty: Fix tty_init_termios() declaration
>   tty: Re-declare tty_driver_remove_tty() file scope
>   pty: Remove pty_unix98_shutdown()
>   tty: Remove __lockfunc annotation from tty lock functions
>   tty: Wait interruptibly for tty lock on reopen
>   pty: Prepare to redefine tty driver remove() interface
>   tty: Re-define tty driver remove() interface
>   tty: Consolidate noctty checks in tty_open()
>   tty: Refactor tty_open()
>   tty: Retry failed reopen if tty teardown in-progress
> 
>  drivers/tty/pty.c            |  40 +++-------
>  drivers/tty/tty_io.c         | 180 +++++++++++++++++++++----------------------
>  drivers/tty/tty_ldisc.c      |  21 +++--
>  drivers/tty/tty_mutex.c      |  16 +++-
>  drivers/usb/serial/console.c |   6 +-
>  include/linux/tty.h          |  19 ++---
>  include/linux/tty_driver.h   |   4 +-
>  7 files changed, 128 insertions(+), 158 deletions(-)
> 

Re: [PATCH 00/12] Rework tty_reopen()

[Pratyush Anand]

On 16/12/2015:07:43:11 AM, Peter Hurley wrote:
> #1. Respin this series w/o the tty-next dependencies
> #2. Split this series into the minimum necessary to fix the regression

As far as kdump issue is concerned, backporting only 12/12 to 4.4-RC is able to
resolve it.

~Pratyush

Re: [PATCH 00/12] Rework tty_reopen()

[Greg Kroah-Hartman]

On Wed, Dec 16, 2015 at 07:43:11AM -0800, Peter Hurley wrote:
> Hi Greg,
> 
> This series has been reported to fix a regression with Redhat's kdump
> systemd service redirecting to /dev/console, when /dev/console is a
> serial port.
> 
> The redirection consistently fails with EIO since
> "tty: Remove tty_wait_until_sent_from_close", which is new to 4.4-rc
> Prior to that patch, redirection would only occasionally fail with EIO. :)
> 
> [  The systemd repeated hangup of /dev/console also seems to be the
> [  trigger for the serial driver crashes on hangup as well, which is
> [  fixed by the 19-patch "Fix driver crashes on hangup" series.
> [  That problem goes back to 3.10, but has only been reported recently,
> [  which leads me to believe recent changes in systemd /dev/console
> [  handling is a contributing factor (which I'm checking right now)
> 
> Here are what I think are the options to resolve the regression:
> 
> #1. Respin this series w/o the tty-next dependencies
> #2. Split this series into the minimum necessary to fix the regression
> #3. Revert from 4.4-rc (in revert order)
>       "tty: Remove wait_event_interruptible_tty()"
>       "tty: r3964: Replace/remove bogus tty lock use"
>       "tty: r3964: Use tty->read_wait waitqueue"
>       "tty: Remove tty_port::close_wait"
>       "usb: gadget: gserial: Privatize close_wait"
>       "tty: Remove ASYNC_CLOSING check in open()/hangup() methods"
>       "tty: Remove tty_wait_until_sent_from_close()"
> 
> Let me know how you'd like me to handle this.

Sounds like a reasonable approach, send the patches on and let's see
what they look like.

thanks,

greg k-h

[PATCH v2 00/10] Rework tty_reopen()

[Peter Hurley]

Hi Greg,

I re-ordered the patches in this series because the first two patches
fix regressions in 4.4-rc. (I did not originally write the patches
to fix regressions; that's just how it turned out).

Obviously, it's too late for those to make 4.4 but at least this way
they're ready for 4.4.1 stable.

Also, Herton Krzesinski brought to my attention a long-resident problem
with releasing the devpts index when the last file reference is /dev/tty.
Since that impacted the tty driver remove() method changes, I just
removed those patches from this series.

original message follows:

This patch series implements two important improvements to tty open()
behavior: interruptible open() and automatic retry when tty teardown
has already commenced.

Interruptible open() allows signals to cancel the open wait if stalled
waiting for tty teardown to complete.

Automatic retry of tty open() when racing a tty teardown now makes tty
open() fully POSIX compliant. For some time, the Linux kernel has
returned EIO from open() under certain circumstances. This happens when
tty_open() observes a valid tty from driver lookup but the tty is
being released (in final close) and teardown is about to commence.

The observable userspace change is that userspace will no longer need
to retry open() on EIO error.

This series also continues the ongoing effort to cleanup and reduce the
kernel tty interface.

Regards,

Peter Hurley (10):
  tty: Wait interruptibly for tty lock on reopen
  tty: Retry failed reopen if tty teardown in-progress
  tty: Fix ldisc leak in failed tty_init_dev()
  tty: Remove !tty check from free_tty_struct()
  tty: Fix tty_init_termios() declaration
  tty: Re-declare tty_driver_remove_tty() file scope
  pty: Remove pty_unix98_shutdown()
  tty: Remove __lockfunc annotation from tty lock functions
  tty: Consolidate noctty checks in tty_open()
  tty: Refactor tty_open()

 drivers/tty/pty.c            |  27 +------
 drivers/tty/tty_io.c         | 174 ++++++++++++++++++++++---------------------
 drivers/tty/tty_ldisc.c      |  21 +++---
 drivers/tty/tty_mutex.c      |  16 +++-
 drivers/usb/serial/console.c |   6 +-
 include/linux/tty.h          |  16 ++--
 6 files changed, 121 insertions(+), 139 deletions(-)

-- 
2.7.0

[PATCH v2 01/10] tty: Wait interruptibly for tty lock on reopen

[Peter Hurley]

Allow a signal to interrupt the wait for a tty reopen; eg., if
the tty has starting final close and is waiting for the device to
drain.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c    | 8 +++++++-
 drivers/tty/tty_mutex.c | 8 ++++++++
 include/linux/tty.h     | 1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index eda8715..1bf67a2 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2071,7 +2071,12 @@ retry_open:
 
 		if (tty) {
 			mutex_unlock(&tty_mutex);
-			tty_lock(tty);
+			retval = tty_lock_interruptible(tty);
+			if (retval) {
+				if (retval == -EINTR)
+					retval = -ERESTARTSYS;
+				goto err_unref;
+			}
 			/* safe to drop the kref from tty_driver_lookup_tty() */
 			tty_kref_put(tty);
 			retval = tty_reopen(tty);
@@ -2158,6 +2163,7 @@ retry_open:
 	return 0;
 err_unlock:
 	mutex_unlock(&tty_mutex);
+err_unref:
 	/* after locks to avoid deadlock */
 	if (!IS_ERR_OR_NULL(driver))
 		tty_driver_kref_put(driver);
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 77703a3..d2f3c4c 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -19,6 +19,14 @@ void __lockfunc tty_lock(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(tty_lock);
 
+int tty_lock_interruptible(struct tty_struct *tty)
+{
+	if (WARN(tty->magic != TTY_MAGIC, "L Bad %p\n", tty))
+		return -EIO;
+	tty_kref_get(tty);
+	return mutex_lock_interruptible(&tty->legacy_mutex);
+}
+
 void __lockfunc tty_unlock(struct tty_struct *tty)
 {
 	if (WARN(tty->magic != TTY_MAGIC, "U Bad %p\n", tty))
diff --git a/include/linux/tty.h b/include/linux/tty.h
index b17f759..acc53ad 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -646,6 +646,7 @@ extern long vt_compat_ioctl(struct tty_struct *tty,
 /* tty_mutex.c */
 /* functions for preparation of BKL removal */
 extern void __lockfunc tty_lock(struct tty_struct *tty);
+extern int  tty_lock_interruptible(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);
-- 
2.7.0

[PATCH v2 02/10] tty: Retry failed reopen if tty teardown in-progress

[Peter Hurley]

A small window exists where a tty reopen will observe the tty
just prior to imminent teardown (tty->count == 0); in this case, open()
returns EIO to userspace.

Instead, retry the open after checking for signals and yielding;
this interruptible retry loop allows teardown to commence and initialize
a new tty on retry. Never retry the BSD master pty reopen; there is no
guarantee the pty pair teardown is imminent since the slave file
descriptors may remain open indefinitely.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 1bf67a2..93fe10d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1468,13 +1468,13 @@ static int tty_reopen(struct tty_struct *tty)
 {
 	struct tty_driver *driver = tty->driver;
 
-	if (!tty->count)
-		return -EIO;
-
 	if (driver->type == TTY_DRIVER_TYPE_PTY &&
 	    driver->subtype == PTY_TYPE_MASTER)
 		return -EIO;
 
+	if (!tty->count)
+		return -EAGAIN;
+
 	if (test_bit(TTY_EXCLUSIVE, &tty->flags) && !capable(CAP_SYS_ADMIN))
 		return -EBUSY;
 
@@ -2094,7 +2094,11 @@ retry_open:
 
 	if (IS_ERR(tty)) {
 		retval = PTR_ERR(tty);
-		goto err_file;
+		if (retval != -EAGAIN || signal_pending(current))
+			goto err_file;
+		tty_free_file(filp);
+		schedule();
+		goto retry_open;
 	}
 
 	tty_add_file(tty, filp);
-- 
2.7.0

[PATCH v2 03/10] tty: Fix ldisc leak in failed tty_init_dev()

[Peter Hurley]

release_tty() leaks the ldisc instance when called directly (rather
than when releasing the file descriptor from tty_release()).

Since tty_ldisc_release() clears tty->ldisc, releasing the ldisc
instance at tty teardown if tty->ldisc is non-null is not in danger
of double-releasing the ldisc.

Remove deinitialize_tty_struct() now that free_tty_struct() always
performs the tty_ldisc_deinit().

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

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index b311004..8cbe802 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -408,7 +408,7 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 		   the easy way .. */
 		retval = tty_init_termios(tty);
 		if (retval)
-			goto err_deinit_tty;
+			goto err_free_tty;
 
 		retval = tty_init_termios(o_tty);
 		if (retval)
@@ -447,8 +447,7 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 err_free_termios:
 	if (legacy)
 		tty_free_termios(tty);
-err_deinit_tty:
-	deinitialize_tty_struct(o_tty);
+err_free_tty:
 	free_tty_struct(o_tty);
 err_put_module:
 	module_put(driver->other->owner);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 93fe10d..e487076 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -173,6 +173,7 @@ void free_tty_struct(struct tty_struct *tty)
 {
 	if (!tty)
 		return;
+	tty_ldisc_deinit(tty);
 	put_device(tty->dev);
 	kfree(tty->write_buf);
 	tty->magic = 0xDEADDEAD;
@@ -1535,7 +1536,7 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
 	tty_lock(tty);
 	retval = tty_driver_install_tty(driver, tty);
 	if (retval < 0)
-		goto err_deinit_tty;
+		goto err_free_tty;
 
 	if (!tty->port)
 		tty->port = driver->ports[idx];
@@ -1557,9 +1558,8 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
 	/* Return the tty locked so that it cannot vanish under the caller */
 	return tty;
 
-err_deinit_tty:
+err_free_tty:
 	tty_unlock(tty);
-	deinitialize_tty_struct(tty);
 	free_tty_struct(tty);
 err_module_put:
 	module_put(driver->owner);
@@ -3179,20 +3179,6 @@ struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx)
 }
 
 /**
- *	deinitialize_tty_struct
- *	@tty: tty to deinitialize
- *
- *	This subroutine deinitializes a tty structure that has been newly
- *	allocated but tty_release cannot be called on that yet.
- *
- *	Locking: none - tty in question must not be exposed at this point
- */
-void deinitialize_tty_struct(struct tty_struct *tty)
-{
-	tty_ldisc_deinit(tty);
-}
-
-/**
  *	tty_put_char	-	write one character to a tty
  *	@tty: tty
  *	@ch: character
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index b404c20..92d2898 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -814,7 +814,7 @@ void tty_ldisc_init(struct tty_struct *tty)
 }
 
 /**
- *	tty_ldisc_init		-	ldisc cleanup for new tty
+ *	tty_ldisc_deinit	-	ldisc cleanup for new tty
  *	@tty: tty that was allocated recently
  *
  *	The tty structure must not becompletely set up (tty_ldisc_setup) when
@@ -822,7 +822,8 @@ void tty_ldisc_init(struct tty_struct *tty)
  */
 void tty_ldisc_deinit(struct tty_struct *tty)
 {
-	tty_ldisc_put(tty->ldisc);
+	if (tty->ldisc)
+		tty_ldisc_put(tty->ldisc);
 	tty->ldisc = NULL;
 }
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index acc53ad..9ce3dbc 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -510,7 +510,6 @@ extern int tty_alloc_file(struct file *file);
 extern void tty_add_file(struct tty_struct *tty, struct file *file);
 extern void tty_free_file(struct file *file);
 extern void free_tty_struct(struct tty_struct *tty);
-extern void deinitialize_tty_struct(struct tty_struct *tty);
 extern struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx);
 extern int tty_release(struct inode *inode, struct file *filp);
 extern int tty_init_termios(struct tty_struct *tty);
-- 
2.7.0

[PATCH v2 04/10] tty: Remove !tty check from free_tty_struct()

[Peter Hurley]

free_tty_struct() is never called with NULL tty; the two call sites
would already have faulted on earlier access.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e487076..17a3ffb 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -171,8 +171,6 @@ static void release_tty(struct tty_struct *tty, int idx);
 
 void free_tty_struct(struct tty_struct *tty)
 {
-	if (!tty)
-		return;
 	tty_ldisc_deinit(tty);
 	put_device(tty->dev);
 	kfree(tty->write_buf);
-- 
2.7.0

[PATCH v2 05/10] tty: Fix tty_init_termios() declaration

[Peter Hurley]

tty_init_termios() never returns an error; re-declare as void. Remove
unnecessary error handling from callers. Remove extern declarations
of tty_free_termios() and free_tty_struct() and re-declare in file
scope.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/pty.c            | 15 +++------------
 drivers/tty/tty_io.c         | 13 ++++---------
 drivers/usb/serial/console.c |  6 +-----
 include/linux/tty.h          |  4 +---
 4 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 8cbe802..7e885a2 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -406,13 +406,8 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	if (legacy) {
 		/* We always use new tty termios data so we can do this
 		   the easy way .. */
-		retval = tty_init_termios(tty);
-		if (retval)
-			goto err_free_tty;
-
-		retval = tty_init_termios(o_tty);
-		if (retval)
-			goto err_free_termios;
+		tty_init_termios(tty);
+		tty_init_termios(o_tty);
 
 		driver->other->ttys[idx] = o_tty;
 		driver->ttys[idx] = tty;
@@ -444,11 +439,7 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	tty->count++;
 	o_tty->count++;
 	return 0;
-err_free_termios:
-	if (legacy)
-		tty_free_termios(tty);
-err_free_tty:
-	free_tty_struct(o_tty);
+
 err_put_module:
 	module_put(driver->other->owner);
 err:
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 17a3ffb..0894293 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -169,7 +169,7 @@ static void release_tty(struct tty_struct *tty, int idx);
  *	Locking: none. Must be called after tty is definitely unused
  */
 
-void free_tty_struct(struct tty_struct *tty)
+static void free_tty_struct(struct tty_struct *tty)
 {
 	tty_ldisc_deinit(tty);
 	put_device(tty->dev);
@@ -1381,7 +1381,7 @@ static struct tty_struct *tty_driver_lookup_tty(struct tty_driver *driver,
  *	the tty_mutex currently so we can be relaxed about ordering.
  */
 
-int tty_init_termios(struct tty_struct *tty)
+void tty_init_termios(struct tty_struct *tty)
 {
 	struct ktermios *tp;
 	int idx = tty->index;
@@ -1400,16 +1400,12 @@ int tty_init_termios(struct tty_struct *tty)
 	/* Compatibility until drivers always set this */
 	tty->termios.c_ispeed = tty_termios_input_baud_rate(&tty->termios);
 	tty->termios.c_ospeed = tty_termios_baud_rate(&tty->termios);
-	return 0;
 }
 EXPORT_SYMBOL_GPL(tty_init_termios);
 
 int tty_standard_install(struct tty_driver *driver, struct tty_struct *tty)
 {
-	int ret = tty_init_termios(tty);
-	if (ret)
-		return ret;
-
+	tty_init_termios(tty);
 	tty_driver_kref_get(driver);
 	tty->count++;
 	driver->ttys[tty->index] = tty;
@@ -1572,7 +1568,7 @@ err_release_tty:
 	return ERR_PTR(retval);
 }
 
-void tty_free_termios(struct tty_struct *tty)
+static void tty_free_termios(struct tty_struct *tty)
 {
 	struct ktermios *tp;
 	int idx = tty->index;
@@ -1591,7 +1587,6 @@ void tty_free_termios(struct tty_struct *tty)
 	}
 	*tp = tty->termios;
 }
-EXPORT_SYMBOL(tty_free_termios);
 
 /**
  *	tty_flush_works		-	flush all works of a tty/pty pair
diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index 3806e70..a66b01b 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -147,10 +147,7 @@ static int usb_console_setup(struct console *co, char *options)
 			kref_get(&tty->driver->kref);
 			__module_get(tty->driver->owner);
 			tty->ops = &usb_console_fake_tty_ops;
-			if (tty_init_termios(tty)) {
-				retval = -ENOMEM;
-				goto put_tty;
-			}
+			tty_init_termios(tty);
 			tty_port_tty_set(&port->port, tty);
 		}
 
@@ -185,7 +182,6 @@ static int usb_console_setup(struct console *co, char *options)
 
  fail:
 	tty_port_tty_set(&port->port, NULL);
- put_tty:
 	tty_kref_put(tty);
  reset_open_count:
 	port->port.count = 0;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9ce3dbc..1b58ead 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -448,7 +448,6 @@ extern int tty_unthrottle_safe(struct tty_struct *tty);
 extern int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
 extern void tty_driver_remove_tty(struct tty_driver *driver,
 				  struct tty_struct *tty);
-extern void tty_free_termios(struct tty_struct *tty);
 extern int is_current_pgrp_orphaned(void);
 extern int is_ignored(int sig);
 extern int tty_signal(int sig, struct tty_struct *tty);
@@ -509,10 +508,9 @@ extern struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx);
 extern int tty_alloc_file(struct file *file);
 extern void tty_add_file(struct tty_struct *tty, struct file *file);
 extern void tty_free_file(struct file *file);
-extern void free_tty_struct(struct tty_struct *tty);
 extern struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx);
 extern int tty_release(struct inode *inode, struct file *filp);
-extern int tty_init_termios(struct tty_struct *tty);
+extern void tty_init_termios(struct tty_struct *tty);
 extern int tty_standard_install(struct tty_driver *driver,
 		struct tty_struct *tty);
 
-- 
2.7.0

Re: [PATCH v2 05/10] tty: Fix tty_init_termios() declaration

[Johan Hovold]

On Sat, Jan 09, 2016 at 09:13:48PM -0800, Peter Hurley wrote:
> tty_init_termios() never returns an error; re-declare as void. Remove
> unnecessary error handling from callers. Remove extern declarations
> of tty_free_termios() and free_tty_struct() and re-declare in file
> scope.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/pty.c            | 15 +++------------
>  drivers/tty/tty_io.c         | 13 ++++---------
>  drivers/usb/serial/console.c |  6 +-----

For the usb-serial bits

Acked-by: Johan Hovold <johan@kernel.org>

Thanks,
Johan

[PATCH v2 06/10] tty: Re-declare tty_driver_remove_tty() file scope

[Peter Hurley]

tty_driver_remove_tty() is only local-scope; declare as static.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 0894293..1f8d06f 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1442,7 +1442,7 @@ static int tty_driver_install_tty(struct tty_driver *driver,
  *
  *	Locking: tty_mutex for now
  */
-void tty_driver_remove_tty(struct tty_driver *driver, struct tty_struct *tty)
+static void tty_driver_remove_tty(struct tty_driver *driver, struct tty_struct *tty)
 {
 	if (driver->ops->remove)
 		driver->ops->remove(driver, tty);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1b58ead..7e1034f 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -446,8 +446,6 @@ extern void tty_unthrottle(struct tty_struct *tty);
 extern int tty_throttle_safe(struct tty_struct *tty);
 extern int tty_unthrottle_safe(struct tty_struct *tty);
 extern int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
-extern void tty_driver_remove_tty(struct tty_driver *driver,
-				  struct tty_struct *tty);
 extern int is_current_pgrp_orphaned(void);
 extern int is_ignored(int sig);
 extern int tty_signal(int sig, struct tty_struct *tty);
-- 
2.7.0

[PATCH v2 07/10] pty: Remove pty_unix98_shutdown()

[Peter Hurley]

The tty core invokes the optional driver shutdown() just before
the optional driver remove() (shutdown() has access to the termios
and remove() does not). Because pty drivers must prevent the default
remove() action, the Unix98 pty drivers define a dummy remove() function.

Instead, release the slave index in the remove() method and delete the
optional shutdown() method.

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

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 7e885a2..be5020d 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -656,20 +656,13 @@ static struct tty_struct *pts_unix98_lookup(struct tty_driver *driver,
 	return tty;
 }
 
-/* We have no need to install and remove our tty objects as devpts does all
-   the work for us */
-
 static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
 {
 	return pty_common_install(driver, tty, false);
 }
 
-static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
-{
-}
-
 /* this is called once with whichever end is closed last */
-static void pty_unix98_shutdown(struct tty_struct *tty)
+static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
 {
 	devpts_kill_index(tty->driver_data, tty->index);
 }
@@ -687,7 +680,6 @@ static const struct tty_operations ptm_unix98_ops = {
 	.unthrottle = pty_unthrottle,
 	.ioctl = pty_unix98_ioctl,
 	.resize = pty_resize,
-	.shutdown = pty_unix98_shutdown,
 	.cleanup = pty_cleanup
 };
 
@@ -705,7 +697,6 @@ static const struct tty_operations pty_unix98_ops = {
 	.set_termios = pty_set_termios,
 	.start = pty_start,
 	.stop = pty_stop,
-	.shutdown = pty_unix98_shutdown,
 	.cleanup = pty_cleanup,
 };
 
-- 
2.7.0

[PATCH v2 08/10] tty: Remove __lockfunc annotation from tty lock functions

[Peter Hurley]

The tty lock/unlock code does not belong in the special lockfunc section
which is treated specially by stack backtraces.

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

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 92d2898..4cb5e572 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -315,13 +315,13 @@ void tty_ldisc_deref(struct tty_ldisc *ld)
 EXPORT_SYMBOL_GPL(tty_ldisc_deref);
 
 
-static inline int __lockfunc
+static inline int
 __tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
 {
 	return ldsem_down_write(&tty->ldisc_sem, timeout);
 }
 
-static inline int __lockfunc
+static inline int
 __tty_ldisc_lock_nested(struct tty_struct *tty, unsigned long timeout)
 {
 	return ldsem_down_write_nested(&tty->ldisc_sem,
@@ -333,8 +333,7 @@ static inline void __tty_ldisc_unlock(struct tty_struct *tty)
 	ldsem_up_write(&tty->ldisc_sem);
 }
 
-static int __lockfunc
-tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
+static int tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
 {
 	int ret;
 
@@ -351,7 +350,7 @@ static void tty_ldisc_unlock(struct tty_struct *tty)
 	__tty_ldisc_unlock(tty);
 }
 
-static int __lockfunc
+static int
 tty_ldisc_lock_pair_timeout(struct tty_struct *tty, struct tty_struct *tty2,
 			    unsigned long timeout)
 {
@@ -387,14 +386,13 @@ tty_ldisc_lock_pair_timeout(struct tty_struct *tty, struct tty_struct *tty2,
 	return 0;
 }
 
-static void __lockfunc
-tty_ldisc_lock_pair(struct tty_struct *tty, struct tty_struct *tty2)
+static void tty_ldisc_lock_pair(struct tty_struct *tty, struct tty_struct *tty2)
 {
 	tty_ldisc_lock_pair_timeout(tty, tty2, MAX_SCHEDULE_TIMEOUT);
 }
 
-static void __lockfunc tty_ldisc_unlock_pair(struct tty_struct *tty,
-					     struct tty_struct *tty2)
+static void tty_ldisc_unlock_pair(struct tty_struct *tty,
+				  struct tty_struct *tty2)
 {
 	__tty_ldisc_unlock(tty);
 	if (tty2)
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index d2f3c4c..75351e4 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -10,7 +10,7 @@
  * Getting the big tty mutex.
  */
 
-void __lockfunc tty_lock(struct tty_struct *tty)
+void tty_lock(struct tty_struct *tty)
 {
 	if (WARN(tty->magic != TTY_MAGIC, "L Bad %p\n", tty))
 		return;
@@ -27,7 +27,7 @@ int tty_lock_interruptible(struct tty_struct *tty)
 	return mutex_lock_interruptible(&tty->legacy_mutex);
 }
 
-void __lockfunc tty_unlock(struct tty_struct *tty)
+void tty_unlock(struct tty_struct *tty)
 {
 	if (WARN(tty->magic != TTY_MAGIC, "U Bad %p\n", tty))
 		return;
@@ -36,13 +36,13 @@ void __lockfunc tty_unlock(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(tty_unlock);
 
-void __lockfunc tty_lock_slave(struct tty_struct *tty)
+void tty_lock_slave(struct tty_struct *tty)
 {
 	if (tty && tty != tty->link)
 		tty_lock(tty);
 }
 
-void __lockfunc tty_unlock_slave(struct tty_struct *tty)
+void tty_unlock_slave(struct tty_struct *tty)
 {
 	if (tty && tty != tty->link)
 		tty_unlock(tty);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 7e1034f..1b31736 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -640,11 +640,11 @@ extern long vt_compat_ioctl(struct tty_struct *tty,
 
 /* tty_mutex.c */
 /* functions for preparation of BKL removal */
-extern void __lockfunc tty_lock(struct tty_struct *tty);
+extern void tty_lock(struct tty_struct *tty);
 extern int  tty_lock_interruptible(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_unlock(struct tty_struct *tty);
+extern void tty_lock_slave(struct tty_struct *tty);
+extern void tty_unlock_slave(struct tty_struct *tty);
 extern void tty_set_lock_subclass(struct tty_struct *tty);
 
 #ifdef CONFIG_PROC_FS
-- 
2.7.0

[PATCH v2 09/10] tty: Consolidate noctty checks in tty_open()

[Peter Hurley]

Evaluate the conditions which prevent this tty being the controlling
terminal in one place, just before setting the controlling terminal.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 1f8d06f..d223e54 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1966,7 +1966,7 @@ static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
  *	Locking: tty_mutex protects get_tty_driver
  */
 static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
-		int *noctty, int *index)
+		int *index)
 {
 	struct tty_driver *driver;
 
@@ -1976,7 +1976,6 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
 		extern struct tty_driver *console_driver;
 		driver = tty_driver_kref_get(console_driver);
 		*index = fg_console;
-		*noctty = 1;
 		break;
 	}
 #endif
@@ -1987,7 +1986,6 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
 			if (driver) {
 				/* Don't let /dev/console block */
 				filp->f_flags |= O_NONBLOCK;
-				*noctty = 1;
 				break;
 			}
 		}
@@ -2042,14 +2040,13 @@ retry_open:
 	if (retval)
 		return -ENOMEM;
 
-	noctty = filp->f_flags & O_NOCTTY;
 	index  = -1;
 	retval = 0;
 
 	tty = tty_open_current_tty(device, filp);
 	if (!tty) {
 		mutex_lock(&tty_mutex);
-		driver = tty_lookup_driver(device, filp, &noctty, &index);
+		driver = tty_lookup_driver(device, filp, &index);
 		if (IS_ERR(driver)) {
 			retval = PTR_ERR(driver);
 			goto err_unlock;
@@ -2097,10 +2094,6 @@ retry_open:
 	tty_add_file(tty, filp);
 
 	check_tty_count(tty, __func__);
-	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
-	    tty->driver->subtype == PTY_TYPE_MASTER)
-		noctty = 1;
-
 	tty_debug_hangup(tty, "opening (count=%d)\n", tty->count);
 
 	if (tty->ops->open)
@@ -2133,6 +2126,12 @@ retry_open:
 
 	read_lock(&tasklist_lock);
 	spin_lock_irq(&current->sighand->siglock);
+	noctty = (filp->f_flags & O_NOCTTY) ||
+			device == MKDEV(TTY_MAJOR, 0) ||
+			device == MKDEV(TTYAUX_MAJOR, 1) ||
+			(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
+			 tty->driver->subtype == PTY_TYPE_MASTER);
+
 	if (!noctty &&
 	    current->signal->leader &&
 	    !current->signal->tty &&
-- 
2.7.0

Re: [PATCH v2 09/10] tty: Consolidate noctty checks in tty_open()

[Richard Weinberger]

On Sun, Jan 10, 2016 at 6:13 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> Evaluate the conditions which prevent this tty being the controlling
> terminal in one place, just before setting the controlling terminal.
>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/tty_io.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)

Peter,

This commit breaks existing userspace.
I noticed that on UserModeLinux with Debian Squeeze as userspace, getty does not
give me a controlling tty upon login.
It does not seem to happen on newer distros. But still this needs
further investigation.

Please me know what debug information you need.

-- 
Thanks,
//richard

Re: [PATCH v2 09/10] tty: Consolidate noctty checks in tty_open()

[Peter Hurley]

Hi Richard,

On 03/26/2016 10:58 AM, Richard Weinberger wrote:
> On Sun, Jan 10, 2016 at 6:13 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> Evaluate the conditions which prevent this tty being the controlling
>> terminal in one place, just before setting the controlling terminal.
>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>  drivers/tty/tty_io.c | 17 ++++++++---------
>>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> Peter,
> 
> This commit breaks existing userspace.
> I noticed that on UserModeLinux with Debian Squeeze as userspace, getty does not
> give me a controlling tty upon login.
> It does not seem to happen on newer distros. But still this needs
> further investigation.
> 
> Please me know what debug information you need.

Sorry about that; I hadn't considered the implications of UML console.
Can you test the blob below?

Regards,
Peter Hurley

--- >% ---
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 608beb6..a361c61 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2158,7 +2157,9 @@ retry_open:
 	read_lock(&tasklist_lock);
 	spin_lock_irq(&current->sighand->siglock);
 	noctty = (filp->f_flags & O_NOCTTY) ||
+#ifdef CONFIG_VT
 			device == MKDEV(TTY_MAJOR, 0) ||
+#endif
 			device == MKDEV(TTYAUX_MAJOR, 1) ||
 			(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
 			 tty->driver->subtype == PTY_TYPE_MASTER);

Re: [PATCH v2 09/10] tty: Consolidate noctty checks in tty_open()

[Richard Weinberger]

Peter,

Am 26.03.2016 um 20:06 schrieb Peter Hurley:
> Sorry about that; I hadn't considered the implications of UML console.
> Can you test the blob below?

Yep, works like a charm. :-)

Thanks,
//richard

Re: [uml-devel] [PATCH v2 09/10] tty: Consolidate noctty checks in tty_open()

[Richard Weinberger]

Peter,

Am 26.03.2016 um 20:06 schrieb Peter Hurley:
> Sorry about that; I hadn't considered the implications of UML console.
> Can you test the blob below?

Yep, works like a charm. :-)

Thanks,
//richard

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785351&iu=/4140
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

[PATCH v2 10/10] tty: Refactor tty_open()

[Peter Hurley]

Extract the driver lookup and reopen-or-initialize logic into helper
function tty_open_by_driver(). No functional change.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index d223e54..cef913c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2001,6 +2001,69 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
 }
 
 /**
+ *	tty_open_by_driver	-	open a tty device
+ *	@device: dev_t of device to open
+ *	@inode: inode of device file
+ *	@filp: file pointer to tty
+ *
+ *	Performs the driver lookup, checks for a reopen, or otherwise
+ *	performs the first-time tty initialization.
+ *
+ *	Returns the locked initialized or re-opened &tty_struct
+ *
+ *	Claims the global tty_mutex to serialize:
+ *	  - concurrent first-time tty initialization
+ *	  - concurrent tty driver removal w/ lookup
+ *	  - concurrent tty removal from driver table
+ */
+static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
+					     struct file *filp)
+{
+	struct tty_struct *tty;
+	struct tty_driver *driver = NULL;
+	int index = -1;
+	int retval;
+
+	mutex_lock(&tty_mutex);
+	driver = tty_lookup_driver(device, filp, &index);
+	if (IS_ERR(driver)) {
+		mutex_unlock(&tty_mutex);
+		return ERR_CAST(driver);
+	}
+
+	/* check whether we're reopening an existing tty */
+	tty = tty_driver_lookup_tty(driver, inode, index);
+	if (IS_ERR(tty)) {
+		mutex_unlock(&tty_mutex);
+		goto out;
+	}
+
+	if (tty) {
+		mutex_unlock(&tty_mutex);
+		retval = tty_lock_interruptible(tty);
+		if (retval) {
+			if (retval == -EINTR)
+				retval = -ERESTARTSYS;
+			tty = ERR_PTR(retval);
+			goto out;
+		}
+		/* 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 */
+		tty = tty_init_dev(driver, index);
+		mutex_unlock(&tty_mutex);
+	}
+out:
+	tty_driver_kref_put(driver);
+	return tty;
+}
+
+/**
  *	tty_open		-	open a tty device
  *	@inode: inode of device file
  *	@filp: file pointer to tty
@@ -2028,8 +2091,6 @@ static int tty_open(struct inode *inode, struct file *filp)
 {
 	struct tty_struct *tty;
 	int noctty, retval;
-	struct tty_driver *driver = NULL;
-	int index;
 	dev_t device = inode->i_rdev;
 	unsigned saved_flags = filp->f_flags;
 
@@ -2040,53 +2101,15 @@ retry_open:
 	if (retval)
 		return -ENOMEM;
 
-	index  = -1;
-	retval = 0;
-
 	tty = tty_open_current_tty(device, filp);
-	if (!tty) {
-		mutex_lock(&tty_mutex);
-		driver = tty_lookup_driver(device, filp, &index);
-		if (IS_ERR(driver)) {
-			retval = PTR_ERR(driver);
-			goto err_unlock;
-		}
-
-		/* check whether we're reopening an existing tty */
-		tty = tty_driver_lookup_tty(driver, inode, index);
-		if (IS_ERR(tty)) {
-			retval = PTR_ERR(tty);
-			goto err_unlock;
-		}
-
-		if (tty) {
-			mutex_unlock(&tty_mutex);
-			retval = tty_lock_interruptible(tty);
-			if (retval) {
-				if (retval == -EINTR)
-					retval = -ERESTARTSYS;
-				goto err_unref;
-			}
-			/* 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 */
-			tty = tty_init_dev(driver, index);
-			mutex_unlock(&tty_mutex);
-		}
-
-		tty_driver_kref_put(driver);
-	}
+	if (!tty)
+		tty = tty_open_by_driver(device, inode, filp);
 
 	if (IS_ERR(tty)) {
+		tty_free_file(filp);
 		retval = PTR_ERR(tty);
 		if (retval != -EAGAIN || signal_pending(current))
-			goto err_file;
-		tty_free_file(filp);
+			return retval;
 		schedule();
 		goto retry_open;
 	}
@@ -2157,15 +2180,6 @@ retry_open:
 	read_unlock(&tasklist_lock);
 	tty_unlock(tty);
 	return 0;
-err_unlock:
-	mutex_unlock(&tty_mutex);
-err_unref:
-	/* after locks to avoid deadlock */
-	if (!IS_ERR_OR_NULL(driver))
-		tty_driver_kref_put(driver);
-err_file:
-	tty_free_file(filp);
-	return retval;
 }
 
 
-- 
2.7.0