Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1279082 > unrolled thread

[PATCH 00/12] Rework tty_reopen()

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/12] Rework tty_reopen() Peter Hurley <peter@hurleysoftware.com> - 2015-11-28 03:30 +0100
    [PATCH 09/12] tty: Re-define tty driver remove() interface Peter Hurley <peter@hurleysoftware.com> - 2015-11-28 03:30 +0100
    [PATCH 06/12] tty: Remove __lockfunc annotation from tty lock functions Peter Hurley <peter@hurleysoftware.com> - 2015-11-28 03:30 +0100
    [PATCH 12/12] tty: Retry failed reopen if tty teardown in-progress Peter Hurley <peter@hurleysoftware.com> - 2015-11-28 03:30 +0100

#1279082 — [PATCH 00/12] Rework tty_reopen()

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web