TTY: pty, fix pty counting

PATCH In review Ilya Zykov 2 participants 4 messages Oct 23, 2011 – Oct 24, 2011 TTY layer and serial drivers

lore

Share & tools

lore

Ilya Zykov

author

2011-10-23 21:01 UTC #0 lore raw

PATCH 1/1 TTY: pty, fix pty counting

New version for commit: 24d406a6bf736f7aebdc8fa0f0ec86e0890c6d24

drivers/tty/pty.c +4 ~~-10~~

diff -uprN a/drivers/tty/pty.c b/drivers/tty/pty.c
@@ -36,13 +36,15 @@
 static struct tty_driver *ptm_driver;
 static struct tty_driver *pts_driver;
 #endif
+static int pty_count;
 
 static void pty_close(struct tty_struct *tty, struct file *filp)
 {
 	BUG_ON(!tty);
-	if (tty->driver->subtype == PTY_TYPE_MASTER)
+	if (tty->driver->subtype == PTY_TYPE_MASTER) {
 		WARN_ON(tty->count > 1);
-	else {
+		pty_count--;
+	} else {
 		if (tty->count > 2)
 			return;
 	}

@@ -446,7 +448,6 @@ static inline void legacy_pty_init(void)
 int pty_limit = NR_UNIX98_PTY_DEFAULT;
 static int pty_limit_min;
 static int pty_limit_max = NR_UNIX98_PTY_MAX;
-static int pty_count;
 
 static struct cdev ptmx_cdev;

@@ -599,15 +600,9 @@ free_mem_out:
 	return -ENOMEM;
 }
 
-static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
-{
-	pty_count--;
-}
-
 static const struct tty_operations ptm_unix98_ops = {
 	.lookup = ptm_unix98_lookup,
 	.install = pty_unix98_install,
-	.remove = pty_unix98_remove,
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,

@@ -624,7 +619,6 @@ static const struct tty_operations ptm_u
 static const struct tty_operations pty_unix98_ops = {
 	.lookup = pts_unix98_lookup,
 	.install = pty_unix98_install,
-	.remove = pty_unix98_remove,
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,

Jiri Slaby

2011-10-24 13:11 UTC #1 lore raw

Quoted 1 line · Ilya Zykov

New version for commit: 24d406a6bf736f7aebdc8fa0f0ec86e0890c6d24

Although it will work, as ptms are not allowed to be reopen, it doesn't look correct. We should decrement the count in ->remove, because we are incrementing in install.

Now, when I understand ptm+devpts layer a bit more, instead of the current hackish approach introduced by 24d406a6b (TTY: pty, fix pty counting), I think we may introduce a ->remove hook specific to ptms. In that one we could decrement the count and don't bother with the pty_count macros anymore. Right?

BTW you cannot remove ->remove hook of pty layer. It would cause an OOPS because driver->ttys is not allocated for ptys.

Quoted 46 lines

diff -uprN a/drivers/tty/pty.c b/drivers/tty/pty.c

drivers/tty/pty.c +4 -9

@@ -36,13 +36,15 @@
 static struct tty_driver *ptm_driver;
 static struct tty_driver *pts_driver;
 #endif
+static int pty_count;
 
 static void pty_close(struct tty_struct *tty, struct file *filp)
 {
 	BUG_ON(!tty);
-	if (tty->driver->subtype == PTY_TYPE_MASTER)
+	if (tty->driver->subtype == PTY_TYPE_MASTER) {
 		WARN_ON(tty->count > 1);
-	else {
+		pty_count--;
+	} else {
 		if (tty->count > 2)
 			return;
 	}

@@ -446,7 +448,6 @@ static inline void legacy_pty_init(void)
 int pty_limit = NR_UNIX98_PTY_DEFAULT;
 static int pty_limit_min;
 static int pty_limit_max = NR_UNIX98_PTY_MAX;
-static int pty_count;
 
 static struct cdev ptmx_cdev;

@@ -599,15 +600,9 @@ free_mem_out:
 	return -ENOMEM;
 }
 
-static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
-{
-	pty_count--;
-}
-
 static const struct tty_operations ptm_unix98_ops = {
 	.lookup = ptm_unix98_lookup,
 	.install = pty_unix98_install,
-	.remove = pty_unix98_remove,
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,

drivers/tty/pty.c static const struct tty_operations ptm_u · line 624

Quoted 7 lines

drivers/tty/pty.c +0 -1

@@ -624,7 +619,6 @@ static const struct tty_operations ptm_u
 static const struct tty_operations pty_unix98_ops = {
 	.lookup = pts_unix98_lookup,
 	.install = pty_unix98_install,
-	.remove = pty_unix98_remove,
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,

thanks,

Signature · 2 lines

js
suse labs

Ilya Zykov

author

2011-10-24 14:33 UTC #2 lore raw

Quoted 63 lines · Jiri Slaby

Quoted 1 line · Ilya Zykov
New version for commit: 24d406a6bf736f7aebdc8fa0f0ec86e0890c6d24
Although it will work, as ptms are not allowed to be reopen, it doesn't look correct. We should decrement the count in ->remove, because we are incrementing in install.
Now, when I understand ptm+devpts layer a bit more, instead of the current hackish approach introduced by 24d406a6b (TTY: pty, fix pty counting), I think we may introduce a ->remove hook specific to ptms. In that one we could decrement the count and don't bother with the pty_count macros anymore. Right?
BTW you cannot remove ->remove hook of pty layer. It would cause an OOPS because driver->ttys is not allocated for ptys.
Quoted 47 lines
diff -uprN a/drivers/tty/pty.c b/drivers/tty/pty.c
drivers/tty/pty.c +4 -9
@@ -36,13 +36,15 @@
 static struct tty_driver *ptm_driver;
 static struct tty_driver *pts_driver;
 #endif
+static int pty_count;
 
 static void pty_close(struct tty_struct *tty, struct file *filp)
 {
 	BUG_ON(!tty);
-	if (tty->driver->subtype == PTY_TYPE_MASTER)
+	if (tty->driver->subtype == PTY_TYPE_MASTER) {
 		WARN_ON(tty->count > 1);
-	else {
+		pty_count--;
+	} else {
 		if (tty->count > 2)
 			return;
 	}
@@ -446,7 +448,6 @@ static inline void legacy_pty_init(void)
 int pty_limit = NR_UNIX98_PTY_DEFAULT;
 static int pty_limit_min;
 static int pty_limit_max = NR_UNIX98_PTY_MAX;
-static int pty_count;
 
 static struct cdev ptmx_cdev;
 
@@ -599,15 +600,9 @@ free_mem_out:
 	return -ENOMEM;
 }
 
-static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
-{
-	pty_count--;
-}
-
 static const struct tty_operations ptm_unix98_ops = {
 	.lookup = ptm_unix98_lookup,
 	.install = pty_unix98_install,
-	.remove = pty_unix98_remove,
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,
@@ -624,7 +619,6 @@ static const struct tty_operations ptm_u
 static const struct tty_operations pty_unix98_ops = {

Quoted 8 lines

drivers/tty/pty.c static const struct tty_operations ptm_u · line 624
Quoted 6 lines
drivers/tty/pty.c +0 -1
 	.lookup = pts_unix98_lookup,
 	.install = pty_unix98_install,
-	.remove = pty_unix98_remove,
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,
thanks,

We can increment pty_count in open()
About BTW(->remove) You say in 24d406a6b:
However tty_shutdown() is called from queue_release_one_tty() only if tty_operations->shutdown is NULL. But for pty, it is not. pty_unix98_shutdown() is used there as ->shutdown.

So tty_operations->remove of pty (i.e. pty_unix98_remove()) is never called. This results in invalid pty_count. I.e. what can be seen in /proc/sys/kernel/pty/nr.

Ilya Zykov

author

2011-10-24 14:50 UTC #3 lore raw

In reply to Jiri Slaby

Quoted 63 lines · Jiri Slaby

Quoted 1 line · Ilya Zykov
New version for commit: 24d406a6bf736f7aebdc8fa0f0ec86e0890c6d24
Although it will work, as ptms are not allowed to be reopen, it doesn't look correct. We should decrement the count in ->remove, because we are incrementing in install.
Now, when I understand ptm+devpts layer a bit more, instead of the current hackish approach introduced by 24d406a6b (TTY: pty, fix pty counting), I think we may introduce a ->remove hook specific to ptms. In that one we could decrement the count and don't bother with the pty_count macros anymore. Right?
BTW you cannot remove ->remove hook of pty layer. It would cause an OOPS because driver->ttys is not allocated for ptys.
Quoted 47 lines
diff -uprN a/drivers/tty/pty.c b/drivers/tty/pty.c
drivers/tty/pty.c +4 -9
@@ -36,13 +36,15 @@
 static struct tty_driver *ptm_driver;
 static struct tty_driver *pts_driver;
 #endif
+static int pty_count;
 
 static void pty_close(struct tty_struct *tty, struct file *filp)
 {
 	BUG_ON(!tty);
-	if (tty->driver->subtype == PTY_TYPE_MASTER)
+	if (tty->driver->subtype == PTY_TYPE_MASTER) {
 		WARN_ON(tty->count > 1);
-	else {
+		pty_count--;
+	} else {
 		if (tty->count > 2)
 			return;
 	}
@@ -446,7 +448,6 @@ static inline void legacy_pty_init(void)
 int pty_limit = NR_UNIX98_PTY_DEFAULT;
 static int pty_limit_min;
 static int pty_limit_max = NR_UNIX98_PTY_MAX;
-static int pty_count;
 
 static struct cdev ptmx_cdev;
 
@@ -599,15 +600,9 @@ free_mem_out:
 	return -ENOMEM;
 }
 
-static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
-{
-	pty_count--;
-}
-
 static const struct tty_operations ptm_unix98_ops = {
 	.lookup = ptm_unix98_lookup,
 	.install = pty_unix98_install,
-	.remove = pty_unix98_remove,
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,
@@ -624,7 +619,6 @@ static const struct tty_operations ptm_u
 static const struct tty_operations pty_unix98_ops = {

Quoted 8 lines

drivers/tty/pty.c static const struct tty_operations ptm_u · line 624
Quoted 6 lines
drivers/tty/pty.c +0 -1
 	.lookup = pts_unix98_lookup,
 	.install = pty_unix98_install,
-	.remove = pty_unix98_remove,
 	.open = pty_open,
 	.close = pty_close,
 	.write = pty_write,
thanks,

Invoke tty_driver_remove_tty() from driver,
not good idea IMHO.