* [PATCH] TTY: pty, fix pty counting
@ 2011-10-23 21:01 Ilya Zykov
2011-10-24 13:11 ` Jiri Slaby
0 siblings, 1 reply; 5+ messages in thread
From: Ilya Zykov @ 2011-10-23 21:01 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg Kroah-Hartman, Alan Cox, linux-kernel, ilya
New version for commit: 24d406a6bf736f7aebdc8fa0f0ec86e0890c6d24
diff -uprN a/drivers/tty/pty.c b/drivers/tty/pty.c
--- a/drivers/tty/pty.c 2011-05-19 08:06:34.000000000 +0400
+++ b/drivers/tty/pty.c 2011-10-23 18:01:20.000000000 +0400
@@ -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,
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] TTY: pty, fix pty counting
2011-10-23 21:01 [PATCH] TTY: pty, fix pty counting Ilya Zykov
@ 2011-10-24 13:11 ` Jiri Slaby
2011-10-24 14:33 ` Ilya Zykov
2011-10-24 14:50 ` Ilya Zykov
0 siblings, 2 replies; 5+ messages in thread
From: Jiri Slaby @ 2011-10-24 13:11 UTC (permalink / raw)
To: Ilya Zykov; +Cc: Greg Kroah-Hartman, Alan Cox, linux-kernel, Jiri Slaby
On 10/23/2011 11:01 PM, Ilya Zykov wrote:
> 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.
> diff -uprN a/drivers/tty/pty.c b/drivers/tty/pty.c
> --- a/drivers/tty/pty.c 2011-05-19 08:06:34.000000000 +0400
> +++ b/drivers/tty/pty.c 2011-10-23 18:01:20.000000000 +0400
> @@ -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,
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] TTY: pty, fix pty counting
2011-10-24 13:11 ` Jiri Slaby
@ 2011-10-24 14:33 ` Ilya Zykov
2011-10-24 14:50 ` Ilya Zykov
1 sibling, 0 replies; 5+ messages in thread
From: Ilya Zykov @ 2011-10-24 14:33 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg Kroah-Hartman, Alan Cox, linux-kernel, ilya
Jiri Slaby wrote:
> On 10/23/2011 11:01 PM, Ilya Zykov wrote:
>> 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.
>
>> diff -uprN a/drivers/tty/pty.c b/drivers/tty/pty.c
>> --- a/drivers/tty/pty.c 2011-05-19 08:06:34.000000000 +0400
>> +++ b/drivers/tty/pty.c 2011-10-23 18:01:20.000000000 +0400
>> @@ -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,
>
> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] TTY: pty, fix pty counting
2011-10-24 13:11 ` Jiri Slaby
2011-10-24 14:33 ` Ilya Zykov
@ 2011-10-24 14:50 ` Ilya Zykov
1 sibling, 0 replies; 5+ messages in thread
From: Ilya Zykov @ 2011-10-24 14:50 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg Kroah-Hartman, Alan Cox, linux-kernel, ilya
Jiri Slaby wrote:
> On 10/23/2011 11:01 PM, Ilya Zykov wrote:
>> 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.
>
>> diff -uprN a/drivers/tty/pty.c b/drivers/tty/pty.c
>> --- a/drivers/tty/pty.c 2011-05-19 08:06:34.000000000 +0400
>> +++ b/drivers/tty/pty.c 2011-10-23 18:01:20.000000000 +0400
>> @@ -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,
>
> thanks,
Invoke tty_driver_remove_tty() from driver,
not good idea IMHO.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] TTY: pty, fix pty counting
@ 2011-10-23 14:42 Ilya Zykov
0 siblings, 0 replies; 5+ messages in thread
From: Ilya Zykov @ 2011-10-23 14:42 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Jiri Slaby, Alan Cox, linux-kernel
New version for commit: 24d406a6bf736f7aebdc8fa0f0ec86e0890c6d24
diff -uprN a/drivers/tty/pty.c b/drivers/tty/pty.c
--- a/drivers/tty/pty.c 2011-05-19 08:06:34.000000000 +0400
+++ b/drivers/tty/pty.c 2011-10-23 18:01:20.000000000 +0400
@@ -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,
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-10-24 14:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-23 21:01 [PATCH] TTY: pty, fix pty counting Ilya Zykov
2011-10-24 13:11 ` Jiri Slaby
2011-10-24 14:33 ` Ilya Zykov
2011-10-24 14:50 ` Ilya Zykov
-- strict thread matches above, loose matches on Subject: below --
2011-10-23 14:42 Ilya Zykov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.