BUG: spinlock recursion (sys_chdir, user_path_at, do_path_lookup ...)

Hi,

I picked this mail from google so I might be missing some recipients.

On Wed, 12 Jan 2011, Russell King - ARM Linux wrote:

On Wed, Jan 12, 2011 at 12:35:08PM +0000, Russell King - ARM Linux wrote:

quoted

ARM doesn't implement save_stack_trace_regs() nor save_stack_trace_bp()
so if the compiler referenced these, you'd have a kernel which doesn't
link.  The only places that this symbol appears is:

arch/x86/kernel/stacktrace.c:void save_stack_trace_regs(struct stack_trace *trac
arch/x86/mm/kmemcheck/error.c:  save_stack_trace_regs(&e->trace, regs);
include/linux/stacktrace.h:extern void save_stack_trace_regs(struct stack_trace

So, if this is where your bisect decided was the problem, your bisect
was faulty.

BTW, a useful thing to do after a bisect is to return to the point in
the history where you first noticed the regression (so Linus' tip,
your tip, or whatever).  Then try reverting the commit which git bisect
_thinks_ is the cause of your problem and re-test that.

If the problem is fixed, you have greater confidence that the commit is
the problem.

Reverting this commit (9c0729dc8062bed96189bd14ac6d4920f3958743 )
didn't improve in my case.

If it made no difference, then you know that something else (maybe in
combination) is causing the problem.

I tried to narrow it down using the dump and another thread mentioning
recent changes from "Nick" (might be Nick Piggin).

Reverting: fs: rcu-walk aware d_revalidate method
commit: 34286d6662308d82aed891852d04c7c3a2649b16

Seems to get rid of the bug, hopefully it will give more information
to someone more experienced with this code (than me).

Regards,

Omar

Hello Omar,

On Wed, Jan 12, 2011 at 02:59:39PM -0600, Ramirez Luna, Omar wrote:

I picked this mail from google so I might be missing some recipients.

On Wed, 12 Jan 2011, Russell King - ARM Linux wrote:

quoted

On Wed, Jan 12, 2011 at 12:35:08PM +0000, Russell King - ARM Linux wrote:

quoted

ARM doesn't implement save_stack_trace_regs() nor save_stack_trace_bp()
so if the compiler referenced these, you'd have a kernel which doesn't
link.  The only places that this symbol appears is:

arch/x86/kernel/stacktrace.c:void save_stack_trace_regs(struct stack_trace *trac
arch/x86/mm/kmemcheck/error.c:  save_stack_trace_regs(&e->trace, regs);
include/linux/stacktrace.h:extern void save_stack_trace_regs(struct stack_trace

So, if this is where your bisect decided was the problem, your bisect
was faulty.

BTW, a useful thing to do after a bisect is to return to the point in
the history where you first noticed the regression (so Linus' tip,
your tip, or whatever).  Then try reverting the commit which git bisect
_thinks_ is the cause of your problem and re-test that.

If the problem is fixed, you have greater confidence that the commit is
the problem.

Reverting this commit (9c0729dc8062bed96189bd14ac6d4920f3958743 )
didn't improve in my case.

Yeah, my bisect was screwed because I somehow changed .config in the
middle ...

You're on ARM, too?

quoted

If it made no difference, then you know that something else (maybe in
combination) is causing the problem.

I tried to narrow it down using the dump and another thread mentioning
recent changes from "Nick" (might be Nick Piggin).

Reverting: fs: rcu-walk aware d_revalidate method
commit: 34286d6662308d82aed891852d04c7c3a2649b16

I found that one, too, in the meantime.  Currently debugging that with
tglx on irc.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Hi Uwe,

2011/1/12 Uwe Kleine-K?nig [off-list ref]:

You're on ARM, too?

Yes, ARMv7 (zoom2).

quoted

quoted

If it made no difference, then you know that something else (maybe in
combination) is causing the problem.

I tried to narrow it down using the dump and another thread mentioning
recent changes from "Nick" (might be Nick Piggin).

Reverting: fs: rcu-walk aware d_revalidate method
commit: 34286d6662308d82aed891852d04c7c3a2649b16

I found that one, too, in the meantime. ?Currently debugging that with
tglx on irc.

OK, let me know if I can help.

Regards,

Omar

On Wed, 12 Jan 2011, Uwe Kleine-K?nig wrote:

quoted

Reverting: fs: rcu-walk aware d_revalidate method
commit: 34286d6662308d82aed891852d04c7c3a2649b16

I found that one, too, in the meantime.  Currently debugging that with
tglx on irc.

The last finding is that parent and dentry in
nameidata_dentry_drop_rcu() are the same, which explains the lock
recursion nicely. 

@nick: Anything you want us to add to the debugging ?

@peterz: Why does lockdep ignore the lock recursion in that
	 spin_lock_nested() call?

Thanks,

	tglx

Hi Nick,

On Wed, Jan 12, 2011 at 11:52:01PM +0100, Thomas Gleixner wrote:

On Wed, 12 Jan 2011, Uwe Kleine-K?nig wrote:

quoted

quoted

Reverting: fs: rcu-walk aware d_revalidate method
commit: 34286d6662308d82aed891852d04c7c3a2649b16

I found that one, too, in the meantime.  Currently debugging that with
tglx on irc.

The last finding is that parent and dentry in
nameidata_dentry_drop_rcu() are the same, which explains the lock
recursion nicely. 

@nick: Anything you want us to add to the debugging ?

If that helps, the chdir is to / (probably while being already in /).
Maybe the problem is that "/." == "/.."?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Wed, 2011-01-12 at 23:52 +0100, Thomas Gleixner wrote:

On Wed, 12 Jan 2011, Uwe Kleine-K?nig wrote:

quoted

quoted

Reverting: fs: rcu-walk aware d_revalidate method
commit: 34286d6662308d82aed891852d04c7c3a2649b16

I found that one, too, in the meantime.  Currently debugging that with
tglx on irc.

The last finding is that parent and dentry in
nameidata_dentry_drop_rcu() are the same, which explains the lock
recursion nicely. 

@nick: Anything you want us to add to the debugging ?

@peterz: Why does lockdep ignore the lock recursion in that
         spin_lock_nested() call?

$ git show 34286d6662308d82aed891852d04c7c3a2649b16 | grep spin_lock_nested | wc -l
0

Uhm, whot?!

On Wed, 2011-01-12 at 23:52 +0100, Thomas Gleixner wrote:

quoted

@peterz: Why does lockdep ignore the lock recursion in that
         spin_lock_nested() call?

So after some hints on IRC on where to look:

<tglx>         spin_lock(&parent->d_lock);
<tglx>         spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
<tglx> if parent == dentry

That won't yell because you explicitly tell lockdep its ok, I know what
I'm doing.

Several lockdep annotations (including this one) allow you to annotate
real bugs away, hence you really need to be sure about things when you
make them.

On Thu, 13 Jan 2011, Peter Zijlstra wrote:

quoted

On Wed, 2011-01-12 at 23:52 +0100, Thomas Gleixner wrote:

quoted

quoted

@peterz: Why does lockdep ignore the lock recursion in that
         spin_lock_nested() call?

So after some hints on IRC on where to look:

<tglx>         spin_lock(&parent->d_lock);
<tglx>         spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
<tglx> if parent == dentry

That won't yell because you explicitly tell lockdep its ok, I know what
I'm doing.

Several lockdep annotations (including this one) allow you to annotate
real bugs away, hence you really need to be sure about things when you
make them.

Yeah, I suspected that, but checking whether the pointers are same
would be nice as it would tell us right away where we fcked up :)

On Thu, 2011-01-13 at 12:21 +0100, Thomas Gleixner wrote:

On Thu, 13 Jan 2011, Peter Zijlstra wrote:

quoted

quoted

On Wed, 2011-01-12 at 23:52 +0100, Thomas Gleixner wrote:

quoted

quoted

@peterz: Why does lockdep ignore the lock recursion in that
         spin_lock_nested() call?

So after some hints on IRC on where to look:

<tglx>         spin_lock(&parent->d_lock);
<tglx>         spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
<tglx> if parent == dentry

That won't yell because you explicitly tell lockdep its ok, I know what
I'm doing.

Several lockdep annotations (including this one) allow you to annotate
real bugs away, hence you really need to be sure about things when you
make them.

Yeah, I suspected that, but checking whether the pointers are same
would be nice as it would tell us right away where we fcked up :)

Something like the below would indeed do that, but it makes the
lock_acquire path more expensive, since it will now have to iterate the
held lock stack every time.

(not actually tested)

---
 kernel/lockdep.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 42ba65d..d053d9a 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c

@@ -2740,11 +2740,12 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 {
 	struct task_struct *curr = current;
 	struct lock_class *class = NULL;
-	struct held_lock *hlock;
+	struct held_lock *hlock, *rhlock;
 	unsigned int depth, id;
 	int chain_head = 0;
 	int class_idx;
 	u64 chain_key;
+	int i;
 
 	if (!prove_locking)
 		check = 1;

@@ -2817,6 +2818,21 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	hlock->holdtime_stamp = lockstat_clock();
 #endif
 
+	for (i = depth-1; i >= 0; i--) {
+		rhlock = curr->held_locks + i;
+		if (rhlock->instance == lock) {
+			if (debug_locks_off() || debug_locks_silent)
+				return 0;
+			printk("Lock recursion, trying to acquire:\n");
+			print_lock(hlock);
+			printk("while already holding:\n");
+			print_lock(rhlock);
+			printk("which is the same lock instance!\n");
+			dump_stack();
+			return 0;
+		}
+	}
+
 	if (check == 2 && !mark_irqflags(curr, hlock))
 		return 0;