On Thu, Feb 19, 2015 at 01:12:33AM +0100, Ingo Molnar wrote:
>
> * Peter Zijlstra <[email protected]> wrote:
>
> > On Tue, Jan 20, 2015 at 05:07:19PM +1030, Rusty Russell wrote:
> > > Andrey Tsyvarev <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
