[2.6.33-rc5] tty: possible irq lock inversion dependency in tty

All of lore.kernel.org
 help / color / mirror / Atom feed

* [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync
@ 2010-02-07  5:52 Tetsuo Handa
  2010-02-07  6:31 ` Linus Torvalds
  0 siblings, 1 reply; 9+ messages in thread
From: Tetsuo Handa @ 2010-02-07  5:52 UTC (permalink / raw)
  To: gregkh, taviso, viro
  Cc: linux-kernel, ebiederm, alan, torvalds, jdike, jln, mpm

Hello.

Below problem (which was introduced between 2.6.33-rc4 and 2.6.33-rc5) is
not yet fixed as of 2.6.33-rc7.
"git bisect start v2.6.33-rc5 v2.6.33-rc4" reported that
703625118069f9f8960d356676662d3db5a9d116 tty: fix race in tty_fasync
is first bad commit.

Config is at http://I-love.SAKURA.ne.jp/tmp/config-2.6.33-rc7

Regards.

=========================================================
[ INFO: possible irq lock inversion dependency detected ]
2.6.33-rc5 #2
---------------------------------------------------------
emacs-x/3230 just changed the state of lock:
 (&(&tty->ctrl_lock)->rlock){+.....}, at: [<c05916f8>] tty_fasync+0x92/0x10b
but this lock took another, HARDIRQ-unsafe lock in the past:
 (&(&sighand->siglock)->rlock){-.....}

and interrupts could create inverse lock ordering between them.


other info that might help us debug this:
1 lock held by emacs-x/3230:
 #0:  (&(&tty->ctrl_lock)->rlock){+.....}, at: [<c05916f8>] tty_fasync+0x92/0x10b

the shortest dependencies between 2nd lock and 1st lock:
 -> (&(&sighand->siglock)->rlock){-.....} ops: 78599 {
    IN-HARDIRQ-W at:
                          [<c045d865>] __lock_acquire+0x275/0xb28
                          [<c045e1a8>] lock_acquire+0x90/0xa7
                          [<c066e76d>] _raw_spin_lock_irqsave+0x2c/0x5f
                          [<c0445d0d>] lock_task_sighand+0x2f/0x54
                          [<c0446571>] do_send_sig_info+0x23/0x56
                          [<c04467c5>] group_send_sig_info+0x2b/0x34
                          [<c0446806>] kill_pid_info+0x38/0x4c
                          [<c043b2ee>] it_real_fn+0x4c/0x53
                          [<c0451704>] hrtimer_run_queues+0x169/0x1e1
                          [<c044378b>] run_local_timers+0xd/0x1e
                          [<c0443a99>] update_process_times+0x29/0x4d
                          [<c0458c99>] tick_periodic+0x6b/0x77
                          [<c0458cc3>] tick_handle_periodic+0x1e/0x6b
                          [<c041994c>] smp_apic_timer_interrupt+0x61/0x75
                          [<c066f3b7>] apic_timer_interrupt+0x2f/0x34
                          [<c0401d08>] cpu_idle+0x6d/0x91
                          [<c066919a>] start_secondary+0x255/0x295
    INITIAL USE at:
                         [<c045d94d>] __lock_acquire+0x35d/0xb28
                         [<c045e1a8>] lock_acquire+0x90/0xa7
                         [<c066e76d>] _raw_spin_lock_irqsave+0x2c/0x5f
                         [<c0445d4e>] flush_signals+0x1c/0x3b
                         [<c0445d9b>] ignore_signals+0x2e/0x31
                         [<c044e1fc>] kthreadd+0x27/0xcf
                         [<c0402fba>] kernel_thread_helper+0x6/0x10
  }
  ... key      at: [<c0a0d375>] __key.43588+0x0/0x8
  ... acquired at:
   [<c045dfc2>] __lock_acquire+0x9d2/0xb28
   [<c045e1a8>] lock_acquire+0x90/0xa7
   [<c066e76d>] _raw_spin_lock_irqsave+0x2c/0x5f
   [<c0591a14>] __proc_set_tty+0x27/0xe3
   [<c05940b2>] tty_ioctl+0x36b/0x711
   [<c04cf8ce>] vfs_ioctl+0x27/0x8a
   [<c04cfe27>] do_vfs_ioctl+0x461/0x4ac
   [<c04cfeb7>] sys_ioctl+0x45/0x5f
   [<c0402a0c>] sysenter_do_call+0x12/0x32

-> (&(&tty->ctrl_lock)->rlock){+.....} ops: 712 {
   HARDIRQ-ON-W at:
                        [<c045c185>] mark_held_locks+0x3d/0x58
                        [<c045c298>] trace_hardirqs_on_caller+0xf8/0x139
                        [<c045c2e4>] trace_hardirqs_on+0xb/0xd
                        [<c066e819>] _raw_write_unlock_irq+0x27/0x2b
                        [<c04cf1a4>] f_modown+0x66/0x6b
                        [<c04cf1ee>] __f_setown+0x2f/0x3a
                        [<c0591721>] tty_fasync+0xbb/0x10b
                        [<c04cf44d>] do_fcntl+0x209/0x39e
                        [<c04cf64e>] sys_fcntl64+0x6c/0x80
                        [<c0402a0c>] sysenter_do_call+0x12/0x32
   INITIAL USE at:
                       [<c045d94d>] __lock_acquire+0x35d/0xb28
                       [<c045e1a8>] lock_acquire+0x90/0xa7
                       [<c066e76d>] _raw_spin_lock_irqsave+0x2c/0x5f
                       [<c0591a14>] __proc_set_tty+0x27/0xe3
                       [<c05940b2>] tty_ioctl+0x36b/0x711
                       [<c04cf8ce>] vfs_ioctl+0x27/0x8a
                       [<c04cfe27>] do_vfs_ioctl+0x461/0x4ac
                       [<c04cfeb7>] sys_ioctl+0x45/0x5f
                       [<c0402a0c>] sysenter_do_call+0x12/0x32
 }
 ... key      at: [<c103df74>] __key.28645+0x0/0x8
 ... acquired at:
   [<c045bde5>] check_usage_backwards+0x53/0x5e
   [<c045c06d>] mark_lock+0xe2/0x1bd
   [<c045c185>] mark_held_locks+0x3d/0x58
   [<c045c298>] trace_hardirqs_on_caller+0xf8/0x139
   [<c045c2e4>] trace_hardirqs_on+0xb/0xd
   [<c066e819>] _raw_write_unlock_irq+0x27/0x2b
   [<c04cf1a4>] f_modown+0x66/0x6b
   [<c04cf1ee>] __f_setown+0x2f/0x3a
   [<c0591721>] tty_fasync+0xbb/0x10b
   [<c04cf44d>] do_fcntl+0x209/0x39e
   [<c04cf64e>] sys_fcntl64+0x6c/0x80
   [<c0402a0c>] sysenter_do_call+0x12/0x32


stack backtrace:
Pid: 3230, comm: emacs-x Not tainted 2.6.33-rc5 #2
Call Trace:
 [<c045bd87>] print_irq_inversion_bug+0xe5/0xf0
 [<c045bde5>] check_usage_backwards+0x53/0x5e
 [<c045b744>] ? trace_hardirqs_off+0xb/0xd
 [<c045c06d>] mark_lock+0xe2/0x1bd
 [<c045bd92>] ? check_usage_backwards+0x0/0x5e
 [<c045c185>] mark_held_locks+0x3d/0x58
 [<c066e819>] ? _raw_write_unlock_irq+0x27/0x2b
 [<c045c298>] trace_hardirqs_on_caller+0xf8/0x139
 [<c045c2e4>] trace_hardirqs_on+0xb/0xd
 [<c066e819>] _raw_write_unlock_irq+0x27/0x2b
 [<c04cf1a4>] f_modown+0x66/0x6b
 [<c04cf1ee>] __f_setown+0x2f/0x3a
 [<c0591721>] tty_fasync+0xbb/0x10b
 [<c0591666>] ? tty_fasync+0x0/0x10b
 [<c04cf44d>] do_fcntl+0x209/0x39e
 [<c04c5a11>] ? rcu_read_unlock+0x1c/0x1e
 [<c04cf64e>] sys_fcntl64+0x6c/0x80
 [<c0402a0c>] sysenter_do_call+0x12/0x32


Here is another one (probably same problem).

[   81.651199] =========================================================
[   81.651199] [ INFO: possible irq lock inversion dependency detected ]
[   81.651199] 2.6.33-rc7 #11
[   81.651199] ---------------------------------------------------------
[   81.651199] swapper/0 just changed the state of lock:
[   81.651199]  (&(&sighand->siglock)->rlock){-.....}, at: [<c0445ed9>] lock_task_sighand+0x2f/0x54
[   81.651199] but this lock took another, HARDIRQ-READ-unsafe lock in the past:
[   81.651199]  (&f->f_owner.lock){.+.+..}
[   81.651199]
[   81.651199] and interrupts could create inverse lock ordering between them.
[   81.651199]
[   81.651199]
[   81.651199] other info that might help us debug this:
[   81.651199] 2 locks held by swapper/0:
[   81.651199]  #0:  (rcu_read_lock){.+.+..}, at: [<c0445b4e>] rcu_read_lock+0x0/0x26
[   81.651199]  #1:  (rcu_read_lock){.+.+..}, at: [<c0445b4e>] rcu_read_lock+0x0/0x26
[   81.651199]
[   81.651199] the shortest dependencies between 2nd lock and 1st lock:
[   81.651199]   -> (&f->f_owner.lock){.+.+..} ops: 101 {
[   81.651199]      HARDIRQ-ON-R at:
[   81.651199]                                            [<c045daa5>] __lock_acquire+0x2bd/0xb28
[   81.651199]                                            [<c045e3a0>] lock_acquire+0x90/0xa7
[   81.651199]                                            [<c066f374>] _raw_read_lock+0x23/0x53
[   81.651199]                                            [<c04cf655>] f_getown+0x17/0x38
[   81.651199]                                            [<c04cf9ea>] do_fcntl+0x264/0x39e
[   81.651199]                                            [<c04cfb90>] sys_fcntl64+0x6c/0x80
[   81.651199]                                            [<c0402a0c>] sysenter_do_call+0x12/0x32
[   81.651199]      SOFTIRQ-ON-R at:
[   81.651199]                                            [<c045daf5>] __lock_acquire+0x30d/0xb28
[   81.651199]                                            [<c045e3a0>] lock_acquire+0x90/0xa7
[   81.651199]                                            [<c066f374>] _raw_read_lock+0x23/0x53
[   81.651199]                                            [<c04cf655>] f_getown+0x17/0x38
[   81.651199]                                            [<c04cf9ea>] do_fcntl+0x264/0x39e
[   81.651199]                                            [<c04cfb90>] sys_fcntl64+0x6c/0x80
[   81.651199]                                            [<c0402a0c>] sysenter_do_call+0x12/0x32
[   81.651199]      INITIAL USE at:
[   81.651199]                                           [<c045db45>] __lock_acquire+0x35d/0xb28
[   81.651199]                                           [<c045e3a0>] lock_acquire+0x90/0xa7
[   81.651199]                                           [<c066f374>] _raw_read_lock+0x23/0x53
[   81.651199]                                           [<c04cf655>] f_getown+0x17/0x38
[   81.651199]                                           [<c04cf9ea>] do_fcntl+0x264/0x39e
[   81.651199]                                           [<c04cfb90>] sys_fcntl64+0x6c/0x80
[   81.651199]                                           [<c0402a0c>] sysenter_do_call+0x12/0x32
[   81.651199]    }
[   81.651199]    ... key      at: [<c0f896b0>] __key.26175+0x0/0x8
[   81.651199]    ... acquired at:
[   81.651199]    [<c045e17e>] __lock_acquire+0x996/0xb28
[   81.651199]    [<c045e3a0>] lock_acquire+0x90/0xa7
[   81.651199]    [<c066f115>] _raw_write_lock_irqsave+0x2c/0x5f
[   81.651199]    [<c04cf692>] f_modown+0x1c/0x75
[   81.651199]    [<c04cf730>] __f_setown+0x2f/0x3a
[   81.651199]    [<c0591cb9>] tty_fasync+0xbb/0x10b
[   81.651199]    [<c04cf98f>] do_fcntl+0x209/0x39e
[   81.651199]    [<c04cfb90>] sys_fcntl64+0x6c/0x80
[   81.651199]    [<c0402a0c>] sysenter_do_call+0x12/0x32
[   81.651199]
[   81.651199]  -> (&(&tty->ctrl_lock)->rlock){......} ops: 662 {
[   81.651199]     INITIAL USE at:
[   81.651199]                                         [<c045db45>] __lock_acquire+0x35d/0xb28
[   81.651199]                                         [<c045e3a0>] lock_acquire+0x90/0xa7
[   81.651199]                                         [<c066ee15>] _raw_spin_lock_irqsave+0x2c/0x5f
[   81.651199]                                         [<c0591fac>] __proc_set_tty+0x27/0xe3
[   81.651199]                                         [<c059464a>] tty_ioctl+0x36b/0x711
[   81.651199]                                         [<c04cfe12>] vfs_ioctl+0x27/0x8a
[   81.651199]                                         [<c04d036b>] do_vfs_ioctl+0x461/0x4ac
[   81.651199]                                         [<c04d03fb>] sys_ioctl+0x45/0x5f
[   81.651199]                                         [<c0402a0c>] sysenter_do_call+0x12/0x32
[   81.651199]   }
[   81.651199]   ... key      at: [<c103df74>] __key.28647+0x0/0x8
[   81.651199]   ... acquired at:
[   81.651199]    [<c045e17e>] __lock_acquire+0x996/0xb28
[   81.651199]    [<c045e3a0>] lock_acquire+0x90/0xa7
[   81.651199]    [<c066ee15>] _raw_spin_lock_irqsave+0x2c/0x5f
[   81.651199]    [<c0591fac>] __proc_set_tty+0x27/0xe3
[   81.651199]    [<c059464a>] tty_ioctl+0x36b/0x711
[   81.651199]    [<c04cfe12>] vfs_ioctl+0x27/0x8a
[   81.651199]    [<c04d036b>] do_vfs_ioctl+0x461/0x4ac
[   81.651199]    [<c04d03fb>] sys_ioctl+0x45/0x5f
[   81.651199]    [<c0402a0c>] sysenter_do_call+0x12/0x32
[   81.651199]
[   81.651199] -> (&(&sighand->siglock)->rlock){-.....} ops: 80027 {
[   81.651199]    IN-HARDIRQ-W at:
[   81.651199]                                        [<c045da5d>] __lock_acquire+0x275/0xb28
[   81.651199]                                        [<c045e3a0>] lock_acquire+0x90/0xa7
[   81.651199]                                        [<c066ee15>] _raw_spin_lock_irqsave+0x2c/0x5f
[   81.651199]                                        [<c0445ed9>] lock_task_sighand+0x2f/0x54
[   81.651199]                                        [<c044673d>] do_send_sig_info+0x23/0x56
[   81.651199]                                        [<c0446991>] group_send_sig_info+0x2b/0x34
[   81.651199]                                        [<c04469d2>] kill_pid_info+0x38/0x4c
[   81.651199]                                        [<c043b4ba>] it_real_fn+0x4c/0x53
[   81.651199]                                        [<c04518d0>] hrtimer_run_queues+0x169/0x1e1
[   81.651199]                                        [<c0443957>] run_local_timers+0xd/0x1e
[   81.651199]                                        [<c0443c65>] update_process_times+0x29/0x4d
[   81.651199]                                        [<c0458e95>] tick_periodic+0x6b/0x77
[   81.651199]                                        [<c0458ebf>] tick_handle_periodic+0x1e/0x6b
[   81.651199]                                        [<c0419958>] smp_apic_timer_interrupt+0x61/0x75
[   81.651199]                                        [<c066fa5f>] apic_timer_interrupt+0x2f/0x34
[   81.651199]                                        [<c0401d08>] cpu_idle+0x6d/0x91
[   81.651199]                                        [<c065be03>] rest_init+0x67/0x69
[   81.651199]                                        [<c08448be>] start_kernel+0x323/0x32a
[   81.651199]                                        [<c084408f>] i386_start_kernel+0x8f/0x94
[   81.651199]    INITIAL USE at:
[   81.651199]                                       [<c045db45>] __lock_acquire+0x35d/0xb28
[   81.651199]                                       [<c045e3a0>] lock_acquire+0x90/0xa7
[   81.651199]                                       [<c066ee15>] _raw_spin_lock_irqsave+0x2c/0x5f
[   81.651199]                                       [<c0445f1a>] flush_signals+0x1c/0x3b
[   81.651199]                                       [<c0445f67>] ignore_signals+0x2e/0x31
[   81.651199]                                       [<c044e3c8>] kthreadd+0x27/0xcf
[   81.651199]                                       [<c0402fba>] kernel_thread_helper+0x6/0x10
[   81.651199]  }
[   81.651199]  ... key      at: [<c0a0d375>] __key.43570+0x0/0x8
[   81.651199]  ... acquired at:
[   81.651199]    [<c045c03b>] check_usage_forwards+0x53/0x5e
[   81.651199]    [<c045c2ae>] mark_lock+0x12b/0x1bd
[   81.651199]    [<c045da5d>] __lock_acquire+0x275/0xb28
[   81.651199]    [<c045e3a0>] lock_acquire+0x90/0xa7
[   81.651199]    [<c066ee15>] _raw_spin_lock_irqsave+0x2c/0x5f
[   81.651199]    [<c0445ed9>] lock_task_sighand+0x2f/0x54
[   81.651199]    [<c044673d>] do_send_sig_info+0x23/0x56
[   81.651199]    [<c0446991>] group_send_sig_info+0x2b/0x34
[   81.651199]    [<c04469d2>] kill_pid_info+0x38/0x4c
[   81.651199]    [<c043b4ba>] it_real_fn+0x4c/0x53
[   81.651199]    [<c04518d0>] hrtimer_run_queues+0x169/0x1e1
[   81.651199]    [<c0443957>] run_local_timers+0xd/0x1e
[   81.651199]    [<c0443c65>] update_process_times+0x29/0x4d
[   81.651199]    [<c0458e95>] tick_periodic+0x6b/0x77
[   81.651199]    [<c0458ebf>] tick_handle_periodic+0x1e/0x6b
[   81.651199]    [<c0419958>] smp_apic_timer_interrupt+0x61/0x75
[   81.651199]    [<c066fa5f>] apic_timer_interrupt+0x2f/0x34
[   81.651199]    [<c0401d08>] cpu_idle+0x6d/0x91
[   81.651199]    [<c065be03>] rest_init+0x67/0x69
[   81.651199]    [<c08448be>] start_kernel+0x323/0x32a
[   81.651199]    [<c084408f>] i386_start_kernel+0x8f/0x94
[   81.651199]
[   81.651199]
[   81.651199] stack backtrace:
[   81.651199] Pid: 0, comm: swapper Not tainted 2.6.33-rc7 #11
[   81.651199] Call Trace:
[   81.651199]  [<c045bf7f>] print_irq_inversion_bug+0xe5/0xf0
[   81.651199]  [<c045c03b>] check_usage_forwards+0x53/0x5e
[   81.651199]  [<c0452f50>] ? cpu_clock+0x2e/0x46
[   81.651199]  [<c045c2ae>] mark_lock+0x12b/0x1bd
[   81.651199]  [<c045bfe8>] ? check_usage_forwards+0x0/0x5e
[   81.651199]  [<c045da5d>] __lock_acquire+0x275/0xb28
[   81.651199]  [<c045b93c>] ? trace_hardirqs_off+0xb/0xd
[   81.651199]  [<c045e2c3>] ? __lock_acquire+0xadb/0xb28
[   81.651199]  [<c045e3a0>] lock_acquire+0x90/0xa7
[   81.651199]  [<c0445ed9>] ? lock_task_sighand+0x2f/0x54
[   81.651199]  [<c066ee15>] _raw_spin_lock_irqsave+0x2c/0x5f
[   81.651199]  [<c0445ed9>] ? lock_task_sighand+0x2f/0x54
[   81.651199]  [<c0445ed9>] lock_task_sighand+0x2f/0x54
[   81.651199]  [<c044673d>] do_send_sig_info+0x23/0x56
[   81.651199]  [<c0446991>] group_send_sig_info+0x2b/0x34
[   81.651199]  [<c04469d2>] kill_pid_info+0x38/0x4c
[   81.651199]  [<c043b4ba>] it_real_fn+0x4c/0x53
[   81.651199]  [<c04518d0>] hrtimer_run_queues+0x169/0x1e1
[   81.651199]  [<c043b46e>] ? it_real_fn+0x0/0x53
[   81.651199]  [<c0443957>] run_local_timers+0xd/0x1e
[   81.651199]  [<c0443c65>] update_process_times+0x29/0x4d
[   81.651199]  [<c0458e95>] tick_periodic+0x6b/0x77
[   81.651199]  [<c0458ebf>] tick_handle_periodic+0x1e/0x6b
[   81.651199]  [<c0419958>] smp_apic_timer_interrupt+0x61/0x75
[   81.651199]  [<c066fa5f>] apic_timer_interrupt+0x2f/0x34
[   81.651199]  [<c04087d6>] ? default_idle+0x49/0x68
[   81.651199]  [<c0401d08>] cpu_idle+0x6d/0x91
[   81.651199]  [<c065be03>] rest_init+0x67/0x69
[   81.651199]  [<c08448be>] start_kernel+0x323/0x32a
[   81.651199]  [<c084408f>] i386_start_kernel+0x8f/0x94

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync
  2010-02-07  5:52 [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync Tetsuo Handa
@ 2010-02-07  6:31 ` Linus Torvalds
  2010-02-07  6:46   ` Linus Torvalds
  2010-02-07  6:46   ` [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync Américo Wang
  0 siblings, 2 replies; 9+ messages in thread
From: Linus Torvalds @ 2010-02-07  6:31 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: gregkh, taviso, viro, linux-kernel, ebiederm, alan, jdike, jln,
	mpm



On Sun, 7 Feb 2010, Tetsuo Handa wrote:
> 
> Below problem (which was introduced between 2.6.33-rc4 and 2.6.33-rc5) is
> not yet fixed as of 2.6.33-rc7.
> "git bisect start v2.6.33-rc5 v2.6.33-rc4" reported that
> 703625118069f9f8960d356676662d3db5a9d116 tty: fix race in tty_fasync
> is first bad commit.

Yeah. I think we need to just revert that commit.

Or maybe we could just do the following, rather than revert it outright: 
just get a ref to the 'struct pid' while holding the spinlock, and then 
releasing it after doing the __f_setown() call.

That way we know 'pid' isn't going away.

What? Untested, of course.

			Linus

---
 drivers/char/tty_io.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index c6f3b48..dcb9083 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1951,8 +1951,10 @@ static int tty_fasync(int fd, struct file *filp, int on)
 			pid = task_pid(current);
 			type = PIDTYPE_PID;
 		}
-		retval = __f_setown(filp, pid, type, 0);
+		get_pid(pid);
 		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+		retval = __f_setown(filp, pid, type, 0);
+		put_pid(pid);
 		if (retval)
 			goto out;
 	} else {

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync
  2010-02-07  6:31 ` Linus Torvalds
@ 2010-02-07  6:46   ` Linus Torvalds
  2010-02-07  7:27     ` Greg KH
  2010-02-07  6:46   ` [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync Américo Wang
  1 sibling, 1 reply; 9+ messages in thread
From: Linus Torvalds @ 2010-02-07  6:46 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: gregkh, taviso, viro, linux-kernel, ebiederm, alan, jdike, jln,
	mpm



On Sat, 6 Feb 2010, Linus Torvalds wrote:
> 
> Yeah. I think we need to just revert that commit.
> 
> Or maybe we could just do the following, rather than revert it outright: 
> just get a ref to the 'struct pid' while holding the spinlock, and then 
> releasing it after doing the __f_setown() call.

Btw, if we do this, then we should probably revert commit 
b04da8bfdfbbd79544cab2fadfdc12e87eb01600 at the same time.

Resulting patch would then look like the appended.

		Linus

---
 drivers/char/tty_io.c |    4 +++-
 fs/fcntl.c            |    6 ++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index c6f3b48..dcb9083 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1951,8 +1951,10 @@ static int tty_fasync(int fd, struct file *filp, int on)
 			pid = task_pid(current);
 			type = PIDTYPE_PID;
 		}
-		retval = __f_setown(filp, pid, type, 0);
+		get_pid(pid);
 		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+		retval = __f_setown(filp, pid, type, 0);
+		put_pid(pid);
 		if (retval)
 			goto out;
 	} else {
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 5ef953e..97e01dc 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -199,9 +199,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
 static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
                      int force)
 {
-	unsigned long flags;
-
-	write_lock_irqsave(&filp->f_owner.lock, flags);
+	write_lock_irq(&filp->f_owner.lock);
 	if (force || !filp->f_owner.pid) {
 		put_pid(filp->f_owner.pid);
 		filp->f_owner.pid = get_pid(pid);
@@ -213,7 +211,7 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
 			filp->f_owner.euid = cred->euid;
 		}
 	}
-	write_unlock_irqrestore(&filp->f_owner.lock, flags);
+	write_unlock_irq(&filp->f_owner.lock);
 }
 
 int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync
  2010-02-07  6:46   ` Linus Torvalds
@ 2010-02-07  7:27     ` Greg KH
  2010-02-07  8:12       ` [2.6.33-rc5] tty: possible irq lock inversion dependency intty_fasync Tetsuo Handa
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2010-02-07  7:27 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Tetsuo Handa, taviso, viro, linux-kernel, ebiederm, alan, jdike,
	jln, mpm

On Sat, Feb 06, 2010 at 10:46:01PM -0800, Linus Torvalds wrote:
> 
> 
> On Sat, 6 Feb 2010, Linus Torvalds wrote:
> > 
> > Yeah. I think we need to just revert that commit.
> > 
> > Or maybe we could just do the following, rather than revert it outright: 
> > just get a ref to the 'struct pid' while holding the spinlock, and then 
> > releasing it after doing the __f_setown() call.
> 
> Btw, if we do this, then we should probably revert commit 
> b04da8bfdfbbd79544cab2fadfdc12e87eb01600 at the same time.
> 
> Resulting patch would then look like the appended.

The patch looks correct to me, thanks for fixing this.

  Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Can you commit it to your tree, or do you need/want me to submit it?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [2.6.33-rc5] tty: possible irq lock inversion dependency intty_fasync
  2010-02-07  7:27     ` Greg KH
@ 2010-02-07  8:12       ` Tetsuo Handa
  0 siblings, 0 replies; 9+ messages in thread
From: Tetsuo Handa @ 2010-02-07  8:12 UTC (permalink / raw)
  To: gregkh, torvalds
  Cc: taviso, viro, linux-kernel, ebiederm, alan, jdike, jln, mpm

Greg KH wrote:
> On Sat, Feb 06, 2010 at 10:46:01PM -0800, Linus Torvalds wrote:
> > 
> > 
> > On Sat, 6 Feb 2010, Linus Torvalds wrote:
> > > 
> > > Yeah. I think we need to just revert that commit.
> > > 
> > > Or maybe we could just do the following, rather than revert it outright: 
> > > just get a ref to the 'struct pid' while holding the spinlock, and then 
> > > releasing it after doing the __f_setown() call.
> > 
> > Btw, if we do this, then we should probably revert commit 
> > b04da8bfdfbbd79544cab2fadfdc12e87eb01600 at the same time.
> > 
> > Resulting patch would then look like the appended.
> 
> The patch looks correct to me, thanks for fixing this.
> 
>   Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> Can you commit it to your tree, or do you need/want me to submit it?
> 
> thanks,
> 
> greg k-h
> 
That patch solved the lockdep warning.

Thank you.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync
  2010-02-07  6:31 ` Linus Torvalds
  2010-02-07  6:46   ` Linus Torvalds
@ 2010-02-07  6:46   ` Américo Wang
  2010-02-07  6:59     ` Linus Torvalds
  2010-02-07  7:00     ` Eric W. Biederman
  1 sibling, 2 replies; 9+ messages in thread
From: Américo Wang @ 2010-02-07  6:46 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Tetsuo Handa, gregkh, taviso, viro, linux-kernel, ebiederm, alan,
	jdike, jln, mpm

On Sat, Feb 06, 2010 at 10:31:30PM -0800, Linus Torvalds wrote:
>
>
>On Sun, 7 Feb 2010, Tetsuo Handa wrote:
>> 
>> Below problem (which was introduced between 2.6.33-rc4 and 2.6.33-rc5) is
>> not yet fixed as of 2.6.33-rc7.
>> "git bisect start v2.6.33-rc5 v2.6.33-rc4" reported that
>> 703625118069f9f8960d356676662d3db5a9d116 tty: fix race in tty_fasync
>> is first bad commit.
>
>Yeah. I think we need to just revert that commit.
>
>Or maybe we could just do the following, rather than revert it outright: 
>just get a ref to the 'struct pid' while holding the spinlock, and then 
>releasing it after doing the __f_setown() call.

We already fixed this, a better fix:

http://lkml.org/lkml/2010/1/26/338

I sent a same fix with Greg's.

Thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync
  2010-02-07  6:46   ` [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync Américo Wang
@ 2010-02-07  6:59     ` Linus Torvalds
  2010-02-07 16:06       ` Américo Wang
  2010-02-07  7:00     ` Eric W. Biederman
  1 sibling, 1 reply; 9+ messages in thread
From: Linus Torvalds @ 2010-02-07  6:59 UTC (permalink / raw)
  To: Américo Wang
  Cc: Tetsuo Handa, gregkh, taviso, viro, linux-kernel, ebiederm, alan,
	jdike, jln, mpm

On Sun, 7 Feb 2010, Américo Wang wrote:
> 
> We already fixed this, a better fix:

No we didn't.

> http://lkml.org/lkml/2010/1/26/338
> 
> I sent a same fix with Greg's.

We already did that. You didn't read Tetsuo's email carefully.

Let's quote the important parts:

  "is not yet fixed as of 2.6.33-rc7."

and also the _second_ lockdep complaint he quotes, which starts out with

  [   81.651199] =========================================================
  [   81.651199] [ INFO: possible irq lock inversion dependency detected ]
  [   81.651199] 2.6.33-rc7 #11
  [   81.651199] ---------------------------------------------------------

(note the -rc7 there).

The problem? Look at f_getown: it does

	read_lock(&filp->f_owner.lock);

ie it holds f_owner without interrupts disabled. Now an interrupt comes 
in, and takes 'siglock' because it ends up sending a signal (timer, SIGIO, 
whatever). So you have a f_owner -> siglock ordering.

But we _also_ have a siglock -> ctrl_lock -> f_owner ordering, in that 
problematic tty_fasync() thing. So we have a ABBA deadlock situation.

Yes, it's hard (practically impossible) to trigger, because you have to 
get an interrupt just at the right point with all the right processes, but 
lockdep seems to be entirely correct. 

So it is simply _wrong_ to take f_owner while we hold ctrl_lock.

Which is why I suggest just reverting both the original problematic commit 
_and_ the commit you point to, and just fix the race with that pid_get/put 
pair instead. As per my patch.

		Linus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync
  2010-02-07  6:59     ` Linus Torvalds
@ 2010-02-07 16:06       ` Américo Wang
  0 siblings, 0 replies; 9+ messages in thread
From: Américo Wang @ 2010-02-07 16:06 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Américo Wang, Tetsuo Handa, gregkh, taviso, viro,
	linux-kernel, ebiederm, alan, jdike, jln, mpm

On Sat, Feb 06, 2010 at 10:59:56PM -0800, Linus Torvalds wrote:
>
>
>On Sun, 7 Feb 2010, Américo Wang wrote:
>> 
>> We already fixed this, a better fix:
>
>No we didn't.
>
>> http://lkml.org/lkml/2010/1/26/338
>> 
>> I sent a same fix with Greg's.
>
>We already did that. You didn't read Tetsuo's email carefully.
>
>Let's quote the important parts:
>
>  "is not yet fixed as of 2.6.33-rc7."
>
>and also the _second_ lockdep complaint he quotes, which starts out with
>
>  [   81.651199] =========================================================
>  [   81.651199] [ INFO: possible irq lock inversion dependency detected ]
>  [   81.651199] 2.6.33-rc7 #11
>  [   81.651199] ---------------------------------------------------------
>
>(note the -rc7 there).
>


Ah, I was just mislead by the subject... sigh


>The problem? Look at f_getown: it does
>
>	read_lock(&filp->f_owner.lock);
>
>ie it holds f_owner without interrupts disabled. Now an interrupt comes 
>in, and takes 'siglock' because it ends up sending a signal (timer, SIGIO, 
>whatever). So you have a f_owner -> siglock ordering.
>
>But we _also_ have a siglock -> ctrl_lock -> f_owner ordering, in that 
>problematic tty_fasync() thing. So we have a ABBA deadlock situation.
>
>Yes, it's hard (practically impossible) to trigger, because you have to 
>get an interrupt just at the right point with all the right processes, but 
>lockdep seems to be entirely correct. 
>
>So it is simply _wrong_ to take f_owner while we hold ctrl_lock.
>
>Which is why I suggest just reverting both the original problematic commit 
>_and_ the commit you point to, and just fix the race with that pid_get/put 
>pair instead. As per my patch.
>

Good analysis, this seems better for me too.

Thank you!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync
  2010-02-07  6:46   ` [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync Américo Wang
  2010-02-07  6:59     ` Linus Torvalds
@ 2010-02-07  7:00     ` Eric W. Biederman
  1 sibling, 0 replies; 9+ messages in thread
From: Eric W. Biederman @ 2010-02-07  7:00 UTC (permalink / raw)
  To: Américo Wang
  Cc: Linus Torvalds, Tetsuo Handa, gregkh, taviso, viro, linux-kernel,
	alan, jdike, jln, mpm

Américo Wang <xiyou.wangcong@gmail.com> writes:

> On Sat, Feb 06, 2010 at 10:31:30PM -0800, Linus Torvalds wrote:
>>
>>
>>On Sun, 7 Feb 2010, Tetsuo Handa wrote:
>>> 
>>> Below problem (which was introduced between 2.6.33-rc4 and 2.6.33-rc5) is
>>> not yet fixed as of 2.6.33-rc7.
>>> "git bisect start v2.6.33-rc5 v2.6.33-rc4" reported that
>>> 703625118069f9f8960d356676662d3db5a9d116 tty: fix race in tty_fasync
>>> is first bad commit.
>>
>>Yeah. I think we need to just revert that commit.
>>
>>Or maybe we could just do the following, rather than revert it outright: 
>>just get a ref to the 'struct pid' while holding the spinlock, and then 
>>releasing it after doing the __f_setown() call.
>
> We already fixed this, a better fix:
>
> http://lkml.org/lkml/2010/1/26/338
>
> I sent a same fix with Greg's.

That fix is present. See below.  Why does lockdep still warn?
Do we have lockdep bug?

Given that there is the only place we take f_owner.lock for write I
don't see how f_owner.lock can be unsafe to be called with irqs
disabled.

commit b04da8bfdfbbd79544cab2fadfdc12e87eb01600
Author: Greg Kroah-Hartman <gregkh@suse.de>
Date:   Tue Jan 26 15:04:02 2010 -0800

    fnctl: f_modown should call write_lock_irqsave/restore
    
    Commit 703625118069f9f8960d356676662d3db5a9d116 exposed that f_modown()
    should call write_lock_irqsave instead of just write_lock_irq so that
    because a caller could have a spinlock held and it would not be good to
    renable interrupts.
    
    Cc: Eric W. Biederman <ebiederm@xmission.com>
    Cc: Al Viro <viro@ZenIV.linux.org.uk>
    Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
    Cc: Tavis Ormandy <taviso@google.com>
    Cc: stable <stable@kernel.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 97e01dc..5ef953e 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -199,7 +199,9 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
 static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
                      int force)
 {
-       write_lock_irq(&filp->f_owner.lock);
+       unsigned long flags;
+
+       write_lock_irqsave(&filp->f_owner.lock, flags);
        if (force || !filp->f_owner.pid) {
                put_pid(filp->f_owner.pid);
                filp->f_owner.pid = get_pid(pid);
@@ -211,7 +213,7 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
                        filp->f_owner.euid = cred->euid;
                }
        }
-       write_unlock_irq(&filp->f_owner.lock);
+       write_unlock_irqrestore(&filp->f_owner.lock, flags);
 }
 
 int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,



^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2010-02-07 16:04 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-07  5:52 [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync Tetsuo Handa
2010-02-07  6:31 ` Linus Torvalds
2010-02-07  6:46   ` Linus Torvalds
2010-02-07  7:27     ` Greg KH
2010-02-07  8:12       ` [2.6.33-rc5] tty: possible irq lock inversion dependency intty_fasync Tetsuo Handa
2010-02-07  6:46   ` [2.6.33-rc5] tty: possible irq lock inversion dependency in tty_fasync Américo Wang
2010-02-07  6:59     ` Linus Torvalds
2010-02-07 16:06       ` Américo Wang
2010-02-07  7:00     ` Eric W. Biederman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.