Oops when accessing /proc/lockdep_chains

hi,

with current -git (aka -rc2) i sometime get the following oops when doing a cat /proc/lockdep_chains
The other /proc/lockdep* files dont cause any errors

I dont get it after a fresh reboot :| But was able to reproduce it when running my testscripts, I'll try to narrow it down.

Quoted 6 lines · * Eric Sesterhenn ([email protected]) wrote:

hi,

with current -git (aka -rc2) i sometime get the following oops when doing a cat /proc/lockdep_chains
The other /proc/lockdep* files dont cause any errors

Quoted 2 lines

I dont get it after a fresh reboot :| But was able to reproduce it when running my testscripts, I'll try to narrow it down.

I can easily reproduce this with

modprobe rcutorture; sleep 10s; rmmod rcutorture; cat /proc/lockdep_chains > /dev/null

/proc/lockdep_chains currently oopses after any module which creates and uses a lock is unloaded. This is because one of the chains involves a class which was defined in the module just unloaded.

The classes are already correctly taken care of using the all_lock_classes which keeps track of all active lock classses. Add a similar all_lock_chains list and use it for keeping track of chains.

Reported-by Eric Sesterhenn <[email protected]>

Signed-off-by Rabin Vincent <[email protected]>

---
 include/linux/lockdep.h    |    3 +-
 kernel/lockdep.c           |   53 ++++++++++++++++++++++++++++++++++++++-----
 kernel/lockdep_internals.h |    2 +-
 kernel/lockdep_proc.c      |   17 ++++++++++----
 4 files changed, 61 insertions(+), 14 deletions(-)

include/linux/lockdep.h +2 -1

index 2486eb4..735d06b 100644
@@ -185,7 +185,8 @@ struct lock_chain {
 	u8				irq_context;
 	u8				depth;
 	u16				base;
-	struct list_head		entry;
+	struct list_head		hash_entry;
+	struct list_head		lock_entry;
 	u64				chain_key;
 };
 

kernel/lockdep.c +46 -7

index d38a643..1b3537b 100644
@@ -253,6 +253,11 @@ LIST_HEAD(all_lock_classes);
 static struct list_head classhash_table[CLASSHASH_SIZE];
 
 /*
+ * We also keep a global list of all lock chains.
+ */
+LIST_HEAD(all_lock_chains);
+
+/*
  * We put the lock dependency chains into a hash-table as well, to cache
  * their existence:
  */

@@ -1462,7 +1467,7 @@ out_bug:
 }
 
 unsigned long nr_lock_chains;
-struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
+static struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
 int nr_chain_hlocks;
 static u16 chain_hlocks[MAX_LOCKDEP_CHAIN_HLOCKS];
 

@@ -1493,7 +1498,7 @@ static inline int lookup_chain_cache(struct task_struct *curr,
 	 * We can walk it lock-free, because entries only get added
 	 * to the hash:
 	 */
-	list_for_each_entry(chain, hash_head, entry) {
+	list_for_each_entry(chain, hash_head, hash_entry) {
 		if (chain->chain_key == chain_key) {
 cache_hit:
 			debug_atomic_inc(&chain_lookup_hits);

@@ -1517,7 +1522,7 @@ cache_hit:
 	/*
 	 * We have to walk the chain again locked - to avoid duplicates:
 	 */
-	list_for_each_entry(chain, hash_head, entry) {
+	list_for_each_entry(chain, hash_head, hash_entry) {
 		if (chain->chain_key == chain_key) {
 			graph_unlock();
 			goto cache_hit;

@@ -1559,7 +1564,8 @@ cache_hit:
 		}
 		chain_hlocks[chain->base + j] = class - lock_classes;
 	}
-	list_add_tail_rcu(&chain->entry, hash_head);
+	list_add_tail_rcu(&chain->hash_entry, hash_head);
+	list_add_tail_rcu(&chain->lock_entry, &all_lock_chains);
 	debug_atomic_inc(&chain_lookup_misses);
 	inc_chains();
 

@@ -2967,6 +2973,8 @@ void lockdep_reset(void)
 	debug_locks = 1;
 	for (i = 0; i < CHAINHASH_SIZE; i++)
 		INIT_LIST_HEAD(chainhash_table + i);
+	for (i = 0; i < CLASSHASH_SIZE; i++)
+		INIT_LIST_HEAD(classhash_table + i);
 	raw_local_irq_restore(flags);
 }
 

@@ -2990,6 +2998,15 @@ static void zap_class(struct lock_class *class)
 
 }
 
+static void zap_chain(struct lock_chain *chain)
+{
+	/*
+	 * Unhash the chain and remove it from the all_lock_chains list:
+	 */
+	list_del_rcu(&chain->hash_entry);
+	list_del_rcu(&chain->lock_entry);
+}
+
 static inline int within(const void *addr, void *start, unsigned long size)
 {
 	return addr >= start && addr < start + size;

@@ -2997,23 +3014,45 @@ static inline int within(const void *addr, void *start, unsigned long size)
 
 void lockdep_free_key_range(void *start, unsigned long size)
 {
-	struct lock_class *class, *next;
+	struct lock_class *class, *nextclass;
+	struct lock_chain *chain, *nextchain;
 	struct list_head *head;
 	unsigned long flags;
-	int i;
+	int i, j;
 	int locked;
 
 	raw_local_irq_save(flags);
 	locked = graph_lock();
 
 	/*
+	 * Unhash all chains that involve classes created by this module:
+	 */
+	for (i = 0; i < CHAINHASH_SIZE; i++) {
+		head = chainhash_table + i;
+		if (list_empty(head))
+			continue;
+		list_for_each_entry_safe(chain, nextchain, head, hash_entry) {
+			for (j = 0; j < chain->depth; j++) {
+				class = lock_classes +
+					chain_hlocks[chain->base + j];
+
+				if (within(class->key, start, size) ||
+					within(class->name, start, size)) {
+					zap_chain(chain);
+					break;
+				}
+			}
+		}
+	}
+
+	/*
 	 * Unhash all classes that were created by this module:
 	 */
 	for (i = 0; i < CLASSHASH_SIZE; i++) {
 		head = classhash_table + i;
 		if (list_empty(head))
 			continue;
-		list_for_each_entry_safe(class, next, head, hash_entry) {
+		list_for_each_entry_safe(class, nextclass, head, hash_entry) {
 			if (within(class->key, start, size))
 				zap_class(class);
 			else if (within(class->name, start, size))

kernel/lockdep_internals.h +1 -1

index c3600a0..ebf2ecb 100644
@@ -32,7 +32,7 @@
 #define MAX_STACK_TRACE_ENTRIES	262144UL
 
 extern struct list_head all_lock_classes;
-extern struct lock_chain lock_chains[];
+extern struct list_head all_lock_chains;
 
 extern void
 get_usage_chars(struct lock_class *class, char *c1, char *c2, char *c3, char *c4);

kernel/lockdep_proc.c +12 -5

index 9b0e940..4f447fd 100644
@@ -190,8 +190,9 @@ static void *lc_next(struct seq_file *m, void *v, loff_t *pos)
 	else {
 		chain = v;
 
-		if (*pos < nr_lock_chains)
-			chain = lock_chains + *pos;
+		if (chain->lock_entry.next != &all_lock_chains)
+			chain = list_entry(chain->lock_entry.next,
+					   struct lock_chain, lock_entry);
 		else
 			chain = NULL;
 	}

@@ -201,11 +202,16 @@ static void *lc_next(struct seq_file *m, void *v, loff_t *pos)
 
 static void *lc_start(struct seq_file *m, loff_t *pos)
 {
+	struct lock_chain *chain;
+	loff_t i = 0;
+
 	if (*pos == 0)
 		return SEQ_START_TOKEN;
 
-	if (*pos < nr_lock_chains)
-		return lock_chains + *pos;
+	list_for_each_entry(chain, &all_lock_chains, lock_entry) {
+		if (++i == *pos)
+			return chain;
+	}
 
 	return NULL;
 }

@@ -252,7 +258,8 @@ static int lockdep_chains_open(struct inode *inode, struct file *file)
 		struct seq_file *m = file->private_data;
 
 		if (nr_lock_chains)
-			m->private = lock_chains;
+			m->private = list_entry(all_lock_chains.next,
+					struct lock_chain, lock_entry);
 		else
 			m->private = NULL;
 	}

Quoted 8 lines · Rabin Vincent

/proc/lockdep_chains currently oopses after any module which creates and uses a lock is unloaded. This is because one of the chains involves a class which was defined in the module just unloaded.

The classes are already correctly taken care of using the all_lock_classes which keeps track of all active lock classses. Add a similar all_lock_chains list and use it for keeping track of chains.

I think there is a simpler method to deal with this.

- Mark class as useless during zap_class()
- When output lock_chain, if some classes are useless, do not output the class.

Quoted 9 lines · Huang Ying

Quoted 8 lines · Rabin Vincent

/proc/lockdep_chains currently oopses after any module which creates and uses a lock is unloaded. This is because one of the chains involves a class which was defined in the module just unloaded.

The classes are already correctly taken care of using the all_lock_classes which keeps track of all active lock classses. Add a similar all_lock_chains list and use it for keeping track of chains.

[...]

Quoted 2 lines

I think there is a simpler method to deal with this.

Yes. I went with the all_lock_chains list approach because there was similar code already being used to keep track of lock_class structures.

Quoted 3 lines

- Mark class as useless during zap_class()
- When output lock_chain, if some classes are useless, do not output the class.

Like the patch below? I set ->key to NULL after zapping the class and use that as a condition to not print the class' information. The only issue is that with this patch there will be some chains output with no locks listed under them.

---
lockdep: handle chains involving classes defined in modules

/proc/lockdep_chains currently oopses after any module which creates and uses a lock is unloaded. This is because one of the chains involves a class which was defined in the module just unloaded.

Solve this by marking the classes as unused and not printing information about the unused classes.

Reported-by Eric Sesterhenn <[email protected]>

Signed-off-by Rabin Vincent <[email protected]>

kernel/lockdep.c +1 -0

index d38a643..8ade874 100644
@@ -2988,6 +2988,7 @@ static void zap_class(struct lock_class *class)
 	list_del_rcu(&class->hash_entry);
 	list_del_rcu(&class->lock_entry);
 
+	class->key = NULL;
 }
 
 static inline int within(const void *addr, void *start, unsigned long size)

kernel/lockdep_proc.c +0 -0

index 9b0e940..f09b6c7 100644

/proc/lockdep_chains currently oopses after any module which creates and uses a lock is unloaded. This is because one of the chains involves a class which was defined in the module just unloaded.

The classes are already correctly taken care of using the all_lock_classes which keeps track of all active lock classses. Add a similar all_lock_chains list and use it for keeping track of chains.

Reported-by Eric Sesterhenn <[email protected]>

Signed-off-by Rabin Vincent <[email protected]>

---
 include/linux/lockdep.h    |    3 +-
 kernel/lockdep.c           |   53 ++++++++++++++++++++++++++++++++++++++-----
 kernel/lockdep_internals.h |    2 +-
 kernel/lockdep_proc.c      |   17 ++++++++++----
 4 files changed, 61 insertions(+), 14 deletions(-)

include/linux/lockdep.h +2 -1

index 2486eb4..735d06b 100644
@@ -185,7 +185,8 @@ struct lock_chain {
 	u8				irq_context;
 	u8				depth;
 	u16				base;
-	struct list_head		entry;
+	struct list_head		hash_entry;
+	struct list_head		lock_entry;
 	u64				chain_key;
 };
 

kernel/lockdep.c +46 -7

index d38a643..1b3537b 100644
@@ -253,6 +253,11 @@ LIST_HEAD(all_lock_classes);
 static struct list_head classhash_table[CLASSHASH_SIZE];
 
 /*
+ * We also keep a global list of all lock chains.
+ */
+LIST_HEAD(all_lock_chains);
+
+/*
  * We put the lock dependency chains into a hash-table as well, to cache
  * their existence:
  */

@@ -1462,7 +1467,7 @@ out_bug:
 }
 
 unsigned long nr_lock_chains;
-struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
+static struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
 int nr_chain_hlocks;
 static u16 chain_hlocks[MAX_LOCKDEP_CHAIN_HLOCKS];
 

@@ -1493,7 +1498,7 @@ static inline int lookup_chain_cache(struct task_struct *curr,
 	 * We can walk it lock-free, because entries only get added
 	 * to the hash:
 	 */
-	list_for_each_entry(chain, hash_head, entry) {
+	list_for_each_entry(chain, hash_head, hash_entry) {
 		if (chain->chain_key == chain_key) {
 cache_hit:
 			debug_atomic_inc(&chain_lookup_hits);

@@ -1517,7 +1522,7 @@ cache_hit:
 	/*
 	 * We have to walk the chain again locked - to avoid duplicates:
 	 */
-	list_for_each_entry(chain, hash_head, entry) {
+	list_for_each_entry(chain, hash_head, hash_entry) {
 		if (chain->chain_key == chain_key) {
 			graph_unlock();
 			goto cache_hit;

@@ -1559,7 +1564,8 @@ cache_hit:
 		}
 		chain_hlocks[chain->base + j] = class - lock_classes;
 	}
-	list_add_tail_rcu(&chain->entry, hash_head);
+	list_add_tail_rcu(&chain->hash_entry, hash_head);
+	list_add_tail_rcu(&chain->lock_entry, &all_lock_chains);
 	debug_atomic_inc(&chain_lookup_misses);
 	inc_chains();
 

@@ -2967,6 +2973,8 @@ void lockdep_reset(void)
 	debug_locks = 1;
 	for (i = 0; i < CHAINHASH_SIZE; i++)
 		INIT_LIST_HEAD(chainhash_table + i);
+	for (i = 0; i < CLASSHASH_SIZE; i++)
+		INIT_LIST_HEAD(classhash_table + i);
 	raw_local_irq_restore(flags);
 }
 

@@ -2990,6 +2998,15 @@ static void zap_class(struct lock_class *class)
 
 }
 
+static void zap_chain(struct lock_chain *chain)
+{
+	/*
+	 * Unhash the chain and remove it from the all_lock_chains list:
+	 */
+	list_del_rcu(&chain->hash_entry);
+	list_del_rcu(&chain->lock_entry);
+}
+
 static inline int within(const void *addr, void *start, unsigned long size)
 {
 	return addr >= start && addr < start + size;

@@ -2997,23 +3014,45 @@ static inline int within(const void *addr, void *start, unsigned long size)
 
 void lockdep_free_key_range(void *start, unsigned long size)
 {
-	struct lock_class *class, *next;
+	struct lock_class *class, *nextclass;
+	struct lock_chain *chain, *nextchain;
 	struct list_head *head;
 	unsigned long flags;
-	int i;
+	int i, j;
 	int locked;
 
 	raw_local_irq_save(flags);
 	locked = graph_lock();
 
 	/*
+	 * Unhash all chains that involve classes created by this module:
+	 */
+	for (i = 0; i < CHAINHASH_SIZE; i++) {
+		head = chainhash_table + i;
+		if (list_empty(head))
+			continue;
+		list_for_each_entry_safe(chain, nextchain, head, hash_entry) {
+			for (j = 0; j < chain->depth; j++) {
+				class = lock_classes +
+					chain_hlocks[chain->base + j];
+
+				if (within(class->key, start, size) ||
+					within(class->name, start, size)) {
+					zap_chain(chain);
+					break;
+				}
+			}
+		}
+	}
+
+	/*
 	 * Unhash all classes that were created by this module:
 	 */
 	for (i = 0; i < CLASSHASH_SIZE; i++) {
 		head = classhash_table + i;
 		if (list_empty(head))
 			continue;
-		list_for_each_entry_safe(class, next, head, hash_entry) {
+		list_for_each_entry_safe(class, nextclass, head, hash_entry) {
 			if (within(class->key, start, size))
 				zap_class(class);
 			else if (within(class->name, start, size))

kernel/lockdep_internals.h +1 -1

index c3600a0..ebf2ecb 100644
@@ -32,7 +32,7 @@
 #define MAX_STACK_TRACE_ENTRIES	262144UL
 
 extern struct list_head all_lock_classes;
-extern struct lock_chain lock_chains[];
+extern struct list_head all_lock_chains;
 
 extern void
 get_usage_chars(struct lock_class *class, char *c1, char *c2, char *c3, char *c4);

kernel/lockdep_proc.c +12 -5

index 9b0e940..4f447fd 100644
@@ -190,8 +190,9 @@ static void *lc_next(struct seq_file *m, void *v, loff_t *pos)
 	else {
 		chain = v;
 
-		if (*pos < nr_lock_chains)
-			chain = lock_chains + *pos;
+		if (chain->lock_entry.next != &all_lock_chains)
+			chain = list_entry(chain->lock_entry.next,
+					   struct lock_chain, lock_entry);
 		else
 			chain = NULL;
 	}

@@ -201,11 +202,16 @@ static void *lc_next(struct seq_file *m, void *v, loff_t *pos)
 
 static void *lc_start(struct seq_file *m, loff_t *pos)
 {
+	struct lock_chain *chain;
+	loff_t i = 0;
+
 	if (*pos == 0)
 		return SEQ_START_TOKEN;
 
-	if (*pos < nr_lock_chains)
-		return lock_chains + *pos;
+	list_for_each_entry(chain, &all_lock_chains, lock_entry) {
+		if (++i == *pos)
+			return chain;
+	}
 
 	return NULL;
 }

@@ -252,7 +258,8 @@ static int lockdep_chains_open(struct inode *inode, struct file *file)
 		struct seq_file *m = file->private_data;
 
 		if (nr_lock_chains)
-			m->private = lock_chains;
+			m->private = list_entry(all_lock_chains.next,
+					struct lock_chain, lock_entry);
 		else
 			m->private = NULL;
 	}