n_tty: release atomic_read_lock before calling schedule_timeout()
Subsystems (2)
PATCH 1/1 n_tty: release atomic_read_lock before calling schedule_timeout()
ldata->atomic_read_lock should be released before scheduling as well as tty->termios_rwsem, otherwise there is a potential deadlock detected by lockdep
Introduced in "n_tty: Access termios values safely" (9356b535fcb71db494fc434acceb79f56d15bda2 in linux-next.git)
[ 16.822058] ======================================================
[ 16.822058] [ INFO: possible circular locking dependency detected ]
[ 16.822058] 3.11.0-rc3-next-20130730+ #140 Tainted: G W
[ 16.822058] -------------------------------------------------------
[ 16.822058] bash/1198 is trying to acquire lock:
[ 16.822058] (&tty->termios_rwsem){++++..}, at: [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660
[ 16.822058]
[ 16.822058] but task is already holding lock:
[ 16.822058] (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660
[ 16.822058]
[ 16.822058] which lock already depends on the new lock.
[ 16.822058]
[ 16.822058]
[ 16.822058] the existing dependency chain (in reverse order) is:
[ 16.822058]
-> #1 (&ldata->atomic_read_lock){+.+...}:
[ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850
[ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0
[ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0
[ 16.822058] [<ffffffff81d34b9c>] mutex_lock_interruptible_nested+0x7c/0x540
[ 16.822058] [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660
[ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0
[ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130
[ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0
[ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b
[ 16.822058]
-> #0 (&tty->termios_rwsem){++++..}:
[ 16.822058] [<ffffffff8111064f>] check_prev_add+0x14f/0x590
[ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850
[ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0
[ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0
[ 16.822058] [<ffffffff81d372c1>] down_read+0x51/0xa0
[ 16.822058] [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660
[ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0
[ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130
[ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0
[ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b
[ 16.822058]
[ 16.822058] other info that might help us debug this:
[ 16.822058]
[ 16.822058] Possible unsafe locking scenario:
[ 16.822058]
[ 16.822058] CPU0 CPU1
[ 16.822058] ---- ----
[ 16.822058] lock(&ldata->atomic_read_lock);
[ 16.822058] lock(&tty->termios_rwsem);
[ 16.822058] lock(&ldata->atomic_read_lock);
[ 16.822058] lock(&tty->termios_rwsem);
[ 16.822058]
[ 16.822058] *** DEADLOCK ***
[ 16.822058]
[ 16.822058] 2 locks held by bash/1198:
[ 16.822058] #0: (&tty->ldisc_sem){.+.+.+}, at: [<ffffffff816ade04>] tty_ldisc_ref_wait+0x24/0x60
[ 16.822058] #1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660
[ 16.822058]
[ 16.822058] stack backtrace:
[ 16.822058] CPU: 1 PID: 1198 Comm: bash Tainted: G W 3.11.0-rc3-next-20130730+ #140
[ 16.822058] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[ 16.822058] 0000000000000000 ffff880019acdb28 ffffffff81d34074 0000000000000002
[ 16.822058] 0000000000000000 ffff880019acdb78 ffffffff8110ed75 ffff880019acdb98
[ 16.822058] ffff880019fd0000 ffff880019acdb78 ffff880019fd0638 ffff880019fd0670
[ 16.822058] Call Trace:
[ 16.822058] [<ffffffff81d34074>] dump_stack+0x59/0x7d
[ 16.822058] [<ffffffff8110ed75>] print_circular_bug+0x105/0x120
[ 16.822058] [<ffffffff8111064f>] check_prev_add+0x14f/0x590
[ 16.822058] [<ffffffff81d3ab5f>] ? _raw_spin_unlock_irq+0x4f/0x70
[ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850
[ 16.822058] [<ffffffff8110ae0f>] ? trace_hardirqs_off_caller+0x1f/0x190
[ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0
[ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0
[ 16.822058] [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660
[ 16.822058] [<ffffffff81d372c1>] down_read+0x51/0xa0
[ 16.822058] [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660
[ 16.822058] [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660
[ 16.822058] [<ffffffff810e4130>] ? try_to_wake_up+0x210/0x210
[ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0
[ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130
[ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0
[ 16.822058] [<ffffffff815e24ee>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b--- drivers/tty/n_tty.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/tty/n_tty.c +12 -0
@@ -2203,11 +2203,23 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,break ; }n_tty_set_room (tty); +mutex_unlock (&ldata->atomic_read_lock);up_read (&tty->termios_rwsem); timeout =schedule_timeout (timeout);down_read (&tty->termios_rwsem); +if (file->f_flags &O_NONBLOCK ) { +if (!mutex_trylock (&ldata->atomic_read_lock)) { + retval = -EAGAIN ; +break ; + } + }else { +if (mutex_lock_interruptible (&ldata->atomic_read_lock)) { + retval = -ERESTARTSYS ; +break ; + } + }continue ; }__set_current_state (TASK_RUNNING );
Signature · 1 line
1.7.0.4
Quoted 2 lines · Artem Savkov
ldata->atomic_read_lockshould be released before scheduling as well astty->termios_rwsem, otherwise there is a potential deadlock detected by lockdep
False positive.
Quoted 45 lines
Introduced in "
n_tty: Access termios values safely" (9356b535fcb71db494fc434acceb79f56d15bda2in linux-next.git)[ 16.822058] ====================================================== [ 16.822058] [ INFO: possible circular locking dependency detected ] [ 16.822058] 3.11.0-rc3-next-20130730+ #140 Tainted: G W [ 16.822058] ------------------------------------------------------- [ 16.822058] bash/1198 is trying to acquire lock: [ 16.822058] (&tty->termios_rwsem){++++..}, at: [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [ 16.822058] [ 16.822058] but task is already holding lock: [ 16.822058] (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [ 16.822058] [ 16.822058] which lock already depends on the new lock. [ 16.822058] [ 16.822058] [ 16.822058] the existing dependency chain (in reverse order) is: [ 16.822058] -> #1 (&ldata->atomic_read_lock){+.+...}: [ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850 [ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [ 16.822058] [<ffffffff81d34b9c>] mutex_lock_interruptible_nested+0x7c/0x540 [ 16.822058] [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0 [ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b [ 16.822058] -> #0 (&tty->termios_rwsem){++++..}: [ 16.822058] [<ffffffff8111064f>] check_prev_add+0x14f/0x590 [ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850 [ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [ 16.822058] [<ffffffff81d372c1>] down_read+0x51/0xa0 [ 16.822058] [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0 [ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b [ 16.822058] [ 16.822058] other info that might help us debug this: [ 16.822058] [ 16.822058] Possible unsafe locking scenario: [ 16.822058]
Quoted 8 lines
[ 16.822058] CPU0 CPU1 [ 16.822058] ---- ---- [ 16.822058] lock(&ldata->atomic_read_lock); [ 16.822058] lock(&tty->termios_rwsem); [ 16.822058] lock(&ldata->atomic_read_lock); [ 16.822058] lock(&tty->termios_rwsem); [ 16.822058] [ 16.822058] *** DEADLOCK ***
This situation is not possible since termios_rwsem is a read/write semaphore; CPU1 cannot prevent CPU0 from obtaining a read lock on termios_rwsem.
This looks like a regression caused by:
commit a51805efae5dda0da66f79268ffcf0715f9dbea4
Author: Michel Lespinasse <[email protected]> Date: Mon Jul 8 14:23:49 2013 -0700
lockdep: Introduce lock_acquire_exclusive()/shared() helper macros
In lockdep.h, the spinlock/mutex/rwsem/rwlock/lock_map acquire macros have different definitions based on the value of CONFIG_PROVE_LOCKING. We have separate ifdefs for each of these definitions, which seems redundant.
Introduce lock_acquire_{exclusive,shared,shared_recursive} helpers which will have different definitions based on CONFIG_PROVE_LOCKING. Then all other helper macros can be defined based on the above ones, which reduces the amount of ifdefined code.
Quoted 65 lines
[ 16.822058] [ 16.822058] 2 locks held by bash/1198: [ 16.822058] #0: (&tty->ldisc_sem){.+.+.+}, at: [<ffffffff816ade04>] tty_ldisc_ref_wait+0x24/0x60 [ 16.822058] #1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [ 16.822058] [ 16.822058] stack backtrace: [ 16.822058] CPU: 1 PID: 1198 Comm: bash Tainted: G W 3.11.0-rc3-next-20130730+ #140 [ 16.822058] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007 [ 16.822058] 0000000000000000 ffff880019acdb28 ffffffff81d34074 0000000000000002 [ 16.822058] 0000000000000000 ffff880019acdb78 ffffffff8110ed75 ffff880019acdb98 [ 16.822058] ffff880019fd0000 ffff880019acdb78 ffff880019fd0638 ffff880019fd0670 [ 16.822058] Call Trace: [ 16.822058] [<ffffffff81d34074>] dump_stack+0x59/0x7d [ 16.822058] [<ffffffff8110ed75>] print_circular_bug+0x105/0x120 [ 16.822058] [<ffffffff8111064f>] check_prev_add+0x14f/0x590 [ 16.822058] [<ffffffff81d3ab5f>] ? _raw_spin_unlock_irq+0x4f/0x70 [ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850 [ 16.822058] [<ffffffff8110ae0f>] ? trace_hardirqs_off_caller+0x1f/0x190 [ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [ 16.822058] [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [ 16.822058] [<ffffffff81d372c1>] down_read+0x51/0xa0 [ 16.822058] [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [ 16.822058] [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [ 16.822058] [<ffffffff810e4130>] ? try_to_wake_up+0x210/0x210 [ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0 [ 16.822058] [<ffffffff815e24ee>] ? trace_hardirqs_on_thunk+0x3a/0x3f [ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1bSigned-off-by Artem Savkov <[email protected]>--- drivers/tty/n_tty.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)drivers/tty/n_tty.c +12
-0@@ -2203,11 +2203,23 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,break ; }n_tty_set_room (tty); +mutex_unlock (&ldata->atomic_read_lock);up_read (&tty->termios_rwsem); timeout =schedule_timeout (timeout);down_read (&tty->termios_rwsem); +if (file->f_flags &O_NONBLOCK ) { +if (!mutex_trylock (&ldata->atomic_read_lock)) { + retval = -EAGAIN ; +break ; + } + }else { +if (mutex_lock_interruptible (&ldata->atomic_read_lock)) { + retval = -ERESTARTSYS ; +break ; + } + }continue ; }__set_current_state (TASK_RUNNING );
Quoted 60 lines · Peter Hurley
Quoted 2 lines · Artem Savkov
ldata->atomic_read_lockshould be released before scheduling as well astty->termios_rwsem, otherwise there is a potential deadlock detected by lockdepFalse positive.
Quoted 45 lines
Introduced in "
n_tty: Access termios values safely" (9356b535fcb71db494fc434acceb79f56d15bda2in linux-next.git)[ 16.822058] ====================================================== [ 16.822058] [ INFO: possible circular locking dependency detected ] [ 16.822058] 3.11.0-rc3-next-20130730+ #140 Tainted: G W [ 16.822058] ------------------------------------------------------- [ 16.822058] bash/1198 is trying to acquire lock: [ 16.822058] (&tty->termios_rwsem){++++..}, at: [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [ 16.822058] [ 16.822058] but task is already holding lock: [ 16.822058] (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [ 16.822058] [ 16.822058] which lock already depends on the new lock. [ 16.822058] [ 16.822058] [ 16.822058] the existing dependency chain (in reverse order) is: [ 16.822058] -> #1 (&ldata->atomic_read_lock){+.+...}: [ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850 [ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [ 16.822058] [<ffffffff81d34b9c>] mutex_lock_interruptible_nested+0x7c/0x540 [ 16.822058] [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0 [ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b [ 16.822058] -> #0 (&tty->termios_rwsem){++++..}: [ 16.822058] [<ffffffff8111064f>] check_prev_add+0x14f/0x590 [ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850 [ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [ 16.822058] [<ffffffff81d372c1>] down_read+0x51/0xa0 [ 16.822058] [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0 [ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b [ 16.822058] [ 16.822058] other info that might help us debug this: [ 16.822058] [ 16.822058] Possible unsafe locking scenario: [ 16.822058]Quoted 8 lines
[ 16.822058] CPU0 CPU1 [ 16.822058] ---- ---- [ 16.822058] lock(&ldata->atomic_read_lock); [ 16.822058] lock(&tty->termios_rwsem); [ 16.822058] lock(&ldata->atomic_read_lock); [ 16.822058] lock(&tty->termios_rwsem); [ 16.822058] [ 16.822058] *** DEADLOCK ***
Quoted 2 lines
This situation is not possible since
termios_rwsemis a read/write semaphore; CPU1 cannot prevent CPU0 from obtaining a read lock ontermios_rwsem.
Oops, yes, sorry.
Quoted 7 lines
This looks like a regression caused by:
commit
a51805efae5dda0da66f79268ffcf0715f9dbea4Author: Michel Lespinasse <[email protected]> Date: Mon Jul 8 14:23:49 2013 -0700lockdep: Introduce lock_acquire_exclusive()/shared() helper macros
Doesn't seem to be this commit. I see nothing wrong here and just to be sure I've checked the kernel with this commit reverted. The issue is still there.
Quoted 89 lines
In
lockdep.h, the spinlock/mutex/rwsem/rwlock/lock_map acquire macros have different definitions based on the value ofCONFIG_PROVE_LOCKING. We have separate ifdefs for each of these definitions, which seems redundant.Introduce lock_acquire_{exclusive,shared,shared_recursive} helpers which will have different definitions based on
CONFIG_PROVE_LOCKING. Then all other helper macros can be defined based on the above ones, which reduces the amount of ifdefined code.Signed-off-by Michel Lespinasse <[email protected]>Signed-off-by Andrew Morton <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>Cc Oleg Nesterov <[email protected]>Cc Lai Jiangshan <[email protected]>Cc "Srivatsa S. Bhat" <[email protected]>Cc Rusty Russell <[email protected]>Cc Andi Kleen <[email protected]>Cc "Paul E. McKenney" <[email protected]>Cc Steven Rostedt <[email protected]>Signed-off-by Ingo Molnar <[email protected]>Quoted 65 lines
[ 16.822058] [ 16.822058] 2 locks held by bash/1198: [ 16.822058] #0: (&tty->ldisc_sem){.+.+.+}, at: [<ffffffff816ade04>] tty_ldisc_ref_wait+0x24/0x60 [ 16.822058] #1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [ 16.822058] [ 16.822058] stack backtrace: [ 16.822058] CPU: 1 PID: 1198 Comm: bash Tainted: G W 3.11.0-rc3-next-20130730+ #140 [ 16.822058] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007 [ 16.822058] 0000000000000000 ffff880019acdb28 ffffffff81d34074 0000000000000002 [ 16.822058] 0000000000000000 ffff880019acdb78 ffffffff8110ed75 ffff880019acdb98 [ 16.822058] ffff880019fd0000 ffff880019acdb78 ffff880019fd0638 ffff880019fd0670 [ 16.822058] Call Trace: [ 16.822058] [<ffffffff81d34074>] dump_stack+0x59/0x7d [ 16.822058] [<ffffffff8110ed75>] print_circular_bug+0x105/0x120 [ 16.822058] [<ffffffff8111064f>] check_prev_add+0x14f/0x590 [ 16.822058] [<ffffffff81d3ab5f>] ? _raw_spin_unlock_irq+0x4f/0x70 [ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850 [ 16.822058] [<ffffffff8110ae0f>] ? trace_hardirqs_off_caller+0x1f/0x190 [ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [ 16.822058] [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [ 16.822058] [<ffffffff81d372c1>] down_read+0x51/0xa0 [ 16.822058] [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [ 16.822058] [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [ 16.822058] [<ffffffff810e4130>] ? try_to_wake_up+0x210/0x210 [ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0 [ 16.822058] [<ffffffff815e24ee>] ? trace_hardirqs_on_thunk+0x3a/0x3f [ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1bSigned-off-by Artem Savkov <[email protected]>--- drivers/tty/n_tty.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)drivers/tty/n_tty.c +12
-0@@ -2203,11 +2203,23 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,break ; }n_tty_set_room (tty); +mutex_unlock (&ldata->atomic_read_lock);up_read (&tty->termios_rwsem); timeout =schedule_timeout (timeout);down_read (&tty->termios_rwsem); +if (file->f_flags &O_NONBLOCK ) { +if (!mutex_trylock (&ldata->atomic_read_lock)) { + retval = -EAGAIN ; +break ; + } + }else { +if (mutex_lock_interruptible (&ldata->atomic_read_lock)) { + retval = -ERESTARTSYS ; +break ; + } + }continue ; }__set_current_state (TASK_RUNNING );
Signature · 2 lines
Regards,
ArtemQuoted 67 lines · Artem Savkov
Quoted 60 lines · Peter Hurley
Quoted 2 lines · Artem Savkov
ldata->atomic_read_lockshould be released before scheduling as well astty->termios_rwsem, otherwise there is a potential deadlock detected by lockdepFalse positive.
Quoted 45 lines
Introduced in "
n_tty: Access termios values safely" (9356b535fcb71db494fc434acceb79f56d15bda2in linux-next.git)[ 16.822058] ====================================================== [ 16.822058] [ INFO: possible circular locking dependency detected ] [ 16.822058] 3.11.0-rc3-next-20130730+ #140 Tainted: G W [ 16.822058] ------------------------------------------------------- [ 16.822058] bash/1198 is trying to acquire lock: [ 16.822058] (&tty->termios_rwsem){++++..}, at: [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [ 16.822058] [ 16.822058] but task is already holding lock: [ 16.822058] (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [ 16.822058] [ 16.822058] which lock already depends on the new lock. [ 16.822058] [ 16.822058] [ 16.822058] the existing dependency chain (in reverse order) is: [ 16.822058] -> #1 (&ldata->atomic_read_lock){+.+...}: [ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850 [ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [ 16.822058] [<ffffffff81d34b9c>] mutex_lock_interruptible_nested+0x7c/0x540 [ 16.822058] [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0 [ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b [ 16.822058] -> #0 (&tty->termios_rwsem){++++..}: [ 16.822058] [<ffffffff8111064f>] check_prev_add+0x14f/0x590 [ 16.822058] [<ffffffff811111cc>] validate_chain+0x73c/0x850 [ 16.822058] [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [ 16.822058] [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [ 16.822058] [<ffffffff81d372c1>] down_read+0x51/0xa0 [ 16.822058] [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [ 16.822058] [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [ 16.822058] [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [ 16.822058] [<ffffffff811f2702>] SyS_read+0x62/0xa0 [ 16.822058] [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b [ 16.822058] [ 16.822058] other info that might help us debug this: [ 16.822058] [ 16.822058] Possible unsafe locking scenario: [ 16.822058]Quoted 8 lines
[ 16.822058] CPU0 CPU1 [ 16.822058] ---- ---- [ 16.822058] lock(&ldata->atomic_read_lock); [ 16.822058] lock(&tty->termios_rwsem); [ 16.822058] lock(&ldata->atomic_read_lock); [ 16.822058] lock(&tty->termios_rwsem); [ 16.822058] [ 16.822058] *** DEADLOCK ***Quoted 2 lines
This situation is not possible since
termios_rwsemis a read/write semaphore; CPU1 cannot prevent CPU0 from obtaining a read lock ontermios_rwsem.Oops, yes, sorry.
Quoted 2 lines
This looks like a regression caused by:
Quoted 8 lines
Quoted 5 lines
commit
a51805efae5dda0da66f79268ffcf0715f9dbea4Author: Michel Lespinasse <[email protected]> Date: Mon Jul 8 14:23:49 2013 -0700lockdep: Introduce lock_acquire_exclusive()/shared() helper macros
Doesn't seem to be this commit. I see nothing wrong here and just to be sure I've checked the kernel with this commit reverted. The issue is still there.
Yes, you're right. Apologies to Michel for the too-hasty blame.
Thanks for the report anyway. I'll track down the lockdep regression as soon as I fix a real deadlock in the nouveau driver that disables lockdep.
Regards,
Peter Hurley
PATCH 1/1 n_tty: Fix termios_rwsem lockdep false positive
In reply to Artem SavkovLockdep reports a circular lock dependency between atomic_read_lock and termios_rwsem [1]. However, a lock order deadlock is not possible since CPU1 only holds a read lock which cannot prevent CPU0 from also acquiring a read lock on the same r/w semaphore.
Unfortunately, lockdep cannot currently distinguish whether the locks are read or write for any particular lock graph, merely that the locks _were_ previously read and/or write.
Until lockdep is fixed, re-order atomic_read_lock so termios_rwsem can be dropped and reacquired without triggering lockdep.
[1] Initial lockdep report from Artem Savkov <[email protected]>
======================================================
[ INFO: possible circular locking dependency detected ]
3.11.0-rc3-next-20130730+ #140 Tainted: G W
-------------------------------------------------------
bash/1198 is trying to acquire lock:
(&tty->termios_rwsem){++++..}, at: [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660
but task is already holding lock:
(&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&ldata->atomic_read_lock){+.+...}:
[<ffffffff811111cc>] validate_chain+0x73c/0x850
[<ffffffff811117e0>] __lock_acquire+0x500/0x5d0
[<ffffffff81111a29>] lock_acquire+0x179/0x1d0
[<ffffffff81d34b9c>] mutex_lock_interruptible_nested+0x7c/0x540
[<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660
[<ffffffff816a3bb6>] tty_read+0x86/0xf0
[<ffffffff811f21d3>] vfs_read+0xc3/0x130
[<ffffffff811f2702>] SyS_read+0x62/0xa0
[<ffffffff81d45259>] system_call_fastpath+0x16/0x1b-> #0 (&tty->termios_rwsem){++++..}:
[<ffffffff8111064f>] check_prev_add+0x14f/0x590
[<ffffffff811111cc>] validate_chain+0x73c/0x850
[<ffffffff811117e0>] __lock_acquire+0x500/0x5d0
[<ffffffff81111a29>] lock_acquire+0x179/0x1d0
[<ffffffff81d372c1>] down_read+0x51/0xa0
[<ffffffff816aa3bb>] n_tty_read+0x49b/0x660
[<ffffffff816a3bb6>] tty_read+0x86/0xf0
[<ffffffff811f21d3>] vfs_read+0xc3/0x130
[<ffffffff811f2702>] SyS_read+0x62/0xa0
[<ffffffff81d45259>] system_call_fastpath+0x16/0x1bother info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&ldata->atomic_read_lock);
lock(&tty->termios_rwsem);
lock(&ldata->atomic_read_lock); lock(&tty->termios_rwsem);
*** DEADLOCK ***
2 locks held by bash/1198:
#0: (&tty->ldisc_sem){.+.+.+}, at: [<ffffffff816ade04>] tty_ldisc_ref_wait+0x24/0x60 #1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660
stack backtrace: CPU: 1 PID: 1198 Comm: bash Tainted: G W 3.11.0-rc3-next-20130730+ #140 Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007 0000000000000000 ffff880019acdb28 ffffffff81d34074 0000000000000002 0000000000000000 ffff880019acdb78 ffffffff8110ed75 ffff880019acdb98 ffff880019fd0000 ffff880019acdb78 ffff880019fd0638 ffff880019fd0670 Call Trace: [<ffffffff81d34074>] dump_stack+0x59/0x7d [<ffffffff8110ed75>] print_circular_bug+0x105/0x120 [<ffffffff8111064f>] check_prev_add+0x14f/0x590 [<ffffffff81d3ab5f>] ? _raw_spin_unlock_irq+0x4f/0x70 [<ffffffff811111cc>] validate_chain+0x73c/0x850 [<ffffffff8110ae0f>] ? trace_hardirqs_off_caller+0x1f/0x190 [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [<ffffffff81d372c1>] down_read+0x51/0xa0 [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [<ffffffff810e4130>] ? try_to_wake_up+0x210/0x210 [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [<ffffffff811f2702>] SyS_read+0x62/0xa0 [<ffffffff815e24ee>] ? trace_hardirqs_on_thunk+0x3a/0x3f [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b --- drivers/tty/n_tty.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
drivers/tty/n_tty.c +11 -14
@@ -2122,6 +2122,17 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,if (c <0 )return c; +/* +* Internal serialization of reads. +*/ +if (file->f_flags &O_NONBLOCK ) { +if (!mutex_trylock (&ldata->atomic_read_lock)) +return -EAGAIN ; + }else { +if (mutex_lock_interruptible (&ldata->atomic_read_lock)) +return -ERESTARTSYS ; + } +down_read (&tty->termios_rwsem); minimum = time =0 ;
@@ -2141,20 +2152,6 @@ 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); -return -EAGAIN ; - } - }else { -if (mutex_lock_interruptible (&ldata->atomic_read_lock)) { -up_read (&tty->termios_rwsem); -return -ERESTARTSYS ; - } - } packet = tty->packet;add_wait_queue (&tty->read_wait, &wait);
Signature · 1 line
1.8.1.2
Re: [PATCH tty-next] n_tty: Fix termios_rwsem lockdep false positive
Hi Peter,
Quoted 6 lines · Peter Hurley
Lockdep reports a circular lock dependency between
atomic_read_lockandtermios_rwsem[1]. However, a lock order deadlock is not possible since CPU1 only holds a read lock which cannot prevent CPU0 from also acquiring a read lock on the same r/w semaphore.
Quoted 7 lines
Unfortunately, lockdep cannot currently distinguish whether the locks are read or write for any particular lock graph, merely that the locks _were_ previously read and/or write.
Until lockdep is fixed, re-order
atomic_read_locksotermios_rwsemcan be dropped and reacquired without triggering lockdep.
Works fine, thanks.
Quoted 135 lines
Reported-by Artem Savkov <[email protected]>Reported-by Sergey Senozhatsky <[email protected]>Signed-off-by Peter Hurley <[email protected]>[1] Initial lockdep report from Artem Savkov <[email protected]>
======================================================
[ INFO: possible circular locking dependency detected ]
3.11.0-rc3-next-20130730+ #140 Tainted: G W
-------------------------------------------------------
bash/1198 is trying to acquire lock:
(&tty->termios_rwsem){++++..}, at: [<ffffffff816aa3bb>]n_tty_read+0x49b/0x660but task is already holding lock:
(&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>]n_tty_read+0x1d0/0x660which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&ldata->atomic_read_lock){+.+...}:
[<ffffffff811111cc>] validate_chain+0x73c/0x850 [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [<ffffffff81d34b9c>] mutex_lock_interruptible_nested+0x7c/0x540 [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [<ffffffff811f2702>] SyS_read+0x62/0xa0 [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b-> #0 (&tty->termios_rwsem){++++..}:
[<ffffffff8111064f>] check_prev_add+0x14f/0x590 [<ffffffff811111cc>] validate_chain+0x73c/0x850 [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [<ffffffff81d372c1>] down_read+0x51/0xa0 [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [<ffffffff811f2702>] SyS_read+0x62/0xa0 [<ffffffff81d45259>] system_call_fastpath+0x16/0x1bother info that might help us debug this:
Possible unsafe locking scenario:CPU0 CPU1 ---- ---- lock(&ldata->atomic_read_lock); lock(&tty->termios_rwsem); lock(&ldata->atomic_read_lock);
lock(&tty->termios_rwsem);*** DEADLOCK ***
2 locks held by bash/1198:
#0: (&tty->ldisc_sem){.+.+.+}, at: [<ffffffff816ade04>]tty_ldisc_ref_wait+0x24/0x60#1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>]n_tty_read+0x1d0/0x660stack backtrace: CPU: 1 PID: 1198 Comm: bash Tainted: G W 3.11.0-rc3-next-20130730+ #140 Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007 0000000000000000 ffff880019acdb28 ffffffff81d34074 0000000000000002 0000000000000000 ffff880019acdb78 ffffffff8110ed75 ffff880019acdb98 ffff880019fd0000 ffff880019acdb78 ffff880019fd0638 ffff880019fd0670 Call Trace: [<ffffffff81d34074>] dump_stack+0x59/0x7d [<ffffffff8110ed75>] print_circular_bug+0x105/0x120 [<ffffffff8111064f>] check_prev_add+0x14f/0x590 [<ffffffff81d3ab5f>] ? _raw_spin_unlock_irq+0x4f/0x70 [<ffffffff811111cc>] validate_chain+0x73c/0x850 [<ffffffff8110ae0f>] ? trace_hardirqs_off_caller+0x1f/0x190 [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [<ffffffff81d372c1>] down_read+0x51/0xa0 [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [<ffffffff810e4130>] ? try_to_wake_up+0x210/0x210 [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [<ffffffff811f2702>] SyS_read+0x62/0xa0 [<ffffffff815e24ee>] ? trace_hardirqs_on_thunk+0x3a/0x3f [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b --- drivers/tty/n_tty.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)drivers/tty/n_tty.c +11
-14@@ -2122,6 +2122,17 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,if (c <0 )return c; +/* +* Internal serialization of reads. +*/ +if (file->f_flags &O_NONBLOCK ) { +if (!mutex_trylock (&ldata->atomic_read_lock)) +return -EAGAIN ; + }else { +if (mutex_lock_interruptible (&ldata->atomic_read_lock)) +return -ERESTARTSYS ; + } +down_read (&tty->termios_rwsem); minimum = time =0 ;@@ -2141,20 +2152,6 @@ 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); -return -EAGAIN ; - } - }else { -if (mutex_lock_interruptible (&ldata->atomic_read_lock)) { -up_read (&tty->termios_rwsem); -return -ERESTARTSYS ; - } - } packet = tty->packet;add_wait_queue (&tty->read_wait, &wait);Signature · 1 line
1.8.1.2
Signature · 2 lines
Regards,
ArtemRe: [PATCH tty-next] n_tty: Fix termios_rwsem lockdep false positive
Quoted 13 lines · Artem Savkov
Hi Peter,
Quoted 10 lines · Peter Hurley
Lockdep reports a circular lock dependency between
atomic_read_lockandtermios_rwsem[1]. However, a lock order deadlock is not possible since CPU1 only holds a read lock which cannot prevent CPU0 from also acquiring a read lock on the same r/w semaphore.Unfortunately, lockdep cannot currently distinguish whether the locks are read or write for any particular lock graph, merely that the locks _were_ previously read and/or write.
Quoted 97 lines
Quoted 3 lines
Until lockdep is fixed, re-order
atomic_read_locksotermios_rwsemcan be dropped and reacquired without triggering lockdep.Works fine, thanks.
Reported-and-tested-by Artem Savkov <[email protected]>Quoted 89 lines
Reported-by Artem Savkov <[email protected]>Reported-by Sergey Senozhatsky <[email protected]>Signed-off-by Peter Hurley <[email protected]>[1] Initial lockdep report from Artem Savkov <[email protected]>
======================================================
[ INFO: possible circular locking dependency detected ]
3.11.0-rc3-next-20130730+ #140 Tainted: G W
-------------------------------------------------------
bash/1198 is trying to acquire lock:
(&tty->termios_rwsem){++++..}, at: [<ffffffff816aa3bb>]n_tty_read+0x49b/0x660but task is already holding lock:
(&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>]n_tty_read+0x1d0/0x660which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&ldata->atomic_read_lock){+.+...}:
[<ffffffff811111cc>] validate_chain+0x73c/0x850 [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [<ffffffff81d34b9c>] mutex_lock_interruptible_nested+0x7c/0x540 [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [<ffffffff811f2702>] SyS_read+0x62/0xa0 [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b-> #0 (&tty->termios_rwsem){++++..}:
[<ffffffff8111064f>] check_prev_add+0x14f/0x590 [<ffffffff811111cc>] validate_chain+0x73c/0x850 [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [<ffffffff81d372c1>] down_read+0x51/0xa0 [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [<ffffffff811f2702>] SyS_read+0x62/0xa0 [<ffffffff81d45259>] system_call_fastpath+0x16/0x1bother info that might help us debug this:
Possible unsafe locking scenario:CPU0 CPU1 ---- ---- lock(&ldata->atomic_read_lock); lock(&tty->termios_rwsem); lock(&ldata->atomic_read_lock);
lock(&tty->termios_rwsem);*** DEADLOCK ***
2 locks held by bash/1198:
#0: (&tty->ldisc_sem){.+.+.+}, at: [<ffffffff816ade04>]tty_ldisc_ref_wait+0x24/0x60#1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>]n_tty_read+0x1d0/0x660stack backtrace: CPU: 1 PID: 1198 Comm: bash Tainted: G W 3.11.0-rc3-next-20130730+ #140 Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007 0000000000000000 ffff880019acdb28 ffffffff81d34074 0000000000000002 0000000000000000 ffff880019acdb78 ffffffff8110ed75 ffff880019acdb98 ffff880019fd0000 ffff880019acdb78 ffff880019fd0638 ffff880019fd0670 Call Trace: [<ffffffff81d34074>] dump_stack+0x59/0x7d [<ffffffff8110ed75>] print_circular_bug+0x105/0x120 [<ffffffff8111064f>] check_prev_add+0x14f/0x590 [<ffffffff81d3ab5f>] ? _raw_spin_unlock_irq+0x4f/0x70 [<ffffffff811111cc>] validate_chain+0x73c/0x850 [<ffffffff8110ae0f>] ? trace_hardirqs_off_caller+0x1f/0x190 [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [<ffffffff81d372c1>] down_read+0x51/0xa0 [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [<ffffffff810e4130>] ? try_to_wake_up+0x210/0x210 [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [<ffffffff811f2702>] SyS_read+0x62/0xa0 [<ffffffff815e24ee>] ? trace_hardirqs_on_thunk+0x3a/0x3f [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b --- drivers/tty/n_tty.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
I hate to do this, but isn't it actually my patch posted here
https://lkml.org/lkml/2013/8/1/510
which was tagged as `wrong'?
-ss
Quoted 51 lines
Quoted 46 lines
drivers/tty/n_tty.c +11
-14@@ -2122,6 +2122,17 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,if (c <0 )return c; +/* +* Internal serialization of reads. +*/ +if (file->f_flags &O_NONBLOCK ) { +if (!mutex_trylock (&ldata->atomic_read_lock)) +return -EAGAIN ; + }else { +if (mutex_lock_interruptible (&ldata->atomic_read_lock)) +return -ERESTARTSYS ; + } +down_read (&tty->termios_rwsem); minimum = time =0 ;@@ -2141,20 +2152,6 @@ 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); -return -EAGAIN ; - } - }else { -if (mutex_lock_interruptible (&ldata->atomic_read_lock)) { -up_read (&tty->termios_rwsem); -return -ERESTARTSYS ; - } - } packet = tty->packet;add_wait_queue (&tty->read_wait, &wait);Signature · 1 line
1.8.1.2Signature · 2 lines
Regards, Artem
Re: [PATCH tty-next] n_tty: Fix termios_rwsem lockdep false positive
Quoted 112 lines · Sergey Senozhatsky
Quoted 13 lines · Artem Savkov
Hi Peter,
Quoted 10 lines · Peter Hurley
Lockdep reports a circular lock dependency between
atomic_read_lockandtermios_rwsem[1]. However, a lock order deadlock is not possible since CPU1 only holds a read lock which cannot prevent CPU0 from also acquiring a read lock on the same r/w semaphore.Unfortunately, lockdep cannot currently distinguish whether the locks are read or write for any particular lock graph, merely that the locks _were_ previously read and/or write.
Quoted 97 lines
Quoted 3 lines
Until lockdep is fixed, re-order
atomic_read_locksotermios_rwsemcan be dropped and reacquired without triggering lockdep.Works fine, thanks.
Reported-and-tested-by Artem Savkov <[email protected]>Quoted 89 lines
Reported-by Artem Savkov <[email protected]>Reported-by Sergey Senozhatsky <[email protected]>Signed-off-by Peter Hurley <[email protected]>[1] Initial lockdep report from Artem Savkov <[email protected]>
======================================================
[ INFO: possible circular locking dependency detected ]
3.11.0-rc3-next-20130730+ #140 Tainted: G W
-------------------------------------------------------
bash/1198 is trying to acquire lock:(&tty->termios_rwsem){++++..}, at: [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660but task is already holding lock:
(&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&ldata->atomic_read_lock){+.+...}:
[<ffffffff811111cc>] validate_chain+0x73c/0x850 [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [<ffffffff81d34b9c>] mutex_lock_interruptible_nested+0x7c/0x540 [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660 [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [<ffffffff811f2702>] SyS_read+0x62/0xa0 [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b-> #0 (&tty->termios_rwsem){++++..}:
[<ffffffff8111064f>] check_prev_add+0x14f/0x590 [<ffffffff811111cc>] validate_chain+0x73c/0x850 [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [<ffffffff81d372c1>] down_read+0x51/0xa0 [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [<ffffffff811f2702>] SyS_read+0x62/0xa0 [<ffffffff81d45259>] system_call_fastpath+0x16/0x1bother info that might help us debug this:
Possible unsafe locking scenario:CPU0 CPU1 ---- ---- lock(&ldata->atomic_read_lock); lock(&tty->termios_rwsem); lock(&ldata->atomic_read_lock); lock(&tty->termios_rwsem);*** DEADLOCK ***2 locks held by bash/1198:
#0: (&tty->ldisc_sem){.+.+.+}, at: [<ffffffff816ade04>] tty_ldisc_ref_wait+0x24/0x60 #1: (&ldata->atomic_read_lock){+.+...}, at: [<ffffffff816aa0f0>] n_tty_read+0x1d0/0x660stack backtrace: CPU: 1 PID: 1198 Comm: bash Tainted: G W 3.11.0-rc3-next-20130730+ #140 Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007 0000000000000000 ffff880019acdb28 ffffffff81d34074 0000000000000002 0000000000000000 ffff880019acdb78 ffffffff8110ed75 ffff880019acdb98 ffff880019fd0000 ffff880019acdb78 ffff880019fd0638 ffff880019fd0670 Call Trace: [<ffffffff81d34074>] dump_stack+0x59/0x7d [<ffffffff8110ed75>] print_circular_bug+0x105/0x120 [<ffffffff8111064f>] check_prev_add+0x14f/0x590 [<ffffffff81d3ab5f>] ? _raw_spin_unlock_irq+0x4f/0x70 [<ffffffff811111cc>] validate_chain+0x73c/0x850 [<ffffffff8110ae0f>] ? trace_hardirqs_off_caller+0x1f/0x190 [<ffffffff811117e0>] __lock_acquire+0x500/0x5d0 [<ffffffff81111a29>] lock_acquire+0x179/0x1d0 [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [<ffffffff81d372c1>] down_read+0x51/0xa0 [<ffffffff816aa3bb>] ? n_tty_read+0x49b/0x660 [<ffffffff816aa3bb>] n_tty_read+0x49b/0x660 [<ffffffff810e4130>] ? try_to_wake_up+0x210/0x210 [<ffffffff816a3bb6>] tty_read+0x86/0xf0 [<ffffffff811f21d3>] vfs_read+0xc3/0x130 [<ffffffff811f2702>] SyS_read+0x62/0xa0 [<ffffffff815e24ee>] ? trace_hardirqs_on_thunk+0x3a/0x3f [<ffffffff81d45259>] system_call_fastpath+0x16/0x1b --- drivers/tty/n_tty.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
Quoted 4 lines
I hate to do this, but isn't it actually my patch posted here
https://lkml.org/lkml/2013/8/1/510which was tagged as `wrong'?
Sergey,
My apologies; I was mistaken regarding this problem being a lockdep regression (although it's still a false positive from lockdep). Once I had worked around some issues with the nouveau driver, I was able to reproduce the lockdep report on 3.10.
I included Artem's lockdep report in the changelog because I received that first, on 30 July.
My patch below is not the same as your patch of 1 Aug. This patch preserves the protected access of termios.c_cc[VMIN] and termios.c_cc[VTIME] (via the MIN_CHAR() and TIME_CHAR() macros).
If you'd prefer, I could add to changelog:
Patch based on original posted here https://lkml.org/lkml/2013/8/1/510
by Sergey Senozhatsky <[email protected]>
Regards,
Peter Hurley
Quoted 51 lines
Quoted 51 lines
Quoted 44 lines
drivers/tty/n_tty.c +11
-15@@ -2122,6 +2122,17 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,if (c <0 )return c; +/* +* Internal serialization of reads. +*/ +if (file->f_flags &O_NONBLOCK ) { +if (!mutex_trylock (&ldata->atomic_read_lock)) +return -EAGAIN ; + }else { +if (mutex_lock_interruptible (&ldata->atomic_read_lock)) +return -ERESTARTSYS ; + } +down_read (&tty->termios_rwsem); minimum = time =0 ;@@ -2141,20 +2152,6 @@ 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); -return -EAGAIN ; - } - }else { -if (mutex_lock_interruptible (&ldata->atomic_read_lock)) { -up_read (&tty->termios_rwsem); -return -ERESTARTSYS ; - } - } packet = tty->packet;add_wait_queue (&tty->read_wait, &wait); --Quoted 2 lines
1.8.1.2
--
Regards,
Artem
Re: [PATCH tty-next] n_tty: Fix termios_rwsem lockdep false positive
Quoted 8 lines · Peter Hurley
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
[..]
drivers/tty/n_tty.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)I hate to do this, but isn't it actually my patch posted here
https://lkml.org/lkml/2013/8/1/510
Quoted 9 lines
Quoted 1 line
which was tagged as `wrong'?
Sergey,
My apologies; I was mistaken regarding this problem being a lockdep regression (although it's still a false positive from lockdep). Once I had worked around some issues with the nouveau driver, I was able to reproduce the lockdep report on 3.10.
no problem.
Quoted 6 lines
I included Artem's lockdep report in the changelog because I received that first, on 30 July.
My patch below is not the same as your patch of 1 Aug. This patch preserves the protected access of termios.c_cc[VMIN] and termios.c_cc[VTIME] (via the
MIN_CHAR()andTIME_CHAR()macros).
fair enough. v3 was protecting VMIN/VTIME (my bad, I noticed this a bit later), but I didn't submit it since v2 did not get positive response.
Quoted 4 lines
If you'd prefer, I could add to changelog:
Patch based on original posted here https://lkml.org/lkml/2013/8/1/510
by Sergey Senozhatsky <[email protected]>
if you don't mind, that would be great.
thanks a lot,
-ss
Quoted 56 lines
Regards,
Peter HurleyQuoted 51 lines
Quoted 51 lines
Quoted 44 lines
drivers/tty/n_tty.c +11
-15@@ -2122,6 +2122,17 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,if (c <0 )return c; +/* +* Internal serialization of reads. +*/ +if (file->f_flags &O_NONBLOCK ) { +if (!mutex_trylock (&ldata->atomic_read_lock)) +return -EAGAIN ; + }else { +if (mutex_lock_interruptible (&ldata->atomic_read_lock)) +return -ERESTARTSYS ; + } +down_read (&tty->termios_rwsem); minimum = time =0 ;@@ -2141,20 +2152,6 @@ 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); -return -EAGAIN ; - } - }else { -if (mutex_lock_interruptible (&ldata->atomic_read_lock)) { -up_read (&tty->termios_rwsem); -return -ERESTARTSYS ; - } - } packet = tty->packet;add_wait_queue (&tty->read_wait, &wait); --Quoted 2 lines
1.8.1.2
--
Regards,
Artem
Re: [PATCH tty-next] n_tty: Fix termios_rwsem lockdep false positive
Quoted 30 lines · Sergey Senozhatsky
Quoted 8 lines · Peter Hurley
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
[..]
drivers/tty/n_tty.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)I hate to do this, but isn't it actually my patch posted here
https://lkml.org/lkml/2013/8/1/510Quoted 9 lines
Quoted 1 line
which was tagged as `wrong'?
Sergey,
My apologies; I was mistaken regarding this problem being a lockdep regression (although it's still a false positive from lockdep). Once I had worked around some issues with the nouveau driver, I was able to reproduce the lockdep report on 3.10.
no problem.
Quoted 6 lines
I included Artem's lockdep report in the changelog because I received that first, on 30 July.
My patch below is not the same as your patch of 1 Aug. This patch preserves the protected access of termios.c_cc[VMIN] and termios.c_cc[VTIME] (via the
MIN_CHAR()andTIME_CHAR()macros).fair enough. v3 was protecting VMIN/VTIME (my bad, I noticed this a bit later), but I didn't submit it since v2 did not get positive response.
Quoted 6 lines
Quoted 4 lines
If you'd prefer, I could add to changelog:
Patch based on original posted here https://lkml.org/lkml/2013/8/1/510
by Sergey Senozhatsky <[email protected]>if you don't mind, that would be great.
Ok.
Greg,
Should I re-spin a v2 to include the note above
(or can you add it with Artem's Tested-by)?
Regards,
Peter Hurley
Greg Kroah-Hartman
maintainerRe: [PATCH tty-next] n_tty: Fix termios_rwsem lockdep false positive
Quoted 36 lines · Peter Hurley
Quoted 35 lines · Sergey Senozhatsky
Quoted 8 lines · Peter Hurley
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
[..]
drivers/tty/n_tty.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)I hate to do this, but isn't it actually my patch posted here
https://lkml.org/lkml/2013/8/1/510Quoted 9 lines
Quoted 1 line
which was tagged as `wrong'?
Sergey,
My apologies; I was mistaken regarding this problem being a lockdep regression (although it's still a false positive from lockdep). Once I had worked around some issues with the nouveau driver, I was able to reproduce the lockdep report on 3.10.
no problem.
Quoted 6 lines
I included Artem's lockdep report in the changelog because I received that first, on 30 July.
My patch below is not the same as your patch of 1 Aug. This patch preserves the protected access of termios.c_cc[VMIN] and termios.c_cc[VTIME] (via the
MIN_CHAR()andTIME_CHAR()macros).fair enough. v3 was protecting VMIN/VTIME (my bad, I noticed this a bit later), but I didn't submit it since v2 did not get positive response.
Quoted 4 lines
If you'd prefer, I could add to changelog:
Patch based on original posted here https://lkml.org/lkml/2013/8/1/510
by Sergey Senozhatsky <[email protected]>
Quoted 7 lines
Quoted 1 line
if you don't mind, that would be great.
Ok.
Greg,
Should I re-spin a v2 to include the note above
(or can you add it with Artem's Tested-by)?
I'll add it, no need to resend.
greg k-h