* [PATCH next] n_tty: change lock oreding in n_tty_read()
@ 2013-08-01 19:31 Sergey Senozhatsky
2013-08-01 19:35 ` Sergey Senozhatsky
2013-08-01 19:46 ` [PATCH next] n_tty: change lock ordering in n_tty_read() (v2) Sergey Senozhatsky
0 siblings, 2 replies; 5+ messages in thread
From: Sergey Senozhatsky @ 2013-08-01 19:31 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg Kroah-Hartman, Peter Hurley, linux-kernel
Acquire atomic_read_lock before termios_rwsem rw-lock in n_tty_read().
termios_rwsem is getting released and re-acquired, while remaining
readers are blocked on atomic_read_lock mutex, in case when process
must wait for input_available_p(), making lockdep unhappy:
[ 463.542463] ======================================================
[ 463.542464] [ INFO: possible circular locking dependency detected ]
[ 463.542466] 3.11.0-rc3-next-20130801-dbg-dirty #5 Not tainted
[ 463.542467] -------------------------------------------------------
[ 463.542469] agetty/2075 is trying to acquire lock:
[ 463.542484] (&tty->termios_rwsem){++++..}, at: [<ffffffff81398660>] n_tty_read+0x460/0xab0
[ 463.542484]
but task is already holding lock:
[ 463.542496] (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff81398959>] n_tty_read+0x759/0xab0
[ 463.542497]
which lock already depends on the new lock.
[ 463.542497]
the existing dependency chain (in reverse order) is:
[ 463.542502]
-> #1 (&ldata->atomic_read_lock){+.+...}:
[ 463.542508] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
[ 463.542514] [<ffffffff8160a787>] mutex_lock_interruptible_nested+0x77/0x4e0
[ 463.542518] [<ffffffff81398959>] n_tty_read+0x759/0xab0
[ 463.542524] [<ffffffff81393e8d>] tty_read+0x8d/0x100
[ 463.542528] [<ffffffff8117987a>] vfs_read+0x9a/0x170
[ 463.542531] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
[ 463.542536] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
[ 463.542540]
-> #0 (&tty->termios_rwsem){++++..}:
[ 463.542543] [<ffffffff810b84e6>] __lock_acquire+0x1756/0x1d20
[ 463.542547] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
[ 463.542550] [<ffffffff8160c1b7>] down_read+0x47/0x60
[ 463.542554] [<ffffffff81398660>] n_tty_read+0x460/0xab0
[ 463.542557] [<ffffffff81393e8d>] tty_read+0x8d/0x100
[ 463.542560] [<ffffffff8117987a>] vfs_read+0x9a/0x170
[ 463.542563] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
[ 463.542566] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
[ 463.542567]
other info that might help us debug this:
[ 463.542569] Possible unsafe locking scenario:
[ 463.542570] CPU0 CPU1
[ 463.542570] ---- ----
[ 463.542573] lock(&ldata->atomic_read_lock);
[ 463.542575] lock(&tty->termios_rwsem);
[ 463.542577] lock(&ldata->atomic_read_lock);
[ 463.542579] lock(&tty->termios_rwsem);
[ 463.542580]
*** DEADLOCK ***
[ 463.542582] 2 locks held by agetty/2075:
[ 463.542590] #0: (&tty->ldisc_sem){++++++}, at: [<ffffffff8139c130>] tty_ldisc_ref_wait+0x20/0x50
[ 463.542597] #1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff81398959>] n_tty_read+0x759/0xab0
[ 463.542598]
stack backtrace:
[ 463.542602] CPU: 0 PID: 2075 Comm: agetty Not tainted 3.11.0-rc3-next-20130801-dbg-dirty #5
[ 463.542603] Hardware name: Acer Aspire 5741G /Aspire 5741G , BIOS V1.20 02/08/2011
[ 463.542609] ffffffff82236010 ffff88009af67c28 ffffffff8160630a ffffffff82236010
[ 463.542613] ffff88009af67c68 ffffffff816031b7 ffff880151ac0000 ffff880151ac0790
[ 463.542617] 000000073469e1bd ffff880151ac0768 ffff880151ac0790 ffff880151ac0000
[ 463.542618] Call Trace:
[ 463.542625] [<ffffffff8160630a>] dump_stack+0x4e/0x82
[ 463.542630] [<ffffffff816031b7>] print_circular_bug+0x2b6/0x2c5
[ 463.542634] [<ffffffff810b84e6>] __lock_acquire+0x1756/0x1d20
[ 463.542638] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
[ 463.542642] [<ffffffff81398660>] ? n_tty_read+0x460/0xab0
[ 463.542645] [<ffffffff8160c1b7>] down_read+0x47/0x60
[ 463.542649] [<ffffffff81398660>] ? n_tty_read+0x460/0xab0
[ 463.542653] [<ffffffff81398660>] n_tty_read+0x460/0xab0
[ 463.542661] [<ffffffff81085020>] ? wake_up_process+0x40/0x40
[ 463.542665] [<ffffffff81393e8d>] tty_read+0x8d/0x100
[ 463.542668] [<ffffffff8117987a>] vfs_read+0x9a/0x170
[ 463.542671] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
[ 463.542674] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/tty/n_tty.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index dd8ae0c..03bd6e8 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2122,8 +2122,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
if (c < 0)
return c;
- down_read(&tty->termios_rwsem);
-
minimum = time = 0;
timeout = MAX_SCHEDULE_TIMEOUT;
if (!ldata->icanon) {
@@ -2145,16 +2143,15 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
* Internal serialization of reads.
*/
if (file->f_flags & O_NONBLOCK) {
- if (!mutex_trylock(&ldata->atomic_read_lock)) {
- up_read(&tty->termios_rwsem);
+ if (!mutex_trylock(&ldata->atomic_read_lock))
return -EAGAIN;
- }
} else {
- if (mutex_lock_interruptible(&ldata->atomic_read_lock)) {
- up_read(&tty->termios_rwsem);
+ if (mutex_lock_interruptible(&ldata->atomic_read_lock))
return -ERESTARTSYS;
- }
}
+
+ down_read(&tty->termios_rwsem);
+
packet = tty->packet;
add_wait_queue(&tty->read_wait, &wait);
@@ -2248,7 +2245,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
if (time)
timeout = time;
}
- mutex_unlock(&ldata->atomic_read_lock);
remove_wait_queue(&tty->read_wait, &wait);
if (!waitqueue_active(&tty->read_wait))
@@ -2260,6 +2256,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
n_tty_set_room(tty);
up_read(&tty->termios_rwsem);
+ mutex_unlock(&ldata->atomic_read_lock);
return retval;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH next] n_tty: change lock oreding in n_tty_read()
2013-08-01 19:31 [PATCH next] n_tty: change lock oreding in n_tty_read() Sergey Senozhatsky
@ 2013-08-01 19:35 ` Sergey Senozhatsky
2013-08-01 19:46 ` [PATCH next] n_tty: change lock ordering in n_tty_read() (v2) Sergey Senozhatsky
1 sibling, 0 replies; 5+ messages in thread
From: Sergey Senozhatsky @ 2013-08-01 19:35 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg Kroah-Hartman, Peter Hurley, linux-kernel
Oops. Really sorry about that. I didn't pay attention and made a silly
typo in the Subject line. Of course, it was going to be "change lock
*ordering* in n_tty_read()". I'll resend the patch.
-ss
On (08/01/13 22:31), Sergey Senozhatsky wrote:
> Acquire atomic_read_lock before termios_rwsem rw-lock in n_tty_read().
> termios_rwsem is getting released and re-acquired, while remaining
> readers are blocked on atomic_read_lock mutex, in case when process
> must wait for input_available_p(), making lockdep unhappy:
>
> [ 463.542463] ======================================================
> [ 463.542464] [ INFO: possible circular locking dependency detected ]
> [ 463.542466] 3.11.0-rc3-next-20130801-dbg-dirty #5 Not tainted
> [ 463.542467] -------------------------------------------------------
> [ 463.542469] agetty/2075 is trying to acquire lock:
> [ 463.542484] (&tty->termios_rwsem){++++..}, at: [<ffffffff81398660>] n_tty_read+0x460/0xab0
> [ 463.542484]
> but task is already holding lock:
> [ 463.542496] (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff81398959>] n_tty_read+0x759/0xab0
> [ 463.542497]
> which lock already depends on the new lock.
>
> [ 463.542497]
> the existing dependency chain (in reverse order) is:
> [ 463.542502]
> -> #1 (&ldata->atomic_read_lock){+.+...}:
> [ 463.542508] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
> [ 463.542514] [<ffffffff8160a787>] mutex_lock_interruptible_nested+0x77/0x4e0
> [ 463.542518] [<ffffffff81398959>] n_tty_read+0x759/0xab0
> [ 463.542524] [<ffffffff81393e8d>] tty_read+0x8d/0x100
> [ 463.542528] [<ffffffff8117987a>] vfs_read+0x9a/0x170
> [ 463.542531] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
> [ 463.542536] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
> [ 463.542540]
> -> #0 (&tty->termios_rwsem){++++..}:
> [ 463.542543] [<ffffffff810b84e6>] __lock_acquire+0x1756/0x1d20
> [ 463.542547] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
> [ 463.542550] [<ffffffff8160c1b7>] down_read+0x47/0x60
> [ 463.542554] [<ffffffff81398660>] n_tty_read+0x460/0xab0
> [ 463.542557] [<ffffffff81393e8d>] tty_read+0x8d/0x100
> [ 463.542560] [<ffffffff8117987a>] vfs_read+0x9a/0x170
> [ 463.542563] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
> [ 463.542566] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
> [ 463.542567]
> other info that might help us debug this:
>
> [ 463.542569] Possible unsafe locking scenario:
>
> [ 463.542570] CPU0 CPU1
> [ 463.542570] ---- ----
> [ 463.542573] lock(&ldata->atomic_read_lock);
> [ 463.542575] lock(&tty->termios_rwsem);
> [ 463.542577] lock(&ldata->atomic_read_lock);
> [ 463.542579] lock(&tty->termios_rwsem);
> [ 463.542580]
> *** DEADLOCK ***
>
> [ 463.542582] 2 locks held by agetty/2075:
> [ 463.542590] #0: (&tty->ldisc_sem){++++++}, at: [<ffffffff8139c130>] tty_ldisc_ref_wait+0x20/0x50
> [ 463.542597] #1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff81398959>] n_tty_read+0x759/0xab0
> [ 463.542598]
> stack backtrace:
> [ 463.542602] CPU: 0 PID: 2075 Comm: agetty Not tainted 3.11.0-rc3-next-20130801-dbg-dirty #5
> [ 463.542603] Hardware name: Acer Aspire 5741G /Aspire 5741G , BIOS V1.20 02/08/2011
> [ 463.542609] ffffffff82236010 ffff88009af67c28 ffffffff8160630a ffffffff82236010
> [ 463.542613] ffff88009af67c68 ffffffff816031b7 ffff880151ac0000 ffff880151ac0790
> [ 463.542617] 000000073469e1bd ffff880151ac0768 ffff880151ac0790 ffff880151ac0000
> [ 463.542618] Call Trace:
> [ 463.542625] [<ffffffff8160630a>] dump_stack+0x4e/0x82
> [ 463.542630] [<ffffffff816031b7>] print_circular_bug+0x2b6/0x2c5
> [ 463.542634] [<ffffffff810b84e6>] __lock_acquire+0x1756/0x1d20
> [ 463.542638] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
> [ 463.542642] [<ffffffff81398660>] ? n_tty_read+0x460/0xab0
> [ 463.542645] [<ffffffff8160c1b7>] down_read+0x47/0x60
> [ 463.542649] [<ffffffff81398660>] ? n_tty_read+0x460/0xab0
> [ 463.542653] [<ffffffff81398660>] n_tty_read+0x460/0xab0
> [ 463.542661] [<ffffffff81085020>] ? wake_up_process+0x40/0x40
> [ 463.542665] [<ffffffff81393e8d>] tty_read+0x8d/0x100
> [ 463.542668] [<ffffffff8117987a>] vfs_read+0x9a/0x170
> [ 463.542671] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
> [ 463.542674] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>
> ---
>
> drivers/tty/n_tty.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index dd8ae0c..03bd6e8 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -2122,8 +2122,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
> if (c < 0)
> return c;
>
> - down_read(&tty->termios_rwsem);
> -
> minimum = time = 0;
> timeout = MAX_SCHEDULE_TIMEOUT;
> if (!ldata->icanon) {
> @@ -2145,16 +2143,15 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
> * Internal serialization of reads.
> */
> if (file->f_flags & O_NONBLOCK) {
> - if (!mutex_trylock(&ldata->atomic_read_lock)) {
> - up_read(&tty->termios_rwsem);
> + if (!mutex_trylock(&ldata->atomic_read_lock))
> return -EAGAIN;
> - }
> } else {
> - if (mutex_lock_interruptible(&ldata->atomic_read_lock)) {
> - up_read(&tty->termios_rwsem);
> + if (mutex_lock_interruptible(&ldata->atomic_read_lock))
> return -ERESTARTSYS;
> - }
> }
> +
> + down_read(&tty->termios_rwsem);
> +
> packet = tty->packet;
>
> add_wait_queue(&tty->read_wait, &wait);
> @@ -2248,7 +2245,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
> if (time)
> timeout = time;
> }
> - mutex_unlock(&ldata->atomic_read_lock);
> remove_wait_queue(&tty->read_wait, &wait);
>
> if (!waitqueue_active(&tty->read_wait))
> @@ -2260,6 +2256,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
>
> n_tty_set_room(tty);
> up_read(&tty->termios_rwsem);
> + mutex_unlock(&ldata->atomic_read_lock);
> return retval;
> }
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH next] n_tty: change lock ordering in n_tty_read() (v2)
2013-08-01 19:31 [PATCH next] n_tty: change lock oreding in n_tty_read() Sergey Senozhatsky
2013-08-01 19:35 ` Sergey Senozhatsky
@ 2013-08-01 19:46 ` Sergey Senozhatsky
2013-08-01 20:01 ` Peter Hurley
1 sibling, 1 reply; 5+ messages in thread
From: Sergey Senozhatsky @ 2013-08-01 19:46 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg Kroah-Hartman, Peter Hurley, linux-kernel
Acquire atomic_read_lock before termios_rwsem rw-lock in n_tty_read().
termios_rwsem is getting released and re-acquired, while remaining
readers are blocked on atomic_read_lock mutex, in case when process
must wait for input_available_p(), making lockdep unhappy:
[ 463.542463] ======================================================
[ 463.542464] [ INFO: possible circular locking dependency detected ]
[ 463.542466] 3.11.0-rc3-next-20130801-dbg-dirty #5 Not tainted
[ 463.542467] -------------------------------------------------------
[ 463.542469] agetty/2075 is trying to acquire lock:
[ 463.542484] (&tty->termios_rwsem){++++..}, at: [<ffffffff81398660>] n_tty_read+0x460/0xab0
[ 463.542484]
but task is already holding lock:
[ 463.542496] (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff81398959>] n_tty_read+0x759/0xab0
[ 463.542497]
which lock already depends on the new lock.
[ 463.542497]
the existing dependency chain (in reverse order) is:
[ 463.542502]
-> #1 (&ldata->atomic_read_lock){+.+...}:
[ 463.542508] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
[ 463.542514] [<ffffffff8160a787>] mutex_lock_interruptible_nested+0x77/0x4e0
[ 463.542518] [<ffffffff81398959>] n_tty_read+0x759/0xab0
[ 463.542524] [<ffffffff81393e8d>] tty_read+0x8d/0x100
[ 463.542528] [<ffffffff8117987a>] vfs_read+0x9a/0x170
[ 463.542531] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
[ 463.542536] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
[ 463.542540]
-> #0 (&tty->termios_rwsem){++++..}:
[ 463.542543] [<ffffffff810b84e6>] __lock_acquire+0x1756/0x1d20
[ 463.542547] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
[ 463.542550] [<ffffffff8160c1b7>] down_read+0x47/0x60
[ 463.542554] [<ffffffff81398660>] n_tty_read+0x460/0xab0
[ 463.542557] [<ffffffff81393e8d>] tty_read+0x8d/0x100
[ 463.542560] [<ffffffff8117987a>] vfs_read+0x9a/0x170
[ 463.542563] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
[ 463.542566] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
[ 463.542567]
other info that might help us debug this:
[ 463.542569] Possible unsafe locking scenario:
[ 463.542570] CPU0 CPU1
[ 463.542570] ---- ----
[ 463.542573] lock(&ldata->atomic_read_lock);
[ 463.542575] lock(&tty->termios_rwsem);
[ 463.542577] lock(&ldata->atomic_read_lock);
[ 463.542579] lock(&tty->termios_rwsem);
[ 463.542580]
*** DEADLOCK ***
[ 463.542582] 2 locks held by agetty/2075:
[ 463.542590] #0: (&tty->ldisc_sem){++++++}, at: [<ffffffff8139c130>] tty_ldisc_ref_wait+0x20/0x50
[ 463.542597] #1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff81398959>] n_tty_read+0x759/0xab0
[ 463.542598]
stack backtrace:
[ 463.542602] CPU: 0 PID: 2075 Comm: agetty Not tainted 3.11.0-rc3-next-20130801-dbg-dirty #5
[ 463.542603] Hardware name: Acer Aspire 5741G /Aspire 5741G , BIOS V1.20 02/08/2011
[ 463.542609] ffffffff82236010 ffff88009af67c28 ffffffff8160630a ffffffff82236010
[ 463.542613] ffff88009af67c68 ffffffff816031b7 ffff880151ac0000 ffff880151ac0790
[ 463.542617] 000000073469e1bd ffff880151ac0768 ffff880151ac0790 ffff880151ac0000
[ 463.542618] Call Trace:
[ 463.542625] [<ffffffff8160630a>] dump_stack+0x4e/0x82
[ 463.542630] [<ffffffff816031b7>] print_circular_bug+0x2b6/0x2c5
[ 463.542634] [<ffffffff810b84e6>] __lock_acquire+0x1756/0x1d20
[ 463.542638] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
[ 463.542642] [<ffffffff81398660>] ? n_tty_read+0x460/0xab0
[ 463.542645] [<ffffffff8160c1b7>] down_read+0x47/0x60
[ 463.542649] [<ffffffff81398660>] ? n_tty_read+0x460/0xab0
[ 463.542653] [<ffffffff81398660>] n_tty_read+0x460/0xab0
[ 463.542661] [<ffffffff81085020>] ? wake_up_process+0x40/0x40
[ 463.542665] [<ffffffff81393e8d>] tty_read+0x8d/0x100
[ 463.542668] [<ffffffff8117987a>] vfs_read+0x9a/0x170
[ 463.542671] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
[ 463.542674] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
v2: correct subject typo.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/tty/n_tty.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index dd8ae0c..03bd6e8 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2122,8 +2122,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
if (c < 0)
return c;
- down_read(&tty->termios_rwsem);
-
minimum = time = 0;
timeout = MAX_SCHEDULE_TIMEOUT;
if (!ldata->icanon) {
@@ -2145,16 +2143,15 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
* Internal serialization of reads.
*/
if (file->f_flags & O_NONBLOCK) {
- if (!mutex_trylock(&ldata->atomic_read_lock)) {
- up_read(&tty->termios_rwsem);
+ if (!mutex_trylock(&ldata->atomic_read_lock))
return -EAGAIN;
- }
} else {
- if (mutex_lock_interruptible(&ldata->atomic_read_lock)) {
- up_read(&tty->termios_rwsem);
+ if (mutex_lock_interruptible(&ldata->atomic_read_lock))
return -ERESTARTSYS;
- }
}
+
+ down_read(&tty->termios_rwsem);
+
packet = tty->packet;
add_wait_queue(&tty->read_wait, &wait);
@@ -2248,7 +2245,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
if (time)
timeout = time;
}
- mutex_unlock(&ldata->atomic_read_lock);
remove_wait_queue(&tty->read_wait, &wait);
if (!waitqueue_active(&tty->read_wait))
@@ -2260,6 +2256,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
n_tty_set_room(tty);
up_read(&tty->termios_rwsem);
+ mutex_unlock(&ldata->atomic_read_lock);
return retval;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH next] n_tty: change lock ordering in n_tty_read() (v2)
2013-08-01 19:46 ` [PATCH next] n_tty: change lock ordering in n_tty_read() (v2) Sergey Senozhatsky
@ 2013-08-01 20:01 ` Peter Hurley
2013-08-01 20:34 ` Sergey Senozhatsky
0 siblings, 1 reply; 5+ messages in thread
From: Peter Hurley @ 2013-08-01 20:01 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jiri Slaby, Greg Kroah-Hartman, linux-kernel
On 08/01/2013 03:46 PM, Sergey Senozhatsky wrote:
> Acquire atomic_read_lock before termios_rwsem rw-lock in n_tty_read().
> termios_rwsem is getting released and re-acquired, while remaining
> readers are blocked on atomic_read_lock mutex, in case when process
> must wait for input_available_p(), making lockdep unhappy:
Thanks for the report. This is a regression in lockdep.
> [ 463.542463] ======================================================
> [ 463.542464] [ INFO: possible circular locking dependency detected ]
> [ 463.542466] 3.11.0-rc3-next-20130801-dbg-dirty #5 Not tainted
> [ 463.542467] -------------------------------------------------------
> [ 463.542469] agetty/2075 is trying to acquire lock:
> [ 463.542484] (&tty->termios_rwsem){++++..}, at: [<ffffffff81398660>] n_tty_read+0x460/0xab0
> [ 463.542484]
> but task is already holding lock:
> [ 463.542496] (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff81398959>] n_tty_read+0x759/0xab0
> [ 463.542497]
> which lock already depends on the new lock.
>
> [ 463.542497]
> the existing dependency chain (in reverse order) is:
> [ 463.542502]
> -> #1 (&ldata->atomic_read_lock){+.+...}:
> [ 463.542508] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
> [ 463.542514] [<ffffffff8160a787>] mutex_lock_interruptible_nested+0x77/0x4e0
> [ 463.542518] [<ffffffff81398959>] n_tty_read+0x759/0xab0
> [ 463.542524] [<ffffffff81393e8d>] tty_read+0x8d/0x100
> [ 463.542528] [<ffffffff8117987a>] vfs_read+0x9a/0x170
> [ 463.542531] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
> [ 463.542536] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
> [ 463.542540]
> -> #0 (&tty->termios_rwsem){++++..}:
> [ 463.542543] [<ffffffff810b84e6>] __lock_acquire+0x1756/0x1d20
> [ 463.542547] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
> [ 463.542550] [<ffffffff8160c1b7>] down_read+0x47/0x60
> [ 463.542554] [<ffffffff81398660>] n_tty_read+0x460/0xab0
> [ 463.542557] [<ffffffff81393e8d>] tty_read+0x8d/0x100
> [ 463.542560] [<ffffffff8117987a>] vfs_read+0x9a/0x170
> [ 463.542563] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
> [ 463.542566] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
> [ 463.542567]
> other info that might help us debug this:
>
> [ 463.542569] Possible unsafe locking scenario:
>
> [ 463.542570] CPU0 CPU1
> [ 463.542570] ---- ----
> [ 463.542573] lock(&ldata->atomic_read_lock);
> [ 463.542575] lock(&tty->termios_rwsem);
> [ 463.542577] lock(&ldata->atomic_read_lock);
> [ 463.542579] lock(&tty->termios_rwsem);
> [ 463.542580]
> *** DEADLOCK ***
False positive.
This deadlock is not possible because CPU1 only holds a read lock
which cannot prevent CPU0 from obtaining a read lock on the same
read/write semaphore.
I'll be only too happy to track down where the regression was
introduced as soon as I fix an actual lock order problem in
the nouveau driver which disables lockdep :)
Regards,
Peter Hurley
> [ 463.542582] 2 locks held by agetty/2075:
> [ 463.542590] #0: (&tty->ldisc_sem){++++++}, at: [<ffffffff8139c130>] tty_ldisc_ref_wait+0x20/0x50
> [ 463.542597] #1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff81398959>] n_tty_read+0x759/0xab0
> [ 463.542598]
> stack backtrace:
> [ 463.542602] CPU: 0 PID: 2075 Comm: agetty Not tainted 3.11.0-rc3-next-20130801-dbg-dirty #5
> [ 463.542603] Hardware name: Acer Aspire 5741G /Aspire 5741G , BIOS V1.20 02/08/2011
> [ 463.542609] ffffffff82236010 ffff88009af67c28 ffffffff8160630a ffffffff82236010
> [ 463.542613] ffff88009af67c68 ffffffff816031b7 ffff880151ac0000 ffff880151ac0790
> [ 463.542617] 000000073469e1bd ffff880151ac0768 ffff880151ac0790 ffff880151ac0000
> [ 463.542618] Call Trace:
> [ 463.542625] [<ffffffff8160630a>] dump_stack+0x4e/0x82
> [ 463.542630] [<ffffffff816031b7>] print_circular_bug+0x2b6/0x2c5
> [ 463.542634] [<ffffffff810b84e6>] __lock_acquire+0x1756/0x1d20
> [ 463.542638] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
> [ 463.542642] [<ffffffff81398660>] ? n_tty_read+0x460/0xab0
> [ 463.542645] [<ffffffff8160c1b7>] down_read+0x47/0x60
> [ 463.542649] [<ffffffff81398660>] ? n_tty_read+0x460/0xab0
> [ 463.542653] [<ffffffff81398660>] n_tty_read+0x460/0xab0
> [ 463.542661] [<ffffffff81085020>] ? wake_up_process+0x40/0x40
> [ 463.542665] [<ffffffff81393e8d>] tty_read+0x8d/0x100
> [ 463.542668] [<ffffffff8117987a>] vfs_read+0x9a/0x170
> [ 463.542671] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
> [ 463.542674] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
>
> v2: correct subject typo.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>
> ---
>
> drivers/tty/n_tty.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index dd8ae0c..03bd6e8 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -2122,8 +2122,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
> if (c < 0)
> return c;
>
> - down_read(&tty->termios_rwsem);
> -
> minimum = time = 0;
> timeout = MAX_SCHEDULE_TIMEOUT;
> if (!ldata->icanon) {
> @@ -2145,16 +2143,15 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
> * Internal serialization of reads.
> */
> if (file->f_flags & O_NONBLOCK) {
> - if (!mutex_trylock(&ldata->atomic_read_lock)) {
> - up_read(&tty->termios_rwsem);
> + if (!mutex_trylock(&ldata->atomic_read_lock))
> return -EAGAIN;
> - }
> } else {
> - if (mutex_lock_interruptible(&ldata->atomic_read_lock)) {
> - up_read(&tty->termios_rwsem);
> + if (mutex_lock_interruptible(&ldata->atomic_read_lock))
> return -ERESTARTSYS;
> - }
> }
> +
> + down_read(&tty->termios_rwsem);
> +
> packet = tty->packet;
>
> add_wait_queue(&tty->read_wait, &wait);
> @@ -2248,7 +2245,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
> if (time)
> timeout = time;
> }
> - mutex_unlock(&ldata->atomic_read_lock);
> remove_wait_queue(&tty->read_wait, &wait);
>
> if (!waitqueue_active(&tty->read_wait))
> @@ -2260,6 +2256,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
>
> n_tty_set_room(tty);
> up_read(&tty->termios_rwsem);
> + mutex_unlock(&ldata->atomic_read_lock);
> return retval;
> }
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH next] n_tty: change lock ordering in n_tty_read() (v2)
2013-08-01 20:01 ` Peter Hurley
@ 2013-08-01 20:34 ` Sergey Senozhatsky
0 siblings, 0 replies; 5+ messages in thread
From: Sergey Senozhatsky @ 2013-08-01 20:34 UTC (permalink / raw)
To: Peter Hurley; +Cc: Jiri Slaby, Greg Kroah-Hartman, linux-kernel
On (08/01/13 16:01), Peter Hurley wrote:
> On 08/01/2013 03:46 PM, Sergey Senozhatsky wrote:
> >Acquire atomic_read_lock before termios_rwsem rw-lock in n_tty_read().
> >termios_rwsem is getting released and re-acquired, while remaining
> >readers are blocked on atomic_read_lock mutex, in case when process
> >must wait for input_available_p(), making lockdep unhappy:
>
> Thanks for the report. This is a regression in lockdep.
Thanks for the update. I was thinkg about that. There is no read/write
termios_rwsem cross-locking on read path with atomic_read_lock being involved,
but the thing that stopped me is that if lockdep has a regression then I would
probably see warnings from different places, not just tty.
Feel free to ask for any help or testing (if need).
thanks,
-ss
> >[ 463.542463] ======================================================
> >[ 463.542464] [ INFO: possible circular locking dependency detected ]
> >[ 463.542466] 3.11.0-rc3-next-20130801-dbg-dirty #5 Not tainted
> >[ 463.542467] -------------------------------------------------------
> >[ 463.542469] agetty/2075 is trying to acquire lock:
> >[ 463.542484] (&tty->termios_rwsem){++++..}, at: [<ffffffff81398660>] n_tty_read+0x460/0xab0
> >[ 463.542484]
> >but task is already holding lock:
> >[ 463.542496] (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff81398959>] n_tty_read+0x759/0xab0
> >[ 463.542497]
> >which lock already depends on the new lock.
> >
> >[ 463.542497]
> >the existing dependency chain (in reverse order) is:
> >[ 463.542502]
> >-> #1 (&ldata->atomic_read_lock){+.+...}:
> >[ 463.542508] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
> >[ 463.542514] [<ffffffff8160a787>] mutex_lock_interruptible_nested+0x77/0x4e0
> >[ 463.542518] [<ffffffff81398959>] n_tty_read+0x759/0xab0
> >[ 463.542524] [<ffffffff81393e8d>] tty_read+0x8d/0x100
> >[ 463.542528] [<ffffffff8117987a>] vfs_read+0x9a/0x170
> >[ 463.542531] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
> >[ 463.542536] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
> >[ 463.542540]
> >-> #0 (&tty->termios_rwsem){++++..}:
> >[ 463.542543] [<ffffffff810b84e6>] __lock_acquire+0x1756/0x1d20
> >[ 463.542547] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
> >[ 463.542550] [<ffffffff8160c1b7>] down_read+0x47/0x60
> >[ 463.542554] [<ffffffff81398660>] n_tty_read+0x460/0xab0
> >[ 463.542557] [<ffffffff81393e8d>] tty_read+0x8d/0x100
> >[ 463.542560] [<ffffffff8117987a>] vfs_read+0x9a/0x170
> >[ 463.542563] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
> >[ 463.542566] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
> >[ 463.542567]
> >other info that might help us debug this:
> >
> >[ 463.542569] Possible unsafe locking scenario:
> >
> >[ 463.542570] CPU0 CPU1
> >[ 463.542570] ---- ----
> >[ 463.542573] lock(&ldata->atomic_read_lock);
> >[ 463.542575] lock(&tty->termios_rwsem);
> >[ 463.542577] lock(&ldata->atomic_read_lock);
> >[ 463.542579] lock(&tty->termios_rwsem);
> >[ 463.542580]
> > *** DEADLOCK ***
>
> False positive.
>
> This deadlock is not possible because CPU1 only holds a read lock
> which cannot prevent CPU0 from obtaining a read lock on the same
> read/write semaphore.
>
> I'll be only too happy to track down where the regression was
> introduced as soon as I fix an actual lock order problem in
> the nouveau driver which disables lockdep :)
>
> Regards,
> Peter Hurley
>
> >[ 463.542582] 2 locks held by agetty/2075:
> >[ 463.542590] #0: (&tty->ldisc_sem){++++++}, at: [<ffffffff8139c130>] tty_ldisc_ref_wait+0x20/0x50
> >[ 463.542597] #1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff81398959>] n_tty_read+0x759/0xab0
> >[ 463.542598]
> >stack backtrace:
> >[ 463.542602] CPU: 0 PID: 2075 Comm: agetty Not tainted 3.11.0-rc3-next-20130801-dbg-dirty #5
> >[ 463.542603] Hardware name: Acer Aspire 5741G /Aspire 5741G , BIOS V1.20 02/08/2011
> >[ 463.542609] ffffffff82236010 ffff88009af67c28 ffffffff8160630a ffffffff82236010
> >[ 463.542613] ffff88009af67c68 ffffffff816031b7 ffff880151ac0000 ffff880151ac0790
> >[ 463.542617] 000000073469e1bd ffff880151ac0768 ffff880151ac0790 ffff880151ac0000
> >[ 463.542618] Call Trace:
> >[ 463.542625] [<ffffffff8160630a>] dump_stack+0x4e/0x82
> >[ 463.542630] [<ffffffff816031b7>] print_circular_bug+0x2b6/0x2c5
> >[ 463.542634] [<ffffffff810b84e6>] __lock_acquire+0x1756/0x1d20
> >[ 463.542638] [<ffffffff810b9394>] lock_acquire+0xa4/0x200
> >[ 463.542642] [<ffffffff81398660>] ? n_tty_read+0x460/0xab0
> >[ 463.542645] [<ffffffff8160c1b7>] down_read+0x47/0x60
> >[ 463.542649] [<ffffffff81398660>] ? n_tty_read+0x460/0xab0
> >[ 463.542653] [<ffffffff81398660>] n_tty_read+0x460/0xab0
> >[ 463.542661] [<ffffffff81085020>] ? wake_up_process+0x40/0x40
> >[ 463.542665] [<ffffffff81393e8d>] tty_read+0x8d/0x100
> >[ 463.542668] [<ffffffff8117987a>] vfs_read+0x9a/0x170
> >[ 463.542671] [<ffffffff8117a05c>] SyS_read+0x4c/0xa0
> >[ 463.542674] [<ffffffff81610c7e>] tracesys+0xd0/0xd5
> >
> >v2: correct subject typo.
> >
> >Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> >
> >---
> >
> > drivers/tty/n_tty.c | 15 ++++++---------
> > 1 file changed, 6 insertions(+), 9 deletions(-)
> >
> >diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> >index dd8ae0c..03bd6e8 100644
> >--- a/drivers/tty/n_tty.c
> >+++ b/drivers/tty/n_tty.c
> >@@ -2122,8 +2122,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
> > if (c < 0)
> > return c;
> >
> >- down_read(&tty->termios_rwsem);
> >-
> > minimum = time = 0;
> > timeout = MAX_SCHEDULE_TIMEOUT;
> > if (!ldata->icanon) {
> >@@ -2145,16 +2143,15 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
> > * Internal serialization of reads.
> > */
> > if (file->f_flags & O_NONBLOCK) {
> >- if (!mutex_trylock(&ldata->atomic_read_lock)) {
> >- up_read(&tty->termios_rwsem);
> >+ if (!mutex_trylock(&ldata->atomic_read_lock))
> > return -EAGAIN;
> >- }
> > } else {
> >- if (mutex_lock_interruptible(&ldata->atomic_read_lock)) {
> >- up_read(&tty->termios_rwsem);
> >+ if (mutex_lock_interruptible(&ldata->atomic_read_lock))
> > return -ERESTARTSYS;
> >- }
> > }
> >+
> >+ down_read(&tty->termios_rwsem);
> >+
> > packet = tty->packet;
> >
> > add_wait_queue(&tty->read_wait, &wait);
> >@@ -2248,7 +2245,6 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
> > if (time)
> > timeout = time;
> > }
> >- mutex_unlock(&ldata->atomic_read_lock);
> > remove_wait_queue(&tty->read_wait, &wait);
> >
> > if (!waitqueue_active(&tty->read_wait))
> >@@ -2260,6 +2256,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
> >
> > n_tty_set_room(tty);
> > up_read(&tty->termios_rwsem);
> >+ mutex_unlock(&ldata->atomic_read_lock);
> > return retval;
> > }
> >
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-08-01 20:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-01 19:31 [PATCH next] n_tty: change lock oreding in n_tty_read() Sergey Senozhatsky
2013-08-01 19:35 ` Sergey Senozhatsky
2013-08-01 19:46 ` [PATCH next] n_tty: change lock ordering in n_tty_read() (v2) Sergey Senozhatsky
2013-08-01 20:01 ` Peter Hurley
2013-08-01 20:34 ` Sergey Senozhatsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox