When there are to many open/close on a tty device in the same time,
there may be a warning like:
Warning: dev (ttyS0) tty->count(4) != #fd's(3) in tty_release_dev
That's because tty->count and files in tty->tty_files are not synchronized
in time.
So I add a lock to avoid this.
Signed-off-by: Li, Zhen-Hua <[email protected]>
---
drivers/tty/tty_io.c | 13 +++++++++----
include/linux/tty.h | 1 +
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 3411071..9283fc2 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -297,6 +297,7 @@ static int check_tty_count(struct tty_struct *tty, const char *routine)
struct list_head *p;
int count = 0;
+ spin_lock(&tty->count_lock);
spin_lock(&tty_files_lock);
list_for_each(p, &tty->tty_files) {
count++;
@@ -310,8 +311,10 @@ static int check_tty_count(struct tty_struct *tty, const char *routine)
printk(KERN_WARNING "Warning: dev (%s) tty->count(%d) "
"!= #fd's(%d) in %s\n",
tty->name, tty->count, count, routine);
+ spin_unlock(&tty->count_lock);
return count;
}
+ spin_unlock(&tty->count_lock);
#endif
return 0;
}
@@ -1337,7 +1340,6 @@ int tty_standard_install(struct tty_driver *driver, struct tty_struct *tty)
return ret;
tty_driver_kref_get(driver);
- tty->count++;
driver->ttys[tty->index] = tty;
return 0;
}
@@ -1408,7 +1410,6 @@ static int tty_reopen(struct tty_struct *tty)
tty->link->count++;
}
- tty->count++;
WARN_ON(!tty->ldisc);
@@ -1796,6 +1797,7 @@ int tty_release(struct inode *inode, struct file *filp)
* We must *not* drop the tty_mutex until we ensure that a further
* entry into tty_open can not pick up this tty.
*/
+ spin_lock(&tty->count_lock);
if (pty_master) {
if (--o_tty->count < 0) {
printk(KERN_WARNING "%s: bad pty slave count (%d) for %s\n",
@@ -1819,7 +1821,7 @@ int tty_release(struct inode *inode, struct file *filp)
* something that needs to be handled for hangups.
*/
tty_del_file(filp);
-
+ spin_unlock(&tty->count_lock);
/*
* Perform some housekeeping before deciding whether to return.
*
@@ -2046,8 +2048,10 @@ retry_open:
retval = PTR_ERR(tty);
goto err_file;
}
-
+ spin_lock(&tty->count_lock);
+ tty->count++;
tty_add_file(tty, filp);
+ spin_unlock(&tty->count_lock);
check_tty_count(tty, __func__);
if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
@@ -3031,6 +3035,7 @@ void initialize_tty_struct(struct tty_struct *tty,
INIT_WORK(&tty->hangup_work, do_tty_hangup);
mutex_init(&tty->atomic_write_lock);
spin_lock_init(&tty->ctrl_lock);
+ spin_lock_init(&tty->count_lock);
INIT_LIST_HEAD(&tty->tty_files);
INIT_WORK(&tty->SAK_work, do_SAK_work);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1c3316a..2959300 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -284,6 +284,7 @@ struct tty_struct {
/* If the tty has a pending do_SAK, queue it here - akpm */
struct work_struct SAK_work;
struct tty_port *port;
+ spinlock_t count_lock;
};
/* Each of a tty's open files has private_data pointing to tty_file_private */
--
2.0.0-rc0
Hi Zhen-Hua,
On 07/14/2014 01:54 AM, Li, Zhen-Hua wrote:
> When there are to many open/close on a tty device in the same time,
> there may be a warning like:
> Warning: dev (ttyS0) tty->count(4) != #fd's(3) in tty_release_dev
>
> That's because tty->count and files in tty->tty_files are not synchronized
> in time.
> So I add a lock to avoid this.
Every place you added the count_lock spinlock is already protected
by tty_lock(tty), so the count is out of sync for some other reason.
What kernel version are you having this problem on and what are the
circumstances?
Regards,
Peter Hurley
> Signed-off-by: Li, Zhen-Hua <[email protected]>
> ---
> drivers/tty/tty_io.c | 13 +++++++++----
> include/linux/tty.h | 1 +
> 2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 3411071..9283fc2 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -297,6 +297,7 @@ static int check_tty_count(struct tty_struct *tty, const char *routine)
> struct list_head *p;
> int count = 0;
>
> + spin_lock(&tty->count_lock);
> spin_lock(&tty_files_lock);
> list_for_each(p, &tty->tty_files) {
> count++;
> @@ -310,8 +311,10 @@ static int check_tty_count(struct tty_struct *tty, const char *routine)
> printk(KERN_WARNING "Warning: dev (%s) tty->count(%d) "
> "!= #fd's(%d) in %s\n",
> tty->name, tty->count, count, routine);
> + spin_unlock(&tty->count_lock);
> return count;
> }
> + spin_unlock(&tty->count_lock);
> #endif
> return 0;
> }
> @@ -1337,7 +1340,6 @@ int tty_standard_install(struct tty_driver *driver, struct tty_struct *tty)
> return ret;
>
> tty_driver_kref_get(driver);
> - tty->count++;
> driver->ttys[tty->index] = tty;
> return 0;
> }
> @@ -1408,7 +1410,6 @@ static int tty_reopen(struct tty_struct *tty)
>
> tty->link->count++;
> }
> - tty->count++;
>
> WARN_ON(!tty->ldisc);
>
> @@ -1796,6 +1797,7 @@ int tty_release(struct inode *inode, struct file *filp)
> * We must *not* drop the tty_mutex until we ensure that a further
> * entry into tty_open can not pick up this tty.
> */
> + spin_lock(&tty->count_lock);
> if (pty_master) {
> if (--o_tty->count < 0) {
> printk(KERN_WARNING "%s: bad pty slave count (%d) for %s\n",
> @@ -1819,7 +1821,7 @@ int tty_release(struct inode *inode, struct file *filp)
> * something that needs to be handled for hangups.
> */
> tty_del_file(filp);
> -
> + spin_unlock(&tty->count_lock);
> /*
> * Perform some housekeeping before deciding whether to return.
> *
> @@ -2046,8 +2048,10 @@ retry_open:
> retval = PTR_ERR(tty);
> goto err_file;
> }
> -
> + spin_lock(&tty->count_lock);
> + tty->count++;
> tty_add_file(tty, filp);
> + spin_unlock(&tty->count_lock);
>
> check_tty_count(tty, __func__);
> if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> @@ -3031,6 +3035,7 @@ void initialize_tty_struct(struct tty_struct *tty,
> INIT_WORK(&tty->hangup_work, do_tty_hangup);
> mutex_init(&tty->atomic_write_lock);
> spin_lock_init(&tty->ctrl_lock);
> + spin_lock_init(&tty->count_lock);
> INIT_LIST_HEAD(&tty->tty_files);
> INIT_WORK(&tty->SAK_work, do_SAK_work);
>
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 1c3316a..2959300 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -284,6 +284,7 @@ struct tty_struct {
> /* If the tty has a pending do_SAK, queue it here - akpm */
> struct work_struct SAK_work;
> struct tty_port *port;
> + spinlock_t count_lock;
> };
>
> /* Each of a tty's open files has private_data pointing to tty_file_private */
>
Hi Peter,
This bug was founded in RHEL 6, kernel version 2.6.32.
But I also checked the file tty_io.c in latest kernel, not all the code
protected by tty_lock(tty).
For example, in function tty_del_file,
if (--tty->count < 0) {
}
and
tty_del_file(filp);
are not surrounded by tty_lock(tty).
So I think the new lock is necessary.
Thanks
Zhenhua
On 07/14/2014 08:25 PM, Peter Hurley wrote:
> Hi Zhen-Hua,
>
> On 07/14/2014 01:54 AM, Li, Zhen-Hua wrote:
>> When there are to many open/close on a tty device in the same time,
>> there may be a warning like:
>> Warning: dev (ttyS0) tty->count(4) != #fd's(3) in tty_release_dev
>>
>> That's because tty->count and files in tty->tty_files are not
>> synchronized
>> in time.
>> So I add a lock to avoid this.
>
> Every place you added the count_lock spinlock is already protected
> by tty_lock(tty), so the count is out of sync for some other reason.
>
> What kernel version are you having this problem on and what are the
> circumstances?
>
> Regards,
> Peter Hurley
>
>
>> Signed-off-by: Li, Zhen-Hua <[email protected]>
>> ---
>> drivers/tty/tty_io.c | 13 +++++++++----
>> include/linux/tty.h | 1 +
>> 2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index 3411071..9283fc2 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -297,6 +297,7 @@ static int check_tty_count(struct tty_struct
>> *tty, const char *routine)
>> struct list_head *p;
>> int count = 0;
>>
>> + spin_lock(&tty->count_lock);
>> spin_lock(&tty_files_lock);
>> list_for_each(p, &tty->tty_files) {
>> count++;
>> @@ -310,8 +311,10 @@ static int check_tty_count(struct tty_struct
>> *tty, const char *routine)
>> printk(KERN_WARNING "Warning: dev (%s) tty->count(%d) "
>> "!= #fd's(%d) in %s\n",
>> tty->name, tty->count, count, routine);
>> + spin_unlock(&tty->count_lock);
>> return count;
>> }
>> + spin_unlock(&tty->count_lock);
>> #endif
>> return 0;
>> }
>> @@ -1337,7 +1340,6 @@ int tty_standard_install(struct tty_driver
>> *driver, struct tty_struct *tty)
>> return ret;
>>
>> tty_driver_kref_get(driver);
>> - tty->count++;
>> driver->ttys[tty->index] = tty;
>> return 0;
>> }
>> @@ -1408,7 +1410,6 @@ static int tty_reopen(struct tty_struct *tty)
>>
>> tty->link->count++;
>> }
>> - tty->count++;
>>
>> WARN_ON(!tty->ldisc);
>>
>> @@ -1796,6 +1797,7 @@ int tty_release(struct inode *inode, struct
>> file *filp)
>> * We must *not* drop the tty_mutex until we ensure that a further
>> * entry into tty_open can not pick up this tty.
>> */
>> + spin_lock(&tty->count_lock);
>> if (pty_master) {
>> if (--o_tty->count < 0) {
>> printk(KERN_WARNING "%s: bad pty slave count (%d) for
>> %s\n",
>> @@ -1819,7 +1821,7 @@ int tty_release(struct inode *inode, struct
>> file *filp)
>> * something that needs to be handled for hangups.
>> */
>> tty_del_file(filp);
>> -
>> + spin_unlock(&tty->count_lock);
>> /*
>> * Perform some housekeeping before deciding whether to return.
>> *
>> @@ -2046,8 +2048,10 @@ retry_open:
>> retval = PTR_ERR(tty);
>> goto err_file;
>> }
>> -
>> + spin_lock(&tty->count_lock);
>> + tty->count++;
>> tty_add_file(tty, filp);
>> + spin_unlock(&tty->count_lock);
>>
>> check_tty_count(tty, __func__);
>> if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
>> @@ -3031,6 +3035,7 @@ void initialize_tty_struct(struct tty_struct *tty,
>> INIT_WORK(&tty->hangup_work, do_tty_hangup);
>> mutex_init(&tty->atomic_write_lock);
>> spin_lock_init(&tty->ctrl_lock);
>> + spin_lock_init(&tty->count_lock);
>> INIT_LIST_HEAD(&tty->tty_files);
>> INIT_WORK(&tty->SAK_work, do_SAK_work);
>>
>> diff --git a/include/linux/tty.h b/include/linux/tty.h
>> index 1c3316a..2959300 100644
>> --- a/include/linux/tty.h
>> +++ b/include/linux/tty.h
>> @@ -284,6 +284,7 @@ struct tty_struct {
>> /* If the tty has a pending do_SAK, queue it here - akpm */
>> struct work_struct SAK_work;
>> struct tty_port *port;
>> + spinlock_t count_lock;
>> };
>>
>> /* Each of a tty's open files has private_data pointing to
>> tty_file_private */
>>
>
Hi,
On 07/15/2014 11:08 AM, Li, ZhenHua wrote:
> This bug was founded in RHEL 6, kernel version 2.6.32.
>
> But I also checked the file tty_io.c in latest kernel, not all the code
> protected by tty_lock(tty).
>
> For example, in function tty_del_file,
>
> if (--tty->count < 0) {
> }
> and
> tty_del_file(filp);
> are not surrounded by tty_lock(tty).
Sure, but there is tty_lock_pair instead.
> So I think the new lock is necessary.
Nope, sorry, you have to complain to RedHat about this kernel. Unless
you can reproduce this with the vanilla kernel.
thanks,
--
js
suse labs
Hi Jiri,
You are right, I tested the latest kernel, did not see this bug.
So I will only submit this patch to Redhat.
Thanks
Zhenhua
On 07/15/2014 05:16 PM, Jiri Slaby wrote:
> Hi,
>
> On 07/15/2014 11:08 AM, Li, ZhenHua wrote:
>> This bug was founded in RHEL 6, kernel version 2.6.32.
>>
>> But I also checked the file tty_io.c in latest kernel, not all the code
>> protected by tty_lock(tty).
>>
>> For example, in function tty_del_file,
>>
>> if (--tty->count < 0) {
>> }
>> and
>> tty_del_file(filp);
>> are not surrounded by tty_lock(tty).
> Sure, but there is tty_lock_pair instead.
>
>> So I think the new lock is necessary.
> Nope, sorry, you have to complain to RedHat about this kernel. Unless
> you can reproduce this with the vanilla kernel.
>
> thanks,