[2.6.33-rc5] starting emacs makes lockdep warning

[2.6.33-rc5] starting emacs makes lockdep warning

[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.


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

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[Américo Wang]

On Tue, Jan 26, 2010 at 11:20 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
> 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.
>
>

Hey,

does reverting commit 703625118 help?



> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[KOSAKI Motohiro]

> On Tue, Jan 26, 2010 at 11:20 AM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> > 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.
> >
> >
> 
> Hey,
> 
> does reverting commit 703625118 help?

Seems solved.

Thanks.

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[KOSAKI Motohiro]

> > On Tue, Jan 26, 2010 at 11:20 AM, KOSAKI Motohiro
> > <kosaki.motohiro@jp.fujitsu.com> wrote:
> > > 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.
> > >
> > >
> > 
> > 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?

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[Américo Wang]

On Tue, Jan 26, 2010 at 1:49 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> > On Tue, Jan 26, 2010 at 11:20 AM, KOSAKI Motohiro
>> > <kosaki.motohiro@jp.fujitsu.com> wrote:
>> > > 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.
>> > >
>> > >
>> >
>> > 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?
>

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

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[Al Viro]

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...

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[Eric W. Biederman]

KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> writes:

>> > On Tue, Jan 26, 2010 at 11:20 AM, KOSAKI Motohiro
>> > <kosaki.motohiro@jp.fujitsu.com> wrote:
>> > > 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.
>> > >
>> > >
>> > 
>> > 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?

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

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[Américo Wang]

On Tue, Jan 26, 2010 at 2:07 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> 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...
>

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?

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[Al Viro]

On Tue, Jan 26, 2010 at 02:24:30PM +0800, Am??rico Wang wrote:
> On Tue, Jan 26, 2010 at 2:07 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > 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...
> >
> 
> 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.

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

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[KOSAKI Motohiro]

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

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[Américo Wang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1640 bytes --]

On Tue, Jan 26, 2010 at 3:45 PM, KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com> 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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[Eric W. Biederman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1942 bytes --]

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

> On Tue, Jan 26, 2010 at 3:45 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> 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.

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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[Américo Wang]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

On Tue, Jan 26, 2010 at 5:14 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Américo Wang <xiyou.wangcong@gmail.com> writes:
>
>> On Tue, Jan 26, 2010 at 3:45 PM, KOSAKI Motohiro
>> <kosaki.motohiro@jp.fujitsu.com> 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.
>
> 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.

Reported-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

----

[-- Attachment #2: fs-fcntl-__f_setown_use-irqsave-lock.diff --]
[-- Type: text/plain, Size: 822 bytes --]

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 97e01dc..556b404 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -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,

Re: [2.6.33-rc5] starting emacs makes lockdep warning

[Eric W. Biederman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 3925 bytes --]

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

> On Tue, Jan 26, 2010 at 5:14 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Américo Wang <xiyou.wangcong@gmail.com> writes:
>>
>>> On Tue, Jan 26, 2010 at 3:45 PM, KOSAKI Motohiro
>>> <kosaki.motohiro@jp.fujitsu.com> 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.
>>
>> 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.
>
> Reported-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
>
> ----
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 97e01dc..556b404 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -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;"

> +	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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[Patch] fix the lockdep warning in tty_fasync()

[Américo Wang]

On Tue, Jan 26, 2010 at 04:33:38AM -0800, Eric W. Biederman wrote:
>>>
>>> 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.
>>
>> Reported-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
>>
>> ----
>>
>> diff --git a/fs/fcntl.c b/fs/fcntl.c
>> index 97e01dc..556b404 100644
>> --- a/fs/fcntl.c
>> +++ b/fs/fcntl.c
>> @@ -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.

Reported-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>

---
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 97e01dc..82cc8a7 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -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,

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

[Oleg Nesterov]

(add  lockdep gurus)

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

On 01/26, KOSAKI Motohiro wrote:
>
> =========================================================
> [ 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.

>   ... 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.

--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -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)

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

[Peter Zijlstra]

On Tue, 2010-01-26 at 19:16 +0100, Oleg Nesterov wrote:
> (add  lockdep gurus)
> 
> Lockdep has found the real bug, but the output doesn't look right to me
> 
> On 01/26, KOSAKI Motohiro wrote:
> >
> > =========================================================
> > [ 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.
> 
> >   ... 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() ?

Yes, very much so, thanks!

> Oleg.
> 
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -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)
> 

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

[KOSAKI Motohiro]

> 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.
> 
> Reported-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> ---
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 97e01dc..82cc8a7 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -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,

I've confirmed the warning disappear :)

Thanks.

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

[Greg KH]

On Wed, Jan 27, 2010 at 10:09:04AM +0900, KOSAKI Motohiro wrote:
> > 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.
> > 
> > Reported-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > 
> > ---
> > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > index 97e01dc..82cc8a7 100644
> > --- a/fs/fcntl.c
> > +++ b/fs/fcntl.c
> > @@ -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,
> 
> 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

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

[Américo Wang]

On Wed, Jan 27, 2010 at 2:16 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> (add  lockdep gurus)
>
> Lockdep has found the real bug, but the output doesn't look right to me
>
> On 01/26, KOSAKI Motohiro wrote:
>>
>> =========================================================
>> [ 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.
>
>>   ... 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.
>
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -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.

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

[tip-bot for Oleg Nesterov]

Commit-ID:  48d50674179981e41f432167b2441cec782d5484
Gitweb:     http://git.kernel.org/tip/48d50674179981e41f432167b2441cec782d5484
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Tue, 26 Jan 2010 19:16:41 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
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:

> =========================================================
> [ 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.

>   ... 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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <20100126181641.GA10460@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/lockdep.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 5feaddc..c62ec14 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -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)