Re: [PATCH] vt: kill tty->count usage

Re: [PATCH] vt: kill tty->count usage

[Alan Cox]

> each driver can keep the number of the references itself. Holding tty_mutex
> before calling driver's close() and updating tty->count is not possible
> because some drivers sleep waiting until the tx buffer is empty.

Agreed.

> This patch was tested under normal usage and using the stress application I
> wrote that triggers the problem more easily.
>
> +			if (vc->vc_tty != NULL) {
> +				/* we're still releasing this console entry */
> +				release_console_sem();
> +				return -EBUSY;

That isn't valid behaviour according to the POSIX tty rules. An open of a
closing tty blocks until the close completes. Most drivers using the
TTY_CLOSING flag to handle this.

So NAK the patch but basically agree with the approach - the EBUSY needs
to become a 'wait for close to complete, goto retry'

Alan

[PATCH] vt: kill tty->count usage

[Aristeu Rozanski]

The commit e0426e6a09954d205da2d674a3d368d2715e3afd fixes a real race but
still isn't enough to prevent these:

kobject_add_internal failed for vcs7 with -EEXIST, don't try to register
things with the same name in the same direc.
Pid: 2967, comm: vcs_stress Not tainted 2.6.25 #10
 [<c04e852e>] kobject_add_internal+0x137/0x149
 [<c04e85d4>] kobject_add_varg+0x35/0x41
 [<c04e8645>] kobject_add+0x43/0x49
 [<c055e54f>] device_add+0x91/0x4b4
 [<c055e984>] device_register+0x12/0x15
 [<c055e9f6>] device_create+0x6f/0x95
 [<c053e69b>] vcs_make_sysfs+0x23/0x4f
 [<c0543aff>] con_open+0x74/0x82
 [<c0538b64>] tty_open+0x188/0x287
 [<c047b1c8>] chrdev_open+0x119/0x135
 [<c04775d4>] __dentry_open+0xcf/0x185
 [<c0477711>] nameidata_to_filp+0x1f/0x33
 [<c047b0af>] ? chrdev_open+0x0/0x135
 [<c0477753>] do_filp_open+0x2e/0x35
 [<c0627da6>] ? _spin_unlock+0x1d/0x20
 [<c04774ef>] ? get_unused_fd_flags+0xc9/0xd3
 [<c047779a>] do_sys_open+0x40/0xb5
 [<c0477851>] sys_open+0x1e/0x26
 [<c0404962>] syscall_call+0x7/0xb
 =======================

and the reason for that is a race in release_dev() that can call driver's close
two or more at the same time. If these are the last two references, both calls
will be made before tty->count is decremented. And most tty drivers check
tty->count to identify the last close so the resources can be released. So
the close() method and the tty->count should happen without concurrency or
each driver can keep the number of the references itself. Holding tty_mutex
before calling driver's close() and updating tty->count is not possible
because some drivers sleep waiting until the tx buffer is empty.

This patch was tested under normal usage and using the stress application I
wrote that triggers the problem more easily.

Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>

---

 drivers/char/vt.c              |   82 +++++++++++++++++++++++++++++------------
 include/linux/console_struct.h |    2 +
 2 files changed, 60 insertions(+), 24 deletions(-)

--- a/drivers/char/vt.c	2008-08-07 16:06:55.000000000 -0400
+++ b/drivers/char/vt.c	2008-08-08 17:20:19.000000000 -0400
@@ -2730,15 +2730,24 @@ static int con_open(struct tty_struct *t
 {
 	unsigned int currcons = tty->index;
 	int ret = 0;
+	struct vc_data *vc;
 
 	acquire_console_sem();
 	if (tty->driver_data == NULL) {
 		ret = vc_allocate(currcons);
 		if (ret == 0) {
-			struct vc_data *vc = vc_cons[currcons].d;
+			vc = vc_cons[currcons].d;
+
+			if (vc->vc_tty != NULL) {
+				/* we're still releasing this console entry */
+				release_console_sem();
+				return -EBUSY;
+			}
+
 			tty->driver_data = vc;
 			vc->vc_tty = tty;
 
+			kref_init(&vc->kref);
 			if (!tty->winsize.ws_row && !tty->winsize.ws_col) {
 				tty->winsize.ws_row = vc_cons[currcons].d->vc_rows;
 				tty->winsize.ws_col = vc_cons[currcons].d->vc_cols;
@@ -2751,39 +2760,63 @@ static int con_open(struct tty_struct *t
 			release_console_sem();
 			return ret;
 		}
+	} else {
+		vc = tty->driver_data;
+		kref_get(&vc->kref);
 	}
 	release_console_sem();
 	return ret;
 }
 
-/*
- * We take tty_mutex in here to prevent another thread from coming in via init_dev
- * and taking a ref against the tty while we're in the process of forgetting
- * about it and cleaning things up.
- *
- * This is because vcs_remove_sysfs() can sleep and will drop the BKL.
- */
-static void con_close(struct tty_struct *tty, struct file *filp)
+static void con_release(struct kref *kref)
 {
-	mutex_lock(&tty_mutex);
+	struct vc_data *vc = container_of(kref, struct vc_data, kref);
+	struct tty_struct *tty = vc->vc_tty;
+
+	tty->driver_data = NULL;
+
+	/* we must release the semaphore here: vcs_remove_sysfs() may sleep */
+	release_console_sem();
+	vcs_remove_sysfs(tty);
 	acquire_console_sem();
-	if (tty && tty->count == 1) {
-		struct vc_data *vc = tty->driver_data;
 
-		if (vc)
-			vc->vc_tty = NULL;
-		tty->driver_data = NULL;
-		vcs_remove_sysfs(tty);
-		release_console_sem();
-		mutex_unlock(&tty_mutex);
-		/*
-		 * tty_mutex is released, but we still hold BKL, so there is
-		 * still exclusion against init_dev()
-		 */
+	/* now this slot is officially free */
+	vc->vc_tty = NULL;
+}
+
+static void con_close(struct tty_struct *tty, struct file *filp)
+{
+	struct vc_data *vc = tty->driver_data;
+
+	/*
+	 * if this tty was hung up, all the resources have been freed already
+	 */
+	if (tty_hung_up_p(filp))
 		return;
-	}
+
+	acquire_console_sem();
+	/*
+	 * tty->driver_data can be NULL if con_open() fails because tty layer
+	 * will call us even if the first open wasn't successful.
+	 */
+	if (vc != NULL)
+		kref_put(&vc->kref, con_release);
+	release_console_sem();
+}
+
+/*
+ * hangup was called: release all resources. all the currently open files will
+ * have the file_operations changed to hung_up_tty_fops. this means that the
+ * only choice the application has is close and open again.
+ */
+static void con_hangup(struct tty_struct *tty)
+{
+	struct vc_data *vc = tty->driver_data;
+
+	acquire_console_sem();
+	BUG_ON(vc == NULL);
+	con_release(&vc->kref);
 	release_console_sem();
-	mutex_unlock(&tty_mutex);
 }
 
 static int default_italic_color    = 2; // green (ASCII)
@@ -2907,6 +2940,7 @@ static const struct tty_operations con_o
 	.start = con_start,
 	.throttle = con_throttle,
 	.unthrottle = con_unthrottle,
+	.hangup = con_hangup,
 };
 
 int __init vty_init(void)
--- a/include/linux/console_struct.h	2008-05-23 15:53:23.000000000 -0400
+++ b/include/linux/console_struct.h	2008-08-08 11:55:49.000000000 -0400
@@ -15,6 +15,7 @@
 #include <linux/wait.h>
 #include <linux/vt.h>
 #include <linux/workqueue.h>
+#include <linux/kref.h>
 
 struct vt_struct;
 
@@ -108,6 +109,7 @@ struct vc_data {
 	unsigned long	vc_uni_pagedir;
 	unsigned long	*vc_uni_pagedir_loc;  /* [!] Location of uni_pagedir variable for this console */
 	/* additional information is in vt_kern.h */
+	struct kref	kref;
 };
 
 struct vc {

[PATCH] vt: kill tty->count usage (v2)

[Aristeu Rozanski]

The commit e0426e6a09954d205da2d674a3d368d2715e3afd fixes a real race but
still isn't enough to prevent these:

kobject_add_internal failed for vcs7 with -EEXIST, don't try to register
things with the same name in the same direc.
Pid: 2967, comm: vcs_stress Not tainted 2.6.25 #10
 [<c04e852e>] kobject_add_internal+0x137/0x149
 [<c04e85d4>] kobject_add_varg+0x35/0x41
 [<c04e8645>] kobject_add+0x43/0x49
 [<c055e54f>] device_add+0x91/0x4b4
 [<c055e984>] device_register+0x12/0x15
 [<c055e9f6>] device_create+0x6f/0x95
 [<c053e69b>] vcs_make_sysfs+0x23/0x4f
 [<c0543aff>] con_open+0x74/0x82
 [<c0538b64>] tty_open+0x188/0x287
 [<c047b1c8>] chrdev_open+0x119/0x135
 [<c04775d4>] __dentry_open+0xcf/0x185
 [<c0477711>] nameidata_to_filp+0x1f/0x33
 [<c047b0af>] ? chrdev_open+0x0/0x135
 [<c0477753>] do_filp_open+0x2e/0x35
 [<c0627da6>] ? _spin_unlock+0x1d/0x20
 [<c04774ef>] ? get_unused_fd_flags+0xc9/0xd3
 [<c047779a>] do_sys_open+0x40/0xb5
 [<c0477851>] sys_open+0x1e/0x26
 [<c0404962>] syscall_call+0x7/0xb
 =======================

and the reason for that is a race in release_dev() that can call driver's close
two or more at the same time. If these are the last two references, both calls
will be made before tty->count is decremented. And most tty drivers check
tty->count to identify the last close so the resources can be released. So
the close() method and the tty->count should happen without concurrency or
each driver can keep the number of the references itself. Holding tty_mutex
before calling driver's close() and updating tty->count is not possible
because some drivers sleep waiting until the tx buffer is empty.

This patch was tested under normal usage and using the stress application I
wrote that triggers the problem more easily.

Updates:
- now con_open() returns -ERESTARTSYS instead of -EBUSY to make tty_open()
  retry the open in case of vt being closed
- do not release console_semaphore on con_release() when calling
  vcs_remove_sysfs(), this would be a revert of
  e0426e6a09954d205da2d674a3d368d2715e3afd

Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>

---

 drivers/char/vt.c              |   79 ++++++++++++++++++++++++++++-------------
 include/linux/console_struct.h |    2 +
 2 files changed, 57 insertions(+), 24 deletions(-)

--- tty.orig/drivers/char/vt.c	2008-08-06 11:49:59.000000000 -0400
+++ tty/drivers/char/vt.c	2008-08-12 09:14:11.000000000 -0400
@@ -2732,15 +2732,24 @@ static int con_open(struct tty_struct *t
 {
 	unsigned int currcons = tty->index;
 	int ret = 0;
+	struct vc_data *vc;
 
 	acquire_console_sem();
 	if (tty->driver_data == NULL) {
 		ret = vc_allocate(currcons);
 		if (ret == 0) {
-			struct vc_data *vc = vc_cons[currcons].d;
+			vc = vc_cons[currcons].d;
+
+			if (vc->vc_tty != NULL) {
+				/* we're still releasing this console entry */
+				release_console_sem();
+				return -ERESTARTSYS;
+			}
+
 			tty->driver_data = vc;
 			vc->vc_tty = tty;
 
+			kref_init(&vc->kref);
 			if (!tty->winsize.ws_row && !tty->winsize.ws_col) {
 				tty->winsize.ws_row = vc_cons[currcons].d->vc_rows;
 				tty->winsize.ws_col = vc_cons[currcons].d->vc_cols;
@@ -2753,39 +2762,60 @@ static int con_open(struct tty_struct *t
 			release_console_sem();
 			return ret;
 		}
+	} else {
+		vc = tty->driver_data;
+		kref_get(&vc->kref);
 	}
 	release_console_sem();
 	return ret;
 }
 
-/*
- * We take tty_mutex in here to prevent another thread from coming in via init_dev
- * and taking a ref against the tty while we're in the process of forgetting
- * about it and cleaning things up.
- *
- * This is because vcs_remove_sysfs() can sleep and will drop the BKL.
- */
+static void con_release(struct kref *kref)
+{
+	struct vc_data *vc = container_of(kref, struct vc_data, kref);
+	struct tty_struct *tty = vc->vc_tty;
+
+	tty->driver_data = NULL;
+
+	vcs_remove_sysfs(tty);
+
+	/* now this slot is officially free */
+	vc->vc_tty = NULL;
+}
+
 static void con_close(struct tty_struct *tty, struct file *filp)
 {
-	mutex_lock(&tty_mutex);
-	acquire_console_sem();
-	if (tty && tty->count == 1) {
-		struct vc_data *vc = tty->driver_data;
+	struct vc_data *vc = tty->driver_data;
 
-		if (vc)
-			vc->vc_tty = NULL;
-		tty->driver_data = NULL;
-		vcs_remove_sysfs(tty);
-		release_console_sem();
-		mutex_unlock(&tty_mutex);
-		/*
-		 * tty_mutex is released, but we still hold BKL, so there is
-		 * still exclusion against init_dev()
-		 */
+	/*
+	 * if this tty was hung up, all the resources have been freed already
+	 */
+	if (tty_hung_up_p(filp))
 		return;
-	}
+
+	acquire_console_sem();
+	/*
+	 * tty->driver_data can be NULL if con_open() fails because tty layer
+	 * will call us even if the first open wasn't successful.
+	 */
+	if (vc != NULL)
+		kref_put(&vc->kref, con_release);
+	release_console_sem();
+}
+
+/*
+ * hangup was called: release all resources. all the currently open files will
+ * have the file_operations changed to hung_up_tty_fops. this means that the
+ * only choice the application has is close and open again.
+ */
+static void con_hangup(struct tty_struct *tty)
+{
+	struct vc_data *vc = tty->driver_data;
+
+	acquire_console_sem();
+	BUG_ON(vc == NULL);
+	con_release(&vc->kref);
 	release_console_sem();
-	mutex_unlock(&tty_mutex);
 }
 
 static int default_italic_color    = 2; // green (ASCII)
@@ -2909,6 +2939,7 @@ static const struct tty_operations con_o
 	.start = con_start,
 	.throttle = con_throttle,
 	.unthrottle = con_unthrottle,
+	.hangup = con_hangup,
 };
 
 int __init vty_init(void)
--- tty.orig/include/linux/console_struct.h	2008-08-12 09:14:18.000000000 -0400
+++ tty/include/linux/console_struct.h	2008-08-11 10:25:51.000000000 -0400
@@ -15,6 +15,7 @@
 #include <linux/wait.h>
 #include <linux/vt.h>
 #include <linux/workqueue.h>
+#include <linux/kref.h>
 
 struct vt_struct;
 
@@ -108,6 +109,7 @@ struct vc_data {
 	unsigned long	vc_uni_pagedir;
 	unsigned long	*vc_uni_pagedir_loc;  /* [!] Location of uni_pagedir variable for this console */
 	/* additional information is in vt_kern.h */
+	struct kref	kref;
 };
 
 struct vc {

Re: [PATCH] vt: kill tty->count usage (v2)

[Alan Cox]

On Tue, 12 Aug 2008 10:58:12 -0400
Aristeu Rozanski <aris@redhat.com> wrote:

> The commit e0426e6a09954d205da2d674a3d368d2715e3afd fixes a real race but
> still isn't enough to prevent these:
> 
> kobject_add_internal failed for vcs7 with -EEXIST, don't try to register
> things with the same name in the same direc.

Ack this. Will want some tweaking for the ttydev tree as it wants to
switch to using krefs for vc->vc_tty as well now.

Alan

Re: [PATCH] vt: kill tty->count usage (v2)

[Alan Cox]

On Tue, 12 Aug 2008 10:58:12 -0400
Aristeu Rozanski <aris@redhat.com> wrote:

> The commit e0426e6a09954d205da2d674a3d368d2715e3afd fixes a real race but
> still isn't enough to prevent these:
> 
> kobject_add_internal failed for vcs7 with -EEXIST, don't try to register
> things with the same name in the same direc.

Patch dropped due to testing failures.

Boot to run level 3, log in and type "reboot\n". Wait

Spews vt->driver_data == NULL warnings and oopses

Re: [PATCH] vt: kill tty->count usage (v2)

[Alan Cox]

On Mon, 18 Aug 2008 17:04:14 -0400
Aristeu Rozanski <aris@redhat.com> wrote:

> Hi Alan,
> > > The commit e0426e6a09954d205da2d674a3d368d2715e3afd fixes a real race but
> > > still isn't enough to prevent these:
> > > 
> > > kobject_add_internal failed for vcs7 with -EEXIST, don't try to register
> > > things with the same name in the same direc.
> > 
> > Patch dropped due to testing failures.
> > 
> > Boot to run level 3, log in and type "reboot\n". Wait
> > 
> > Spews vt->driver_data == NULL warnings and oopses
> do you mind posting the logs somewhere? I wasn't able to reproduce it here so
> far.

Tricky given I can currently only make it happen at reboot.

Could be an interaction with the tty refcounting and other changes I
guess but it seems to be tied to that patch at the moment. I'll
investigate further tomorrow.

Re: [PATCH] vt: kill tty->count usage (v2)

[Aristeu Rozanski]

Hi Alan,
> > The commit e0426e6a09954d205da2d674a3d368d2715e3afd fixes a real race but
> > still isn't enough to prevent these:
> > 
> > kobject_add_internal failed for vcs7 with -EEXIST, don't try to register
> > things with the same name in the same direc.
> 
> Patch dropped due to testing failures.
> 
> Boot to run level 3, log in and type "reboot\n". Wait
> 
> Spews vt->driver_data == NULL warnings and oopses
do you mind posting the logs somewhere? I wasn't able to reproduce it here so
far.

-- 
Aristeu

Re: [PATCH] vt: kill tty->count usage (v2)

[Alan Cox]

On Mon, 18 Aug 2008 17:04:14 -0400
Aristeu Rozanski <aris@redhat.com> wrote:

> Hi Alan,
> > > The commit e0426e6a09954d205da2d674a3d368d2715e3afd fixes a real race but
> > > still isn't enough to prevent these:
> > > 
> > > kobject_add_internal failed for vcs7 with -EEXIST, don't try to register
> > > things with the same name in the same direc.
> > 
> > Patch dropped due to testing failures.
> > 
> > Boot to run level 3, log in and type "reboot\n". Wait
> > 
> > Spews vt->driver_data == NULL warnings and oopses
> do you mind posting the logs somewhere? I wasn't able to reproduce it here so
> far.

I had a quick look and there are some obvious races from the change

You don't seem to have any locking between a hangup occuring at the same
time as say an ioctl. The f_ops change means no further calls will be
made to the console code but *not* that current ones won't continue.

Most of the time tty->driver_data is internally protected and checked by
the console_sem but not always. In particular your changes are showing up
corner cases in do_con_write and in the tty ioctl code.

Re: [PATCH] vt: kill tty->count usage (v2)

[Alan Cox]

On Mon, 18 Aug 2008 17:04:14 -0400
Aristeu Rozanski <aris@redhat.com> wrote:

> Hi Alan,
> > > The commit e0426e6a09954d205da2d674a3d368d2715e3afd fixes a real race but
> > > still isn't enough to prevent these:
> > > 
> > > kobject_add_internal failed for vcs7 with -EEXIST, don't try to register
> > > things with the same name in the same direc.
> > 
> > Patch dropped due to testing failures.
> > 
> > Boot to run level 3, log in and type "reboot\n". Wait
> > 
> > Spews vt->driver_data == NULL warnings and oopses
> do you mind posting the logs somewhere? I wasn't able to reproduce it here so

I decided it might be better to tackle this one 'head on' and go to the
root of the problem. I've pushed the following into the stack of patches
for -next

tty: shutdown method

From: Alan Cox <alan@redhat.com>

Right now there are various drivers that try to use tty->count to know when
they get the final close. Aristeau Rozanski showed while debugging the vt
sysfs race that this isn't entirely safe.

Instead of driver side tricks to work around this introduce a shutdown which
is called when the tty is being destructed. This also means that the shutdown
method is tied into the refcounting.

Use this to rework the console close/sysfs logic.

Remove lots of special case code from the tty core code. The pty code can now
have a shutdown() method that replaces the special case hackery in the tree
free up paths.

Signed-off-by: Alan Cox <alan@redhat.com>
---

 drivers/char/pty.c         |   29 ++++++++++++++++++++++----
 drivers/char/tty_io.c      |   49 ++++++++++++++++++++++++++------------------
 drivers/char/vt.c          |   34 +++++++++++++++----------------
 include/linux/tty.h        |    3 ++-
 include/linux/tty_driver.h |    6 +++++
 5 files changed, 79 insertions(+), 42 deletions(-)


diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index 76b2793..fbd215b 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -388,7 +388,27 @@ static int pty_unix98_ioctl(struct tty_struct *tty, struct file *file,
 	return -ENOIOCTLCMD;
 }
 
-static const struct tty_operations pty_unix98_ops = {
+static void pty_shutdown(struct tty_struct *tty)
+{
+	/* We have our own method as we don't use the tty index */
+	kfree(tty->termios);
+	kfree(tty->termios_locked);
+}
+
+static const struct tty_operations ptm_unix98_ops = {
+	.open = pty_open,
+	.close = pty_close,
+	.write = pty_write,
+	.write_room = pty_write_room,
+	.flush_buffer = pty_flush_buffer,
+	.chars_in_buffer = pty_chars_in_buffer,
+	.unthrottle = pty_unthrottle,
+	.set_termios = pty_set_termios,
+	.ioctl = pty_unix98_ioctl,
+	.shutdown = pty_shutdown
+};
+
+static const struct tty_operations pts_unix98_ops = {
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,
@@ -397,7 +417,8 @@ static const struct tty_operations pty_unix98_ops = {
 	.chars_in_buffer = pty_chars_in_buffer,
 	.unthrottle = pty_unthrottle,
 	.set_termios = pty_set_termios,
-	.ioctl = pty_unix98_ioctl
+	.ioctl = pty_bsd_ioctl,
+	.shutdown = pty_shutdown
 };
 
 
@@ -427,7 +448,7 @@ static void __init unix98_pty_init(void)
 	ptm_driver->flags = TTY_DRIVER_RESET_TERMIOS | TTY_DRIVER_REAL_RAW |
 		TTY_DRIVER_DYNAMIC_DEV | TTY_DRIVER_DEVPTS_MEM;
 	ptm_driver->other = pts_driver;
-	tty_set_operations(ptm_driver, &pty_unix98_ops);
+	tty_set_operations(ptm_driver, &ptm_unix98_ops);
 
 	pts_driver->owner = THIS_MODULE;
 	pts_driver->driver_name = "pty_slave";
@@ -443,7 +464,7 @@ static void __init unix98_pty_init(void)
 	pts_driver->flags = TTY_DRIVER_RESET_TERMIOS | TTY_DRIVER_REAL_RAW |
 		TTY_DRIVER_DYNAMIC_DEV | TTY_DRIVER_DEVPTS_MEM;
 	pts_driver->other = ptm_driver;
-	tty_set_operations(pts_driver, &pty_ops);
+	tty_set_operations(pts_driver, &pts_unix98_ops);
 	
 	if (tty_register_driver(ptm_driver))
 		panic("Couldn't register Unix98 ptm driver");
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 3aabf2e..8a70cbe 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1472,6 +1472,31 @@ release_mem_out:
 	goto end_init;
 }
 
+void tty_free_termios(struct tty_struct *tty)
+{
+	struct ktermios *tp;
+	int idx = tty->index;
+	/* Kill this flag and push into drivers for locking etc */
+	if (tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
+		/* FIXME: Locking on ->termios array */
+		tp = tty->termios;
+		tty->driver->termios[idx] = NULL;
+		kfree(tp);
+
+		tp = tty->termios_locked;
+		tty->driver->termios_locked[idx] = NULL;
+		kfree(tp);
+	}
+}
+EXPORT_SYMBOL(tty_free_termios);
+
+void tty_shutdown(struct tty_struct *tty)
+{
+	tty->driver->ttys[tty->index] = NULL;
+	tty_free_termios(tty);
+}
+EXPORT_SYMBOL(tty_shutdown);
+
 /**
  *	release_one_tty		-	release tty structure memory
  *	@kref: kref of tty we are obliterating
@@ -1489,27 +1514,11 @@ static void release_one_tty(struct kref *kref)
 {
 	struct tty_struct *tty = container_of(kref, struct tty_struct, kref);
 	struct tty_driver *driver = tty->driver;
-	int devpts = tty->driver->flags & TTY_DRIVER_DEVPTS_MEM;
-	struct ktermios *tp;
-	int idx = tty->index;
-
-	if (!devpts)
-		tty->driver->ttys[idx] = NULL;
-
-	if (tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
-		/* FIXME: Locking on ->termios array */
-		tp = tty->termios;
-		if (!devpts)
-			tty->driver->termios[idx] = NULL;
-		kfree(tp);
-
-		tp = tty->termios_locked;
-		if (!devpts)
-			tty->driver->termios_locked[idx] = NULL;
-		kfree(tp);
-	}
-
 
+	if (tty->ops->shutdown)
+		tty->ops->shutdown(tty);
+	else
+		tty_shutdown(tty);
 	tty->magic = 0;
 	/* FIXME: locking on tty->driver->refcount */
 	tty->driver->refcount--;
diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index ec94521..37a45db 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -2758,6 +2758,12 @@ static int con_open(struct tty_struct *tty, struct file *filp)
 		ret = vc_allocate(currcons);
 		if (ret == 0) {
 			struct vc_data *vc = vc_cons[currcons].d;
+
+			/* Still being freed */
+			if (vc->vc_tty) {
+				release_console_sem();
+				return -ERESTARTSYS;
+			}
 			tty->driver_data = vc;
 			vc->vc_tty = tty;
 
@@ -2787,25 +2793,18 @@ static int con_open(struct tty_struct *tty, struct file *filp)
  */
 static void con_close(struct tty_struct *tty, struct file *filp)
 {
-	mutex_lock(&tty_mutex);
-	acquire_console_sem();
-	if (tty && tty->count == 1) {
-		struct vc_data *vc = tty->driver_data;
+	/* Nothing to do - we defer to shutdown */
+}
 
-		if (vc)
-			vc->vc_tty = NULL;
-		tty->driver_data = NULL;
-		vcs_remove_sysfs(tty);
-		release_console_sem();
-		mutex_unlock(&tty_mutex);
-		/*
-		 * tty_mutex is released, but we still hold BKL, so there is
-		 * still exclusion against init_dev()
-		 */
-		return;
-	}
+static void con_shutdown(struct tty_struct *tty)
+{
+	struct vc_data *vc = tty->driver_data;
+	BUG_ON(vc == NULL);
+	acquire_console_sem();
+	vc->vc_tty = NULL;
+	vcs_remove_sysfs(tty);
 	release_console_sem();
-	mutex_unlock(&tty_mutex);
+	tty_shutdown(tty);
 }
 
 static int default_italic_color    = 2; // green (ASCII)
@@ -2930,6 +2929,7 @@ static const struct tty_operations con_ops = {
 	.throttle = con_throttle,
 	.unthrottle = con_unthrottle,
 	.resize = vt_resize,
+	.shutdown = con_shutdown
 };
 
 int __init vty_init(void)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 852484b..ee6d655 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -352,7 +352,8 @@ extern void tty_throttle(struct tty_struct *tty);
 extern void tty_unthrottle(struct tty_struct *tty);
 extern int tty_do_resize(struct tty_struct *tty, struct tty_struct *real_tty,
 						struct winsize *ws);
-
+extern void tty_shutdown(struct tty_struct *tty);
+extern void tty_free_termios(struct tty_struct *tty);
 extern int is_current_pgrp_orphaned(void);
 extern struct pid *tty_get_pgrp(struct tty_struct *tty);
 extern int is_ignored(int sig);
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index 16d2794..fe29dcd 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -21,6 +21,11 @@
  *
  *	Required method.
  *
+ * void (*shutdown)(struct tty_struct * tty);
+ *
+ * 	This routine is called when a particular tty device is closed for
+ *	the last time freeing up the resources.
+ *
  * int (*write)(struct tty_struct * tty,
  * 		 const unsigned char *buf, int count);
  *
@@ -192,6 +197,7 @@ struct tty_driver;
 struct tty_operations {
 	int  (*open)(struct tty_struct * tty, struct file * filp);
 	void (*close)(struct tty_struct * tty, struct file * filp);
+	void (*shutdown)(struct tty_struct *tty);
 	int  (*write)(struct tty_struct * tty,
 		      const unsigned char *buf, int count);
 	int  (*put_char)(struct tty_struct *tty, unsigned char ch);

Re: [PATCH] vt: kill tty->count usage (v2)

[Aristeu Rozanski]

Hi Alan,
> > > > The commit e0426e6a09954d205da2d674a3d368d2715e3afd fixes a real race but
> > > > still isn't enough to prevent these:
> > > > 
> > > > kobject_add_internal failed for vcs7 with -EEXIST, don't try to register
> > > > things with the same name in the same direc.
> > > 
> > > Patch dropped due to testing failures.
> > > 
> > > Boot to run level 3, log in and type "reboot\n". Wait
> > > 
> > > Spews vt->driver_data == NULL warnings and oopses
> > do you mind posting the logs somewhere? I wasn't able to reproduce it here so
> 
> I decided it might be better to tackle this one 'head on' and go to the
> root of the problem. I've pushed the following into the stack of patches
> for -next
> 
> tty: shutdown method
I agree with the idea, but:

>  /**
>   *	release_one_tty		-	release tty structure memory
>   *	@kref: kref of tty we are obliterating
> @@ -1489,27 +1514,11 @@ static void release_one_tty(struct kref *kref)
>  {
>  	struct tty_struct *tty = container_of(kref, struct tty_struct, kref);
>  	struct tty_driver *driver = tty->driver;
> -	int devpts = tty->driver->flags & TTY_DRIVER_DEVPTS_MEM;
> -	struct ktermios *tp;
> -	int idx = tty->index;
> -
> -	if (!devpts)
> -		tty->driver->ttys[idx] = NULL;
> -
> -	if (tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
> -		/* FIXME: Locking on ->termios array */
> -		tp = tty->termios;
> -		if (!devpts)
> -			tty->driver->termios[idx] = NULL;
> -		kfree(tp);
> -
> -		tp = tty->termios_locked;
> -		if (!devpts)
> -			tty->driver->termios_locked[idx] = NULL;
> -		kfree(tp);
> -	}
> -
>  
> +	if (tty->ops->shutdown)
> +		tty->ops->shutdown(tty);
> +	else
> +		tty_shutdown(tty);
>  	tty->magic = 0;
>  	/* FIXME: locking on tty->driver->refcount */
>  	tty->driver->refcount--;
isn't too late for drivers that wait for the remaining data to be transmitted
such as amiserial and cyclades? Unless there's a plan to do it at
release_dev()?

-- 
Aristeu

Re: [PATCH] vt: kill tty->count usage (v2)

[Alan Cox]

> isn't too late for drivers that wait for the remaining data to be transmitted
> such as amiserial and cyclades? Unless there's a plan to do it at
> release_dev()?

Agreed: waiting for data to be transmitted is one thing, freeing resources
another. We don't want to be freeing resources until the tty object is
destructed. Also most drivers won't be able to block there as they will
be using krefs to the tty and thus potentially destructing it from an IRQ
handler.

Re: [PATCH] vt: kill tty->count usage (v2)

[Aristeu Rozanski]

> > isn't too late for drivers that wait for the remaining data to be transmitted
> > such as amiserial and cyclades? Unless there's a plan to do it at
> > release_dev()?
> 
> Agreed: waiting for data to be transmitted is one thing, freeing resources
> another. We don't want to be freeing resources until the tty object is
> destructed. Also most drivers won't be able to block there as they will
> be using krefs to the tty and thus potentially destructing it from an IRQ
> handler.
sorry to ask again, but did you managed to make your stack of patches
available for download?

-- 
Aristeu