locking/lockdep: Prevent chain_key collisions

The chain_key hashing macro iterate_chain_key(key1, key2) does not
generate a new different value if both key1 and key2 are 0. In that
case the generated value is again 0. This can lead to collisions which
can result in lockdep not detecting deadlocks or circular
dependencies.

Avoid the problem by using class_idx (1-based) instead of class id
(0-based) as an input for the hashing macro 'key2' in
iterate_chain_key(key1, key2).

The use of class id created collisions in cases like the following:

1.- Consider an initial state in which no class has been acquired yet.
Under these circumstances an AA deadlock will not be detected by
lockdep:

  lock  [key1,key2]->new key  (key1=old chain_key, key2=id)
  --------------------------
  A     [0,0]->0
  A     [0,0]->0 (collision)

  The newly generated chain_key collides with the one used before and as
  a result the check for a deadlock is skipped

  A simple test using liblockdep and a pthread mutex confirms the
  problem: (omitting stack traces)

    new class 0xe15038: 0x7ffc64950f20
    acquire class [0xe15038] 0x7ffc64950f20
    acquire class [0xe15038] 0x7ffc64950f20
    hash chain already cached, key: 0000000000000000 tail class:
    [0xe15038] 0x7ffc64950f20

2.- Consider an ABBA in 2 different tasks and no class yet acquired.

  T1 [key1,key2]->new key     T2[key1,key2]->new key
  --                          --
  A [0,0]->0

                              B [0,1]->1

  B [0,1]->1  (collision)

                              A

In this case the collision prevents lockdep from creating the new
dependency A->B. This in turn results in lockdep not detecting the
circular dependency when T2 acquires A.

Signed-off-by: Alfredo Alvarez Fernandez <alfredoalvarezernandez@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: sasha.levin@oracle.com
Link: http://lkml.kernel.org/r/1455147212-2389-4-git-send-email-alfredoalvarezernandez@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>

1 files changed, 6 insertions, 8 deletions

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 3261214323fa..6f446eb3c742 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -2143,7 +2143,7 @@ static void check_chain_key(struct task_struct *curr)
 {
 #ifdef CONFIG_DEBUG_LOCKDEP
         struct held_lock *hlock, *prev_hlock = NULL;
-        unsigned int i, id;
+        unsigned int i;
         u64 chain_key = 0;
 
         for (i = 0; i < curr->lockdep_depth; i++) {
@@ -2160,17 +2160,16 @@ static void check_chain_key(struct task_struct *curr)
                                 (unsigned long long)hlock->prev_chain_key);
                         return;
                 }
-                id = hlock->class_idx - 1;
                 /*
                  * Whoops ran out of static storage again?
                  */
-                if (DEBUG_LOCKS_WARN_ON(id >= MAX_LOCKDEP_KEYS))
+                if (DEBUG_LOCKS_WARN_ON(hlock->class_idx > MAX_LOCKDEP_KEYS))
                         return;
 
                 if (prev_hlock && (prev_hlock->irq_context !=
                                                         hlock->irq_context))
                         chain_key = 0;
-                chain_key = iterate_chain_key(chain_key, id);
+                chain_key = iterate_chain_key(chain_key, hlock->class_idx);
                 prev_hlock = hlock;
         }
         if (chain_key != curr->curr_chain_key) {
@@ -3048,7 +3047,7 @@ 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;
-        unsigned int depth, id;
+        unsigned int depth;
         int chain_head = 0;
         int class_idx;
         u64 chain_key;
@@ -3151,11 +3150,10 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
          * The 'key ID' is what is the most compact key value to drive
          * the hash, not class->key.
          */
-        id = class - lock_classes;
         /*
          * Whoops, we did it again.. ran straight out of our static allocation.
          */
-        if (DEBUG_LOCKS_WARN_ON(id >= MAX_LOCKDEP_KEYS))
+        if (DEBUG_LOCKS_WARN_ON(class_idx > MAX_LOCKDEP_KEYS))
                 return 0;
 
         chain_key = curr->curr_chain_key;
@@ -3173,7 +3171,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
                 chain_key = 0;
                 chain_head = 1;
         }
-        chain_key = iterate_chain_key(chain_key, id);
+        chain_key = iterate_chain_key(chain_key, class_idx);
 
         if (nest_lock && !__lock_is_held(nest_lock))
                 return print_lock_nested_lock_not_held(curr, hlock, ip);