Re: [PATCH] kernel/module.c: Free lock-classes if parse_args failed

[Peter Zijlstra]

On Thu, Feb 19, 2015 at 01:12:33AM +0100, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> 
> > On Tue, Jan 20, 2015 at 05:07:19PM +1030, Rusty Russell wrote:
> > > Andrey Tsyvarev <tsyvarev@xxxxxxxxx> writes:
> > > > parse_args call module parameters' .set handlers, which may use locks defined in the module.
> > > > So, these classes should be freed in case parse_args returns error(e.g. due to incorrect parameter passed).
> > > 
> > > Thanks, this seems right.  Applied.
> > > 
> > > But this makes me ask: where is lockdep_free_key_range() called on the
> > > module init code?  It doesn't seem to be at all...
> > 
> > Hmm, Ingo, how does this work? The lockless class lookup in
> > look_up_lock_class() very much assumes the class hash is only added too,
> > but here we go wipe stuff from it.
> > 
> > From what I can tell, every use of lockdep_free_key_range() is broken.
> 
> indeed ...

How about something like so? It would fix this particular issue and lays
the groundwork for maybe reusing some of the resources we now leak.

---
 kernel/locking/lockdep.c | 30 +++++++++++++++++++++++++++---
 kernel/module.c          |  8 ++++----
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 88d0d4420ad2..9fdf029f90d9 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -700,10 +700,12 @@ look_up_lock_class(struct lockdep_map *lock, unsigned int subclass)
 	hash_head = classhashentry(key);
 
 	/*
-	 * We can walk the hash lockfree, because the hash only
-	 * grows, and we are careful when adding entries to the end:
+	 * We do an RCU walk of the hash, see lockdep_free_key_range().
 	 */
-	list_for_each_entry(class, hash_head, hash_entry) {
+	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
+		return NULL;
+
+	list_for_each_entry_rcu(class, hash_head, hash_entry) {
 		if (class->key == key) {
 			/*
 			 * Huh! same key, different name? Did someone trample
@@ -3887,6 +3889,14 @@ static inline int within(const void *addr, void *start, unsigned long size)
 	return addr >= start && addr < start + size;
 }
 
+/*
+ * Used in module.c to remove lock classes from memory that is going to be
+ * freed; and possibly re-used by other modules.
+ *
+ * We will have had one sync_rcu() before getting here, so we're guaranteed
+ * nobody will look up these exact classes -- they're properly dead but still
+ * allocated.
+ */
 void lockdep_free_key_range(void *start, unsigned long size)
 {
 	struct lock_class *class, *next;
@@ -3916,6 +3926,20 @@ void lockdep_free_key_range(void *start, unsigned long size)
 	if (locked)
 		graph_unlock();
 	raw_local_irq_restore(flags);
+
+	/*
+	 * Wait for any possible iterators from look_up_lock_class() to pass
+	 * before continuing to free the memory they refer to.
+	 *
+	 * sync_sched() is sufficient because the read-side is IRQ disable.
+	 */
+	synchronize_sched();
+
+	/*
+	 * XXX at this point we could return the resources to the pool;
+	 * instead we leak them. We would need to change to bitmap allocators
+	 * instead of the linear allocators we have now.
+	 */
 }
 
 void lockdep_reset_lock(struct lockdep_map *lock)
diff --git a/kernel/module.c b/kernel/module.c
index b34813f725e9..326608f34b23 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1867,7 +1867,7 @@ static void free_module(struct module *mod)
 	kfree(mod->args);
 	percpu_modfree(mod);
 
-	/* Free lock-classes: */
+	/* Free lock-classes; relies on the preceding sync_rcu(). */
 	lockdep_free_key_range(mod->module_core, mod->core_size);
 
 	/* Finally, free the core (containing the module structure) */
@@ -3349,9 +3349,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	module_bug_cleanup(mod);
 	mutex_unlock(&module_mutex);
 
-	/* Free lock-classes: */
-	lockdep_free_key_range(mod->module_core, mod->core_size);
-
 	/* we can't deallocate the module until we clear memory protection */
 	unset_module_init_ro_nx(mod);
 	unset_module_core_ro_nx(mod);
@@ -3375,6 +3372,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	synchronize_rcu();
 	mutex_unlock(&module_mutex);
  free_module:
+	/* Free lock-classes; relies on the preceding sync_rcu() */
+	lockdep_free_key_range(mod->module_core, mod->core_size);
+
 	module_deallocate(mod, info);
  free_copy:
 	free_copy(info);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/