Skip to contentSkip to conversation

starting emacs makes lockdep warning

2.6.33-rc5 In review KOSAKI Motohiro 7 participants 21 messages Jan 26, 2010 – Jan 27, 2010

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

Hi

Current linus tree made following lockdep warning when starting emacs command. Is this known issue?

=========================================================
[ INFO: possible irq lock inversion dependency detected ]
2.6.33-rc5 #77
---------------------------------------------------------
emacs/1609 just changed the state of lock:
(&(&tty->ctrl_lock)->rlock){+.....}, at: [<ffffffff8127c648>] tty_fasync+0xe8/0x190
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/1609:
#0: (&(&tty->ctrl_lock)->rlock){+.....}, at: [<ffffffff8127c648>] tty_fasync+0xe8/0x190

the shortest dependencies between 2nd lock and 1st lock:

 -> (&(&sighand->siglock)->rlock){-.....} ops: 50393 {
    IN-HARDIRQ-W at:
                          [<ffffffff8108924e>] __lock_acquire+0x7ae/0x15a0
                          [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
                          [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
                          [<ffffffff81065799>] lock_task_sighand+0x79/0x100
                          [<ffffffff8106600f>] do_send_sig_info+0x3f/0x90
                          [<ffffffff810660f0>] group_send_sig_info+0x40/0x50
                          [<ffffffff81066703>] kill_pid_info+0x73/0xe0
                          [<ffffffff81054014>] it_real_fn+0x44/0xa0
                          [<ffffffff81075d1e>] __run_hrtimer+0x8e/0x1e0
                          [<ffffffff81076116>] hrtimer_interrupt+0xe6/0x250
                          [<ffffffff8142a0b9>] smp_apic_timer_interrupt+0x69/0x9b
                          [<ffffffff81003a93>] apic_timer_interrupt+0x13/0x20
                          [<ffffffff81001956>] cpu_idle+0x66/0xd0
                          [<ffffffff814082e2>] rest_init+0x92/0xa0
                          [<ffffffff81b4cd84>] start_kernel+0x3b9/0x3c5
                          [<ffffffff81b4c310>] x86_64_start_reservations+0x120/0x124
                          [<ffffffff81b4c3f8>] x86_64_start_kernel+0xe4/0xeb
    INITIAL USE at:
                         [<ffffffff81088e86>] __lock_acquire+0x3e6/0x15a0
                         [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
                         [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
                         [<ffffffff810646dc>] flush_signals+0x2c/0x60
                         [<ffffffff81064743>] ignore_signals+0x33/0x40
                         [<ffffffff81071067>] kthreadd+0x37/0x180
                         [<ffffffff81003ed4>] kernel_thread_helper+0x4/0x10
  }
  ... key      at: [<ffffffff81c054a4>] __key.46539+0x0/0x8
  ... acquired at:
   [<ffffffff81089af6>] __lock_acquire+0x1056/0x15a0
   [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
   [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
   [<ffffffff8127c1be>] __proc_set_tty+0x3e/0x150
   [<ffffffff8127e01d>] tty_open+0x51d/0x5e0
   [<ffffffff81142400>] chrdev_open+0x170/0x290
   [<ffffffff8113c561>] __dentry_open+0x131/0x3a0
   [<ffffffff8113c8e4>] nameidata_to_filp+0x54/0x70
   [<ffffffff8114c098>] do_filp_open+0x948/0xcd0
   [<ffffffff8113c2e9>] do_sys_open+0x69/0x140
   [<ffffffff8113c400>] sys_open+0x20/0x30
   [<ffffffff8100309b>] system_call_fastpath+0x16/0x1b

-> (&(&tty->ctrl_lock)->rlock){+.....} ops: 191 {
HARDIRQ-ON-W at:

                        [<ffffffff81087e63>] mark_held_locks+0x73/0xa0
                        [<ffffffff810880bb>] trace_hardirqs_on_caller+0x7b/0x1c0
                        [<ffffffff8108820d>] trace_hardirqs_on+0xd/0x10
                        [<ffffffff81423730>] _raw_write_unlock_irq+0x30/0x60
                        [<ffffffff8114cc63>] f_modown+0x53/0xe0
                        [<ffffffff8114cd1e>] __f_setown+0xe/0x20
                        [<ffffffff8127c667>] tty_fasync+0x107/0x190
                        [<ffffffff8114d842>] sys_fcntl+0x222/0x580
                        [<ffffffff8100309b>] system_call_fastpath+0x16/0x1b
   INITIAL USE at:
                       [<ffffffff81088e86>] __lock_acquire+0x3e6/0x15a0
                       [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
                       [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
                       [<ffffffff8127c1be>] __proc_set_tty+0x3e/0x150
                       [<ffffffff8127e01d>] tty_open+0x51d/0x5e0
                       [<ffffffff81142400>] chrdev_open+0x170/0x290
                       [<ffffffff8113c561>] __dentry_open+0x131/0x3a0
                       [<ffffffff8113c8e4>] nameidata_to_filp+0x54/0x70
                       [<ffffffff8114c098>] do_filp_open+0x948/0xcd0
                       [<ffffffff8113c2e9>] do_sys_open+0x69/0x140
                       [<ffffffff8113c400>] sys_open+0x20/0x30
                       [<ffffffff8100309b>] system_call_fastpath+0x16/0x1b
 }
 ... key      at: [<ffffffff82533fb8>] __key.30033+0x0/0x8
 ... acquired at:
   [<ffffffff81087263>] check_usage_backwards+0x93/0x100
   [<ffffffff81087b9a>] mark_lock+0x1ca/0x420
   [<ffffffff81087e63>] mark_held_locks+0x73/0xa0
   [<ffffffff810880bb>] trace_hardirqs_on_caller+0x7b/0x1c0
   [<ffffffff8108820d>] trace_hardirqs_on+0xd/0x10
   [<ffffffff81423730>] _raw_write_unlock_irq+0x30/0x60
   [<ffffffff8114cc63>] f_modown+0x53/0xe0
   [<ffffffff8114cd1e>] __f_setown+0xe/0x20
   [<ffffffff8127c667>] tty_fasync+0x107/0x190
   [<ffffffff8114d842>] sys_fcntl+0x222/0x580
   [<ffffffff8100309b>] system_call_fastpath+0x16/0x1b


stack backtrace:
Pid: 1609, comm: emacs Not tainted 2.6.33-rc5 #77
Call Trace:
 [<ffffffff810870bd>] print_irq_inversion_bug.clone.0+0x12d/0x140
 [<ffffffff810871d0>] ? check_usage_backwards+0x0/0x100
 [<ffffffff81087263>] check_usage_backwards+0x93/0x100
 [<ffffffff8114cc4c>] ? f_modown+0x3c/0xe0
 [<ffffffff81087b9a>] mark_lock+0x1ca/0x420
 [<ffffffff81087e63>] mark_held_locks+0x73/0xa0
 [<ffffffff81423730>] ? _raw_write_unlock_irq+0x30/0x60
 [<ffffffff810880bb>] trace_hardirqs_on_caller+0x7b/0x1c0
 [<ffffffff8108820d>] trace_hardirqs_on+0xd/0x10
 [<ffffffff81423730>] _raw_write_unlock_irq+0x30/0x60
 [<ffffffff8114cc63>] f_modown+0x53/0xe0
 [<ffffffff8114cd1e>] __f_setown+0xe/0x20
 [<ffffffff8127c667>] tty_fasync+0x107/0x190
 [<ffffffff8114d842>] sys_fcntl+0x222/0x580
 [<ffffffff8100309b>] system_call_fastpath+0x16/0x1b
#1 lore raw
Quoted 15 lines · KOSAKI Motohiro

Hi

Current linus tree made following lockdep warning when starting emacs command. Is this known issue?

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

Quoted 3 lines

and interrupts could create inverse lock ordering between them.

Hey,

does reverting commit 703625118 help?

Quoted 113 lines

other info that might help us debug this:
1 lock held by emacs/1609:
 #0:  (&(&tty->ctrl_lock)->rlock){+.....}, at: [<ffffffff8127c648>] tty_fasync+0xe8/0x190

the shortest dependencies between 2nd lock and 1st lock:
 -> (&(&sighand->siglock)->rlock){-.....} ops: 50393 { IN-HARDIRQ-W at:

                         [<ffffffff8108924e>] __lock_acquire+0x7ae/0x15a0
                         [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
                         [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
                         [<ffffffff81065799>] lock_task_sighand+0x79/0x100
                         [<ffffffff8106600f>] do_send_sig_info+0x3f/0x90
                         [<ffffffff810660f0>] group_send_sig_info+0x40/0x50
                         [<ffffffff81066703>] kill_pid_info+0x73/0xe0
                         [<ffffffff81054014>] it_real_fn+0x44/0xa0
                         [<ffffffff81075d1e>] __run_hrtimer+0x8e/0x1e0
                         [<ffffffff81076116>] hrtimer_interrupt+0xe6/0x250
                         [<ffffffff8142a0b9>] smp_apic_timer_interrupt+0x69/0x9b
                         [<ffffffff81003a93>] apic_timer_interrupt+0x13/0x20
                         [<ffffffff81001956>] cpu_idle+0x66/0xd0
                         [<ffffffff814082e2>] rest_init+0x92/0xa0
                         [<ffffffff81b4cd84>] start_kernel+0x3b9/0x3c5
                         [<ffffffff81b4c310>] x86_64_start_reservations+0x120/0x124
                         [<ffffffff81b4c3f8>] x86_64_start_kernel+0xe4/0xeb
   INITIAL USE at:
                        [<ffffffff81088e86>] __lock_acquire+0x3e6/0x15a0
                        [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
                        [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
                        [<ffffffff810646dc>] flush_signals+0x2c/0x60
                        [<ffffffff81064743>] ignore_signals+0x33/0x40
                        [<ffffffff81071067>] kthreadd+0x37/0x180
                        [<ffffffff81003ed4>] kernel_thread_helper+0x4/0x10
 }
 ... key      at: [<ffffffff81c054a4>] __key.46539+0x0/0x8
 ... acquired at:
  [<ffffffff81089af6>] __lock_acquire+0x1056/0x15a0
  [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
  [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
  [<ffffffff8127c1be>] __proc_set_tty+0x3e/0x150
  [<ffffffff8127e01d>] tty_open+0x51d/0x5e0
  [<ffffffff81142400>] chrdev_open+0x170/0x290
  [<ffffffff8113c561>] __dentry_open+0x131/0x3a0
  [<ffffffff8113c8e4>] nameidata_to_filp+0x54/0x70
  [<ffffffff8114c098>] do_filp_open+0x948/0xcd0
  [<ffffffff8113c2e9>] do_sys_open+0x69/0x140
  [<ffffffff8113c400>] sys_open+0x20/0x30
  [<ffffffff8100309b>] system_call_fastpath+0x16/0x1b

-> (&(&tty->ctrl_lock)->rlock){+.....} ops: 191 {
  HARDIRQ-ON-W at:

                       [<ffffffff81087e63>] mark_held_locks+0x73/0xa0
                       [<ffffffff810880bb>] trace_hardirqs_on_caller+0x7b/0x1c0
                       [<ffffffff8108820d>] trace_hardirqs_on+0xd/0x10
                       [<ffffffff81423730>] _raw_write_unlock_irq+0x30/0x60
                       [<ffffffff8114cc63>] f_modown+0x53/0xe0
                       [<ffffffff8114cd1e>] __f_setown+0xe/0x20
                       [<ffffffff8127c667>] tty_fasync+0x107/0x190
                       [<ffffffff8114d842>] sys_fcntl+0x222/0x580
                       [<ffffffff8100309b>] system_call_fastpath+0x16/0x1b
  INITIAL USE at:
                      [<ffffffff81088e86>] __lock_acquire+0x3e6/0x15a0
                      [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
                      [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
                      [<ffffffff8127c1be>] __proc_set_tty+0x3e/0x150
                      [<ffffffff8127e01d>] tty_open+0x51d/0x5e0
                      [<ffffffff81142400>] chrdev_open+0x170/0x290
                      [<ffffffff8113c561>] __dentry_open+0x131/0x3a0
                      [<ffffffff8113c8e4>] nameidata_to_filp+0x54/0x70
                      [<ffffffff8114c098>] do_filp_open+0x948/0xcd0
                      [<ffffffff8113c2e9>] do_sys_open+0x69/0x140
                      [<ffffffff8113c400>] sys_open+0x20/0x30
                      [<ffffffff8100309b>] system_call_fastpath+0x16/0x1b
 }
 ... key      at: [<ffffffff82533fb8>] __key.30033+0x0/0x8
 ... acquired at:
  [<ffffffff81087263>] check_usage_backwards+0x93/0x100
  [<ffffffff81087b9a>] mark_lock+0x1ca/0x420
  [<ffffffff81087e63>] mark_held_locks+0x73/0xa0
  [<ffffffff810880bb>] trace_hardirqs_on_caller+0x7b/0x1c0
  [<ffffffff8108820d>] trace_hardirqs_on+0xd/0x10
  [<ffffffff81423730>] _raw_write_unlock_irq+0x30/0x60
  [<ffffffff8114cc63>] f_modown+0x53/0xe0
  [<ffffffff8114cd1e>] __f_setown+0xe/0x20
  [<ffffffff8127c667>] tty_fasync+0x107/0x190
  [<ffffffff8114d842>] sys_fcntl+0x222/0x580
  [<ffffffff8100309b>] system_call_fastpath+0x16/0x1b


stack backtrace:
Pid: 1609, comm: emacs Not tainted 2.6.33-rc5 #77
Call Trace:
 [<ffffffff810870bd>] print_irq_inversion_bug.clone.0+0x12d/0x140
 [<ffffffff810871d0>] ? check_usage_backwards+0x0/0x100
 [<ffffffff81087263>] check_usage_backwards+0x93/0x100
 [<ffffffff8114cc4c>] ? f_modown+0x3c/0xe0
 [<ffffffff81087b9a>] mark_lock+0x1ca/0x420
 [<ffffffff81087e63>] mark_held_locks+0x73/0xa0
 [<ffffffff81423730>] ? _raw_write_unlock_irq+0x30/0x60
 [<ffffffff810880bb>] trace_hardirqs_on_caller+0x7b/0x1c0
 [<ffffffff8108820d>] trace_hardirqs_on+0xd/0x10
 [<ffffffff81423730>] _raw_write_unlock_irq+0x30/0x60
 [<ffffffff8114cc63>] f_modown+0x53/0xe0
 [<ffffffff8114cd1e>] __f_setown+0xe/0x20
 [<ffffffff8127c667>] tty_fasync+0x107/0x190
 [<ffffffff8114d842>] sys_fcntl+0x222/0x580
 [<ffffffff8100309b>] system_call_fastpath+0x16/0x1b

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

#2 lore raw
Quoted 17 lines
Quoted 15 lines · KOSAKI Motohiro

Hi

Current linus tree made following lockdep warning when starting emacs command. Is this known issue?

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

Quoted 7 lines
Quoted 3 lines

and interrupts could create inverse lock ordering between them.

Hey,

does reverting commit 703625118 help?

Seems solved.

Thanks.

#3 lore raw
Quoted 21 lines
Quoted 21 lines
Quoted 18 lines · KOSAKI Motohiro

Hi

Current linus tree made following lockdep warning when starting emacs command. Is this known issue?

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

and interrupts could create inverse lock ordering between them.

Quoted 7 lines
Quoted 3 lines

Hey,

does reverting commit 703625118 help?

Seems solved.

Thanks.

I'm sorry.
I forgot to cc related person at last mail.

Greg, can you please consider revert commit 703625118?

#4 lore raw
Quoted 25 lines · KOSAKI Motohiro
Quoted 25 lines
Quoted 24 lines
Quoted 15 lines · KOSAKI Motohiro

Hi

Current linus tree made following lockdep warning when starting emacs command. Is this known issue?

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

Quoted 3 lines

and interrupts could create inverse lock ordering between them.

Hey,

does reverting commit 703625118 help?

Quoted 9 lines
Quoted 3 lines

Seems solved.

Thanks.

I'm sorry.
I forgot to cc related person at last mail.

Greg, can you please consider revert commit 703625118?

I agree, it seems that patch is useless, since we already do lock_kernel() before calling __f_setown()...

maintainer
#5 lore raw
Quoted 2 lines · Am??rico Wang

I agree, it seems that patch is useless, since we already do lock_kernel() before calling __f_setown()...

What's to prevent pid from being freed under us? BKL won't...

#6 lore raw
Quoted 7 lines · Al Viro
Quoted 2 lines · Am??rico Wang

I agree, it seems that patch is useless, since we already do lock_kernel() before calling __f_setown()...

What's to prevent pid from being freed under us?  BKL won't...

Hmm, I don't fully understand the race here. If it is used to protect 'pid' which we get from 'tty->pgrp' or 'current', in the former case, it is protected by 'tty->ctrl_lock', in the later case, it doesn't need the lock. So it's not necessary to protect 'pid' by 'tty->crtl_lock'.

Am i missing something?

maintainer
#7 lore raw
Quoted 6 lines · Am??rico Wang
Quoted 5 lines · Al Viro
Quoted 2 lines · Am??rico Wang

I agree, it seems that patch is useless, since we already do lock_kernel() before calling __f_setown()...

Quoted 6 lines
Quoted 2 lines

What's to prevent pid from being freed under us? ??BKL won't...

Hmm, I don't fully understand the race here. If it is used to protect 'pid' which we get from 'tty->pgrp' or 'current', in the former case, it is protected by 'tty->ctrl_lock', in the later case, it doesn't need

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Without that patch it isn't.

Quoted 1 line

the lock. So it's not necessary to protect 'pid' by 'tty->crtl_lock'.

#8 lore raw
In reply to Al Viro

Hi

Quoted 6 lines · Am??rico Wang
Quoted 3 lines

I agree, it seems that patch is useless, since we already do lock_kernel() before calling __f_setown()...

What's to prevent pid from being freed under us? BKL won't...

I don't understand this issue at all. so, this is stupid dumb question. Why can't we write following code?

                enum pid_type type;
                struct pid *pid;
                if (!waitqueue_active(&tty->read_wait))
                        tty->minimum_to_wake = 1;
                spin_lock_irqsave(&tty->ctrl_lock, flags);
                if (tty->pgrp) {
                        pid = tty->pgrp;
                        type = PIDTYPE_PGID;
                } else {
                        pid = task_pid(current);
                        type = PIDTYPE_PID;
                }
		get_pid(pid)					// insert here
                spin_unlock_irqrestore(&tty->ctrl_lock, flags);
                retval = __f_setown(filp, pid, type, 0);
		put_pid(pid)					// insert here
#9 lore raw

On Tue, Jan 26, 2010 at 3:45 PM, KOSAKI Motohiro<[email protected]> wrote:> Hi>>> On Tue, Jan 26, 2010 at 02:01:12PM +0800, Am??rico Wang wrote:>>>> > I agree, it seems that patch is useless, since we already>> > do lock_kernel() before calling __f_setown()...>>>> What's to prevent pid from being freed under us?  BKL won't...>> I don't understand this issue at all. so, this is stupid dumb question.> Why can't we write following code?>>>                enum pid_type type;>                struct pid *pid;>                if (!waitqueue_active(&tty->read_wait))>                        tty->minimum_to_wake = 1;>                spin_lock_irqsave(&tty->ctrl_lock, flags);>                if (tty->pgrp) {>                        pid = tty->pgrp;>                        type = PIDTYPE_PGID;>                } else {>                        pid = task_pid(current);>                        type = PIDTYPE_PID;>                }>                get_pid(pid)                                    // insert here>                spin_unlock_irqrestore(&tty->ctrl_lock, flags);>                retval = __f_setown(filp, pid, type, 0);>                put_pid(pid)                                    // insert here> Yeah, this seems reasonable for me, but not sure if this is the best fix.���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m���� ������?�I�

#10 lore raw
Quoted 33 lines · Américo Wang
Quoted 13 lines · KOSAKI Motohiro

Hi

Quoted 6 lines · Am??rico Wang
Quoted 3 lines

I agree, it seems that patch is useless, since we already do lock_kernel() before calling __f_setown()...

What's to prevent pid from being freed under us?  BKL won't...

I don't understand this issue at all. so, this is stupid dumb question. Why can't we write following code?

Quoted 17 lines

               enum pid_type type;
               struct pid *pid;
               if (!waitqueue_active(&tty->read_wait)) tty->minimum_to_wake = 1; spin_lock_irqsave(&tty->ctrl_lock, flags);
               if (tty->pgrp) {
                       pid = tty->pgrp;
                       type = PIDTYPE_PGID;
               } else {
                       pid = task_pid(current);
                       type = PIDTYPE_PID;
               }
               get_pid(pid)                                    // insert here spin_unlock_irqrestore(&tty->ctrl_lock, flags); retval = __f_setown(filp, pid, type, 0); put_pid(pid)                                    // insert here

Quoted 1 line

Yeah, this seems reasonable for me, but not sure if this is the best fix.

That or tweak __f_setown to use irqsave/irqrestore variants for it's locks, __f_setown is already atomic. I prefer that direction because the code is just a little simpler.

Eric

���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m���� ������?�I�

#11 lore raw
Quoted 35 lines · Eric W. Biederman
Quoted 33 lines · Américo Wang
Quoted 30 lines · KOSAKI Motohiro

Hi

Quoted 6 lines · Am??rico Wang
Quoted 3 lines

I agree, it seems that patch is useless, since we already do lock_kernel() before calling __f_setown()...

What's to prevent pid from being freed under us?  BKL won't...

I don't understand this issue at all. so, this is stupid dumb question. Why can't we write following code?

               enum pid_type type;
               struct pid *pid;
               if (!waitqueue_active(&tty->read_wait)) tty->minimum_to_wake = 1; spin_lock_irqsave(&tty->ctrl_lock, flags);
               if (tty->pgrp) {
                       pid = tty->pgrp;
                       type = PIDTYPE_PGID;
               } else {
                       pid = task_pid(current);
                       type = PIDTYPE_PID;
               }
               get_pid(pid)                                    // insert here spin_unlock_irqrestore(&tty->ctrl_lock, flags); retval = __f_setown(filp, pid, type, 0); put_pid(pid)                                    // insert here

Quoted 6 lines
Quoted 1 line

Yeah, this seems reasonable for me, but not sure if this is the best fix.

That or tweak __f_setown to use irqsave/irqrestore variants for it's locks, __f_setown is already atomic.  I prefer that direction because the code is just a little simpler.

Oh, very good advice!

Patch is below.

-------------->
Commit 703625118 causes a lockdep warning:

[ INFO: possible irq lock inversion dependency detected ] 2.6.33-rc5 #77
---------------------------------------------------------
emacs/1609 just changed the state of lock:
(&(&tty->ctrl_lock)->rlock){+.....}, at: [<ffffffff8127c648>]
tty_fasync+0xe8/0x190
but this lock took another, HARDIRQ-unsafe lock in the past:
(&(&sighand->siglock)->rlock){-.....}

This is due to we use write_lock_irq() in __f_setown() which turns the IRQ on in write_unlock_irq(), causes this warning.

Switch it ot write_lock_irqsave() and write_unlock_irqrestore(), as suggested by Eric.

----

fs/fcntl.c +3 -2
index 97e01dc..556b404 100644
@@ -199,7 +199,8 @@ 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);
+	int 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 +212,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,
#12 lore raw
Quoted 76 lines · Américo Wang
Quoted 35 lines · Eric W. Biederman
Quoted 33 lines · Américo Wang
Quoted 30 lines · KOSAKI Motohiro

Hi

Quoted 6 lines · Am??rico Wang
Quoted 3 lines

I agree, it seems that patch is useless, since we already do lock_kernel() before calling __f_setown()...

What's to prevent pid from being freed under us?  BKL won't...

I don't understand this issue at all. so, this is stupid dumb question. Why can't we write following code?

               enum pid_type type;
               struct pid *pid;
               if (!waitqueue_active(&tty->read_wait)) tty->minimum_to_wake = 1; spin_lock_irqsave(&tty->ctrl_lock, flags);
               if (tty->pgrp) {
                       pid = tty->pgrp;
                       type = PIDTYPE_PGID;
               } else {
                       pid = task_pid(current);
                       type = PIDTYPE_PID;
               }
               get_pid(pid)                                    // insert here spin_unlock_irqrestore(&tty->ctrl_lock, flags); retval = __f_setown(filp, pid, type, 0); put_pid(pid)                                    // insert here

Quoted 6 lines
Quoted 1 line

Yeah, this seems reasonable for me, but not sure if this is the best fix.

That or tweak __f_setown to use irqsave/irqrestore variants for it's locks, __f_setown is already atomic.  I prefer that direction because the code is just a little simpler.

Oh, very good advice!

Patch is below.

-------------->
Commit 703625118 causes a lockdep warning:

[ INFO: possible irq lock inversion dependency detected ] 2.6.33-rc5 #77
---------------------------------------------------------
emacs/1609 just changed the state of lock:
(&(&tty->ctrl_lock)->rlock){+.....}, at: [<ffffffff8127c648>]
tty_fasync+0xe8/0x190
but this lock took another, HARDIRQ-unsafe lock in the past:
(&(&sighand->siglock)->rlock){-.....}

This is due to we use write_lock_irq() in __f_setown() which turns the IRQ on in write_unlock_irq(), causes this warning.

Switch it ot write_lock_irqsave() and write_unlock_irqrestore(), as suggested by Eric.

----

fs/fcntl.c +0 -0
index 97e01dc..556b404 100644

fs/fcntl.c static int setfl(int fd, struct file * filp, unsigned long arg) · line 202

Quoted 5 lines
fs/fcntl.c +1 -1
@@ -199,7 +199,8 @@ 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);
+	int flags;

Minor nit. This should be "unsigned long flags;"

fs/fcntl.c static void f_modown(struct file *filp, struct pid *pid, enum pid_type type, · line 218

Quoted 13 lines
fs/fcntl.c +2 -1
+	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 +212,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,

Eric ���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m���� ������?�I�

#13 lore raw

PATCH 1/1 fix the lockdep warning in tty_fasync()

Quoted 33 lines · Eric W. Biederman
Quoted 33 lines
Quoted 5 lines

That or tweak __f_setown to use irqsave/irqrestore variants for it's locks, __f_setown is already atomic.  I prefer that direction because the code is just a little simpler.

Oh, very good advice!

Patch is below.

-------------->
Commit 703625118 causes a lockdep warning:

[ INFO: possible irq lock inversion dependency detected ] 2.6.33-rc5 #77
---------------------------------------------------------
emacs/1609 just changed the state of lock:
(&(&tty->ctrl_lock)->rlock){+.....}, at: [<ffffffff8127c648>]
tty_fasync+0xe8/0x190
but this lock took another, HARDIRQ-unsafe lock in the past:
(&(&sighand->siglock)->rlock){-.....}

This is due to we use write_lock_irq() in __f_setown() which turns the IRQ on in write_unlock_irq(), causes this warning.

Switch it ot write_lock_irqsave() and write_unlock_irqrestore(), as suggested by Eric.

----

Quoted 13 lines

fs/fcntl.c static int setfl(int fd, struct file * filp, unsigned long arg) · line 202

Quoted 10 lines
fs/fcntl.c +1 -1
index 97e01dc..556b404 100644
@@ -199,7 +199,8 @@ 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);
+	int flags;

Minor nit. This should be "unsigned long flags;"

Right... Below is an updated version.

Thanks.

------------>

Commit 703625118 causes a lockdep warning:

[ INFO: possible irq lock inversion dependency detected ] 2.6.33-rc5 #77
---------------------------------------------------------
emacs/1609 just changed the state of lock:
(&(&tty->ctrl_lock)->rlock){+.....}, at: [<ffffffff8127c648>]
tty_fasync+0xe8/0x190
but this lock took another, HARDIRQ-unsafe lock in the past:
(&(&sighand->siglock)->rlock){-.....}

This is due to we use write_lock_irq() in __f_setown() which turns the IRQ on in write_unlock_irq(), causes this warning.

Switch it to write_lock_irqsave() and write_unlock_irqrestore(), as suggested by Eric.

---

fs/fcntl.c +3 -2
index 97e01dc..82cc8a7 100644
@@ -199,7 +199,8 @@ 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 +212,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,
#14 lore raw

Re: [Patch] fix the lockdep warning in tty_fasync()

Quoted 37 lines

Commit 703625118 causes a lockdep warning:

[ INFO: possible irq lock inversion dependency detected ] 2.6.33-rc5 #77
---------------------------------------------------------
emacs/1609 just changed the state of lock:
(&(&tty->ctrl_lock)->rlock){+.....}, at: [<ffffffff8127c648>]
tty_fasync+0xe8/0x190
but this lock took another, HARDIRQ-unsafe lock in the past:
(&(&sighand->siglock)->rlock){-.....}

This is due to we use write_lock_irq() in __f_setown() which turns the IRQ on in write_unlock_irq(), causes this warning.

Switch it to write_lock_irqsave() and write_unlock_irqrestore(), as suggested by Eric.

---

fs/fcntl.c +2 -1
index 97e01dc..82cc8a7 100644
@@ -199,7 +199,8 @@ 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);

fs/fcntl.c static void f_modown(struct file *filp, struct pid *pid, enum pid_type type, · line 218

Quoted 8 lines
fs/fcntl.c +1 -1
@@ -211,7 +212,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,

I've confirmed the warning disappear :)

Thanks.

#15 lore raw

Re: [Patch] fix the lockdep warning in tty_fasync()

Quoted 44 lines · KOSAKI Motohiro
Quoted 44 lines

Commit 703625118 causes a lockdep warning:

[ INFO: possible irq lock inversion dependency detected ] 2.6.33-rc5 #77
---------------------------------------------------------
emacs/1609 just changed the state of lock:
(&(&tty->ctrl_lock)->rlock){+.....}, at: [<ffffffff8127c648>]
tty_fasync+0xe8/0x190
but this lock took another, HARDIRQ-unsafe lock in the past:
(&(&sighand->siglock)->rlock){-.....}

This is due to we use write_lock_irq() in __f_setown() which turns the IRQ on in write_unlock_irq(), causes this warning.

Switch it to write_lock_irqsave() and write_unlock_irqrestore(), as suggested by Eric.

---

fs/fcntl.c +3 -2
index 97e01dc..82cc8a7 100644
@@ -199,7 +199,8 @@ 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 +212,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);
 }
 
Quoted 3 lines

fs/fcntl.c static void f_modown(struct file *filp, struct pid *pid, enum pid_type type, · line 218

Quoted 1 line
fs/fcntl.c +0 -0
 int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,

I've confirmed the warning disappear :)

Great, I did the same fix, it's now commit id b04da8bfdfbbd79544cab2fadfdc12e87eb01600 in Linus's tree.

thanks for testing,

greg k-h

#16 lore raw
In reply to KOSAKI Motohiro
Quoted 25 lines · KOSAKI Motohiro
Quoted 25 lines
Quoted 24 lines
Quoted 15 lines · KOSAKI Motohiro

Hi

Current linus tree made following lockdep warning when starting emacs command. Is this known issue?

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

Quoted 3 lines

and interrupts could create inverse lock ordering between them.

Hey,

does reverting commit 703625118 help?

Quoted 8 lines
Quoted 3 lines

Seems solved.

Thanks.

I'm sorry.
I forgot to cc related person at last mail.

Greg, can you please consider revert commit 703625118?

It looks like f_modown needs to do irqsave irqrestore to be safely called in this context. My apologies for missing this when I originally made the suggestion.

As for the other comments I would be very surprised if lock_kernel() offers any real protection.

I really don't understand what it is talking about siglock being irq unsafe, that seems wrong on oh so many levels.

Eric

n

#17 lore raw

check_usage_backwards() && forwards? (Was: [2.6.33-rc5] starting emacs makes lockdep warning)

In reply to KOSAKI Motohiro

(add lockdep gurus)

Lockdep has found the real bug, but the output doesn't look right to me

Quoted 9 lines · KOSAKI Motohiro

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

"HARDIRQ-unsafe" and "this lock took another" looks wrong, afaics.

Quoted 7 lines

... key at: [<ffffffff81c054a4>] __key.46539+0x0/0x8
... acquired at:

   [<ffffffff81089af6>] __lock_acquire+0x1056/0x15a0
   [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
   [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
   [<ffffffff8127c1be>] __proc_set_tty+0x3e/0x150
   [<ffffffff8127e01d>] tty_open+0x51d/0x5e0

The stack-trace shows that this lock (ctrl_lock) was taken under ->siglock (which is hopefully irq-safe).

Typo in check_usage_backwards() ?

Oleg.

kernel/lockdep.c +1 -1
@@ -2147,7 +2147,7 @@ check_usage_backwards(struct task_struct
 		return ret;
 
 	return print_irq_inversion_bug(curr, &root, target_entry,
-					this, 1, irqclass);
+					this, 0, irqclass);
 }
 
 void print_irqtrace_events(struct task_struct *curr)
#18 lore raw

Re: check_usage_backwards() && forwards? (Was: [2.6.33-rc5] starting emacs makes lockdep warning)

Quoted 25 lines · Oleg Nesterov

(add lockdep gurus)

Lockdep has found the real bug, but the output doesn't look right to me

Quoted 9 lines · KOSAKI Motohiro

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

"HARDIRQ-unsafe" and "this lock took another" looks wrong, afaics.

Quoted 7 lines

... key at: [<ffffffff81c054a4>] __key.46539+0x0/0x8
... acquired at:

   [<ffffffff81089af6>] __lock_acquire+0x1056/0x15a0
   [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
   [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
   [<ffffffff8127c1be>] __proc_set_tty+0x3e/0x150
   [<ffffffff8127e01d>] tty_open+0x51d/0x5e0
Quoted 4 lines

The stack-trace shows that this lock (ctrl_lock) was taken under ->siglock (which is hopefully irq-safe).

Typo in check_usage_backwards() ?

Yes, very much so, thanks!

Quoted 14 lines

Oleg.

kernel/lockdep.c +1 -1
@@ -2147,7 +2147,7 @@ check_usage_backwards(struct task_struct
 		return ret;
 
 	return print_irq_inversion_bug(curr, &root, target_entry,
-					this, 1, irqclass);
+					this, 0, irqclass);
 }
 
 void print_irqtrace_events(struct task_struct *curr)
#19 lore raw

Re: check_usage_backwards() && forwards? (Was: [2.6.33-rc5] starting emacs makes lockdep warning)

In reply to Oleg Nesterov
Quoted 37 lines · Oleg Nesterov

(add  lockdep gurus)

Lockdep has found the real bug, but the output doesn't look right to me

Quoted 9 lines · KOSAKI Motohiro

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

"HARDIRQ-unsafe" and "this lock took another" looks wrong, afaics.

Quoted 7 lines

  ... key      at: [<ffffffff81c054a4>] __key.46539+0x0/0x8 ... acquired at:

   [<ffffffff81089af6>] __lock_acquire+0x1056/0x15a0
   [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
   [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
   [<ffffffff8127c1be>] __proc_set_tty+0x3e/0x150
   [<ffffffff8127e01d>] tty_open+0x51d/0x5e0

The stack-trace shows that this lock (ctrl_lock) was taken under ->siglock (which is hopefully irq-safe).

Typo in check_usage_backwards() ?

Oleg.

kernel/lockdep.c check_usage_backwards(struct task_struct · line 2153

Quoted 8 lines
kernel/lockdep.c +1 -1
@@ -2147,7 +2147,7 @@ check_usage_backwards(struct task_struct
                return ret;
 
        return print_irq_inversion_bug(curr, &root, target_entry,
-                                       this, 1, irqclass);
+                                       this, 0, irqclass);
  }
 
  void print_irqtrace_events(struct task_struct *curr)

Yes!! Almost definitely... You are so careful!
ACK, please submit it as a normal patch.

Thanks.

#20 lore raw

[tip:core/urgent] lockdep: Fix check_usage_backwards() error message

In reply to Oleg Nesterov
Commit-ID:  48d50674179981e41f432167b2441cec782d5484
Gitweb:     http://git.kernel.org/tip/48d50674179981e41f432167b2441cec782d5484
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Tue, 26 Jan 2010 19:16:41 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 27 Jan 2010 08:34:02 +0100

lockdep: Fix check_usage_backwards() error message

Lockdep has found the real bug, but the output doesn't look right to me:

Quoted 8 lines

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

"HARDIRQ-unsafe" and "this lock took another" looks wrong, afaics.

Quoted 7 lines

... key at: [<ffffffff81c054a4>] __key.46539+0x0/0x8
... acquired at:

   [<ffffffff81089af6>] __lock_acquire+0x1056/0x15a0
   [<ffffffff8108a0df>] lock_acquire+0x9f/0x120
   [<ffffffff81423012>] _raw_spin_lock_irqsave+0x52/0x90
   [<ffffffff8127c1be>] __proc_set_tty+0x3e/0x150
   [<ffffffff8127e01d>] tty_open+0x51d/0x5e0

The stack-trace shows that this lock (ctrl_lock) was taken under ->siglock (which is hopefully irq-safe).

This is a clear typo in check_usage_backwards() where we tell the print a fancy routine we're forwards.

LKML-Reference: <[email protected]>

---
 kernel/lockdep.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/lockdep.c +1 -1
index 5feaddc..c62ec14 100644
@@ -2147,7 +2147,7 @@ check_usage_backwards(struct task_struct *curr, struct held_lock *this,
 		return ret;
 
 	return print_irq_inversion_bug(curr, &root, target_entry,
-					this, 1, irqclass);
+					this, 0, irqclass);
 }
 
 void print_irqtrace_events(struct task_struct *curr)