[PATCH] lockdep: Check if nested lock is actually held

[PATCH] lockdep: Check if nested lock is actually held

[Maarten Lankhorst]

It is considered good form to lock the lock you claim to be nested in.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

---
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index ea9ee45..7175447 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -2998,6 +2998,43 @@ EXPORT_SYMBOL_GPL(lockdep_init_map);
 
 struct lock_class_key __lockdep_no_validate__;
 
+static int
+print_lock_nested_lock_not_held(struct task_struct *curr,
+				struct held_lock *lock,
+				struct lockdep_map *nest,
+				unsigned long ip)
+{
+	if (!debug_locks_off())
+		return 0;
+	if (debug_locks_silent)
+		return 0;
+
+	printk("\n");
+	printk("==================================\n");
+	printk("[ BUG: Nested lock was not taken ]\n");
+	print_kernel_ident();
+	printk("----------------------------------\n");
+
+	printk("%s/%d is trying to lock:\n", curr->comm, task_pid_nr(curr));
+	print_lock(lock);
+
+	printk("\nbut this task is not holding:\n");
+	printk("%s\n", nest->name);
+
+	printk("\nstack backtrace:\n");
+	dump_stack();
+
+	printk("\nother info that might help us debug this:\n");
+	lockdep_print_held_locks(curr);
+
+	printk("\nstack backtrace:\n");
+	dump_stack();
+
+	return 0;
+}
+
+static int __lock_is_held(struct lockdep_map *lock);
+
 /*
  * This gets called for every mutex_lock*()/spin_lock*() operation.
  * We maintain the dependency maps and validate the locking attempt:
@@ -3139,6 +3176,10 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	}
 	chain_key = iterate_chain_key(chain_key, id);
 
+	if (nest_lock && !__lock_is_held(nest_lock))
+		return print_lock_nested_lock_not_held(curr, hlock,
+						       nest_lock, ip);
+
 	if (!validate_chain(curr, lock, hlock, chain_head, chain_key))
 		return 0;
 

Re: [PATCH] lockdep: Check if nested lock is actually held

[Peter Zijlstra]

On Thu, 2012-09-13 at 11:39 +0200, Maarten Lankhorst wrote:
> It is considered good form to lock the lock you claim to be nested in.

Uhm yeah.. cute. You actually found a site where this triggered?

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> ---
> diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> index ea9ee45..7175447 100644
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -2998,6 +2998,43 @@ EXPORT_SYMBOL_GPL(lockdep_init_map);
>  
>  struct lock_class_key __lockdep_no_validate__;
>  
> +static int
> +print_lock_nested_lock_not_held(struct task_struct *curr,
> +				struct held_lock *lock,
> +				struct lockdep_map *nest,
> +				unsigned long ip)
> +{
> +	if (!debug_locks_off())
> +		return 0;
> +	if (debug_locks_silent)
> +		return 0;
> +
> +	printk("\n");
> +	printk("==================================\n");
> +	printk("[ BUG: Nested lock was not taken ]\n");
> +	print_kernel_ident();
> +	printk("----------------------------------\n");
> +
> +	printk("%s/%d is trying to lock:\n", curr->comm, task_pid_nr(curr));
> +	print_lock(lock);
> +
> +	printk("\nbut this task is not holding:\n");
> +	printk("%s\n", nest->name);
> +
> +	printk("\nstack backtrace:\n");
> +	dump_stack();
> +
> +	printk("\nother info that might help us debug this:\n");
> +	lockdep_print_held_locks(curr);
> +
> +	printk("\nstack backtrace:\n");
> +	dump_stack();
> +
> +	return 0;
> +}
> +
> +static int __lock_is_held(struct lockdep_map *lock);
> +
>  /*
>   * This gets called for every mutex_lock*()/spin_lock*() operation.
>   * We maintain the dependency maps and validate the locking attempt:
> @@ -3139,6 +3176,10 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
>  	}
>  	chain_key = iterate_chain_key(chain_key, id);
>  
> +	if (nest_lock && !__lock_is_held(nest_lock))
> +		return print_lock_nested_lock_not_held(curr, hlock,
> +						       nest_lock, ip);

At this time we've already set hlock->nest_lock, so I've shortened the
argument list here a little.

>  	if (!validate_chain(curr, lock, hlock, chain_head, chain_key))
>  		return 0;
>  
> 

Re: [PATCH] lockdep: Check if nested lock is actually held

[Maarten Lankhorst]

Hey,

Op 13-09-12 11:59, Peter Zijlstra schreef:
> On Thu, 2012-09-13 at 11:39 +0200, Maarten Lankhorst wrote:
>> It is considered good form to lock the lock you claim to be nested in.
> Uhm yeah.. cute. You actually found a site where this triggered?
>
Not in mainline, I was working on some lockdep annotations for my work on
moving ttm reservations to base kernel, and I wrote a whole bunch of tests
to stress interaction between reservations and locks, one of the tests I
was doing was taking a spinlock without the nested object:

static void reservation_test_fence_nest_unreserved(void)
{
	struct reservation_object o;

	reservation_object_init(&o);

	spin_lock_nest_lock(&o.fence_lock, &o);
	spin_unlock(&o.fence_lock);
}

I would have expected it to fail, and the patch fixed it. As a nice side effect
it also complained about another hack I was doing elsewhere with reservations to
tests for deadlocks, and it forced me to fix it in a slightly less hacky way.

~Maarten

Re: [PATCH] lockdep: Check if nested lock is actually held

[Peter Zijlstra]

On Thu, 2012-09-13 at 12:10 +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 13-09-12 11:59, Peter Zijlstra schreef:
> > On Thu, 2012-09-13 at 11:39 +0200, Maarten Lankhorst wrote:
> >> It is considered good form to lock the lock you claim to be nested in.
> > Uhm yeah.. cute. You actually found a site where this triggered?
> >
> Not in mainline, I was working on some lockdep annotations for my work on
> moving ttm reservations to base kernel, and I wrote a whole bunch of tests
> to stress interaction between reservations and locks, one of the tests I
> was doing was taking a spinlock without the nested object:
> 
> static void reservation_test_fence_nest_unreserved(void)
> {
> 	struct reservation_object o;
> 
> 	reservation_object_init(&o);
> 
> 	spin_lock_nest_lock(&o.fence_lock, &o);
> 	spin_unlock(&o.fence_lock);
> }
> 
> I would have expected it to fail, and the patch fixed it. As a nice side effect
> it also complained about another hack I was doing elsewhere with reservations to
> tests for deadlocks, and it forced me to fix it in a slightly less hacky way.

Nice.. thanks for noticing!

[tip:core/locking] lockdep: Check if nested lock is actually held

[tip-bot for Maarten Lankhorst]

Commit-ID:  d094595078d00b63839d0c5ccb8b184ef242cb45
Gitweb:     http://git.kernel.org/tip/d094595078d00b63839d0c5ccb8b184ef242cb45
Author:     Maarten Lankhorst <maarten.lankhorst@canonical.com>
AuthorDate: Thu, 13 Sep 2012 11:39:51 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 13 Sep 2012 17:00:44 +0200

lockdep: Check if nested lock is actually held

It is considered good form to lock the lock you claim to be nested in.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

[ removed nest_lock arg to print_lock_nested_lock_not_held in favour
  of hlock->nest_lock, also renamed the lock arg to hlock since its
  a held_lock type ]
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/5051A9E7.5040501@canonical.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/lockdep.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index ea9ee45..7981e5b 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -2998,6 +2998,42 @@ EXPORT_SYMBOL_GPL(lockdep_init_map);
 
 struct lock_class_key __lockdep_no_validate__;
 
+static int
+print_lock_nested_lock_not_held(struct task_struct *curr,
+				struct held_lock *hlock,
+				unsigned long ip)
+{
+	if (!debug_locks_off())
+		return 0;
+	if (debug_locks_silent)
+		return 0;
+
+	printk("\n");
+	printk("==================================\n");
+	printk("[ BUG: Nested lock was not taken ]\n");
+	print_kernel_ident();
+	printk("----------------------------------\n");
+
+	printk("%s/%d is trying to lock:\n", curr->comm, task_pid_nr(curr));
+	print_lock(hlock);
+
+	printk("\nbut this task is not holding:\n");
+	printk("%s\n", hlock->nest_lock->name);
+
+	printk("\nstack backtrace:\n");
+	dump_stack();
+
+	printk("\nother info that might help us debug this:\n");
+	lockdep_print_held_locks(curr);
+
+	printk("\nstack backtrace:\n");
+	dump_stack();
+
+	return 0;
+}
+
+static int __lock_is_held(struct lockdep_map *lock);
+
 /*
  * This gets called for every mutex_lock*()/spin_lock*() operation.
  * We maintain the dependency maps and validate the locking attempt:
@@ -3139,6 +3175,9 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	}
 	chain_key = iterate_chain_key(chain_key, id);
 
+	if (nest_lock && !__lock_is_held(nest_lock))
+		return print_lock_nested_lock_not_held(curr, hlock, ip);
+
 	if (!validate_chain(curr, lock, hlock, chain_head, chain_key))
 		return 0;