Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[2.6.33-rc5] starting emacs makes lockdep warning

0 views
Skip to first unread message

KOSAKI Motohiro

unread,
Jan 25, 2010, 10:30:01 PM1/25/10
to
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

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

Américo Wang

unread,
Jan 26, 2010, 12:30:01 AM1/26/10
to
On Tue, Jan 26, 2010 at 11:20 AM, KOSAKI Motohiro
<kosaki....@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?

KOSAKI Motohiro

unread,
Jan 26, 2010, 12:40:01 AM1/26/10
to
> On Tue, Jan 26, 2010 at 11:20 AM, KOSAKI Motohiro
> <kosaki....@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.

KOSAKI Motohiro

unread,
Jan 26, 2010, 12:50:01 AM1/26/10
to
> > On Tue, Jan 26, 2010 at 11:20 AM, KOSAKI Motohiro
> > <kosaki....@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?

Américo Wang

unread,
Jan 26, 2010, 1:10:02 AM1/26/10
to
On Tue, Jan 26, 2010 at 1:49 PM, KOSAKI Motohiro
<kosaki....@jp.fujitsu.com> wrote:
>> > On Tue, Jan 26, 2010 at 11:20 AM, KOSAKI Motohiro
>> > <kosaki....@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()...

Al Viro

unread,
Jan 26, 2010, 1:10:02 AM1/26/10
to
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...

Eric W. Biederman

unread,
Jan 26, 2010, 1:20:02 AM1/26/10
to
KOSAKI Motohiro <kosaki....@jp.fujitsu.com> writes:

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

Américo Wang

unread,
Jan 26, 2010, 1:30:02 AM1/26/10
to
On Tue, Jan 26, 2010 at 2:07 PM, Al Viro <vi...@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?

Al Viro

unread,
Jan 26, 2010, 2:00:02 AM1/26/10
to
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 <vi...@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'.

KOSAKI Motohiro

unread,
Jan 26, 2010, 2:50:01 AM1/26/10
to
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

Américo Wang

unread,
Jan 26, 2010, 3:50:02 AM1/26/10
to
On Tue, Jan 26, 2010 at 3:45 PM, KOSAKI Motohiro
<kosaki....@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.

Eric W. Biederman

unread,
Jan 26, 2010, 4:20:02 AM1/26/10
to
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

Américo Wang

unread,
Jan 26, 2010, 4:40:02 AM1/26/10
to

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....@jp.fujitsu.com>
Signed-off-by: WANG Cong <xiyou.w...@gmail.com>

----

fs-fcntl-__f_setown_use-irqsave-lock.diff

Eric W. Biederman

unread,
Jan 26, 2010, 7:40:02 AM1/26/10
to
> 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

Américo Wang

unread,
Jan 26, 2010, 11:00:01 AM1/26/10
to

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....@jp.fujitsu.com>
Signed-off-by: WANG Cong <xiyou.w...@gmail.com>

Cc: "Eric W. Biederman" <ebie...@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,

Oleg Nesterov

unread,
Jan 26, 2010, 1:20:01 PM1/26/10
to
(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)

Peter Zijlstra

unread,
Jan 26, 2010, 1:50:02 PM1/26/10
to
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!

KOSAKI Motohiro

unread,
Jan 26, 2010, 8:10:01 PM1/26/10
to

I've confirmed the warning disappear :)

Thanks.

Greg KH

unread,
Jan 26, 2010, 8:50:01 PM1/26/10
to

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

thanks for testing,

greg k-h

Américo Wang

unread,
Jan 26, 2010, 10:00:01 PM1/26/10
to

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

Thanks.

tip-bot for Oleg Nesterov

unread,
Jan 27, 2010, 8:20:03 AM1/27/10
to
Commit-ID: 48d50674179981e41f432167b2441cec782d5484
Gitweb: http://git.kernel.org/tip/48d50674179981e41f432167b2441cec782d5484
Author: Oleg Nesterov <ol...@redhat.com>
AuthorDate: Tue, 26 Jan 2010 19:16:41 +0100
Committer: Ingo Molnar <mi...@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 <ol...@redhat.com>
Signed-off-by: Peter Zijlstra <a.p.zi...@chello.nl>
LKML-Reference: <20100126181...@redhat.com>
Signed-off-by: Ingo Molnar <mi...@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,

0 new messages