lockdep: Check if nested lock is actually held
PATCH 1/1 lockdep: Check if nested lock is actually held
It is considered good form to lock the lock you claim to be nested in.
---
kernel/lockdep.c +41 -0
@@ -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 ;
Quoted 1 line · Maarten Lankhorst
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?
Quoted 51 lines
Signed-off-by Maarten Lankhorst <[email protected]>---
kernel/lockdep.c +37
-0@@ -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:
kernel/lockdep.c static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass, · line 3181
Quoted 6 lines
kernel/lockdep.c +3
-0@@ -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.
Quoted 4 lines
kernel/lockdep.c +0
-0if (!validate_chain (curr, lock, hlock, chain_head, chain_key))return 0 ;
Hey,
Op 13-09-12 11:59, Peter Zijlstra schreef:
Quoted 4 lines · Maarten Lankhorst
Quoted 1 line
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
Quoted 22 lines · Maarten Lankhorst
Hey,
Op 13-09-12 11:59, Peter Zijlstra schreef:
Quoted 4 lines · Maarten Lankhorst
Quoted 1 line
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); }
Quoted 3 lines
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!
Commit-ID: d094595078d00b63839d0c5ccb8b184ef242cb45 Gitweb: http://git.kernel.org/tip/d094595078d00b63839d0c5ccb8b184ef242cb45 Author: Maarten Lankhorst <[email protected]> AuthorDate: Thu, 13 Sep 2012 11:39:51 +0200 Committer: Ingo Molnar <[email protected]> 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.
[ 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 ]
--- kernel/lockdep.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 files changed, 39 insertions(+), 0 deletions(-)
kernel/lockdep.c +39 -0
@@ -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 ;