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

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

[Andrey Tsyvarev]

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).

Signed-off-by: Andrey Tsyvarev <tsyvarev@ispras.ru>
---
 kernel/module.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/module.c b/kernel/module.c
index 3965511..2b44de4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3311,6 +3311,9 @@ 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);
-- 
1.8.3.1

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

[Rusty Russell]

Andrey Tsyvarev <tsyvarev@ispras.ru> 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...

Confused,
Rusty.

> Signed-off-by: Andrey Tsyvarev <tsyvarev@ispras.ru>
> ---
>  kernel/module.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 3965511..2b44de4 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3311,6 +3311,9 @@ 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);
> -- 
> 1.8.3.1

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

[Andrey Tsyvarev]

20.01.2015 9:37, Rusty Russell пишет:
> Andrey Tsyvarev <tsyvarev@ispras.ru> 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...
As I understand, locks are not allowed to be defined in the module init 
section. So, no needs to call lockdep_free_key_range() for it.
This has a sense: objects from that section are allowed to be used only 
by module->init() function. But a single function call doesn't require 
any synchronization wrt itself.


If it helps, I used module below for verify effect of the patch:

test.c:
---
#include <linux/module.h>
#include <linux/moduleparam.h>

MODULE_AUTHOR("Tsyvarev Andrey");
MODULE_LICENSE("GPL");

static DEFINE_MUTEX(mutex_main);
static DEFINE_MUTEX(mutex_param);

static int param_set(const char* val, const struct kernel_param* kp)
{
     mutex_lock(&mutex_param); //Use mutex defined in the module
     mutex_unlock(&mutex_param);

     return -EINVAL; // Setting this parameter is always invalid
}

static struct kernel_param_ops param_ops =
{
     .set = param_set
};

module_param_cb(param, &param_ops, NULL, S_IRUGO);

static int minit(void)
{
     mutex_lock(&mutex_main); // Use another mutex
     mutex_unlock(&mutex_main);
     return 0;
}

static void mexit(void)
{
}

module_init(minit);
module_exit(mexit);
--

insmod test.ko param=1 # Will fail
insmod test.ko

If kernel is compiled with CONFIG_LOCKDEP, then, without patch, lockdep 
crashes on the second module insertion.

>
> Confused,
> Rusty.
>
>> Signed-off-by: Andrey Tsyvarev <tsyvarev@ispras.ru>
>> ---
>>   kernel/module.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 3965511..2b44de4 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3311,6 +3311,9 @@ 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);
>> -- 
>> 1.8.3.1

-- 
Best regards,

Andrey Tsyvarev
Linux Verification Center, ISPRAS
web:http://linuxtesting.org

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

[Rusty Russell]

Andrey Tsyvarev <tsyvarev@ispras.ru> writes:
> 20.01.2015 9:37, Rusty Russell пишет:
>> Andrey Tsyvarev <tsyvarev@ispras.ru> 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...
> As I understand, locks are not allowed to be defined in the module init 
> section. So, no needs to call lockdep_free_key_range() for it.
> This has a sense: objects from that section are allowed to be used only 
> by module->init() function. But a single function call doesn't require 
> any synchronization wrt itself.

I don't know that we have any __initdata locks; it would be really
weird.

But change 'static DEFINE_MUTEX(mutex_param);' to 'static __initdata
DEFINE_MUTEX(mutex_param);' to test.

Cheers,
Rusty.

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

[Andrey Tsyvarev]

21.01.2015 4:40, Rusty Russell пишет:
> Andrey Tsyvarev <tsyvarev@ispras.ru> writes:
>> 20.01.2015 9:37, Rusty Russell пишет:
>>> Andrey Tsyvarev <tsyvarev@ispras.ru> 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...
>> As I understand, locks are not allowed to be defined in the module init
>> section. So, no needs to call lockdep_free_key_range() for it.
>> This has a sense: objects from that section are allowed to be used only
>> by module->init() function. But a single function call doesn't require
>> any synchronization wrt itself.
> I don't know that we have any __initdata locks; it would be really
> weird.
>
> But change 'static DEFINE_MUTEX(mutex_param);' to 'static __initdata
> DEFINE_MUTEX(mutex_param);' to test.
Compiler warns about sections mismatch, but the test works.

According to lockdep_free_key_range() code, lock class is cleared not 
only according to
its key(which is equal to lock address in the case of static lock) but 
also according to its name.
Lock class name are assigned from the name of lock itself, which is 
initialized using
.name = #lockname
construction inside a macro.
While "__initdata" force mutex structure to be placed inside .init.data 
section, string for the .name
field is placed in its normal section. That's why lock class for the 
"__initdata" mutex is cleared when
lockdep_free_key_range() is called for "core" module section.

I remember that message about lockdep crash(on unmodified kernel and 
with original test)
was also be concerned with the lock class name:

[  184.903688] BUG: unable to handle kernel paging request at 
ffffffffa00110be
[  184.903697] IP: [<ffffffff81321b48>] strcmp+0x18/0x40
[  184.903705] PGD 1e15067 PUD 1e16063 PMD 3793e067 PTE 0
[  184.903711] Oops: 0000 [#1] SMP
[  184.903715] Modules linked in: test(O+) iptable_nat nf_nat_ipv4 nf_nat
[  184.903724] CPU: 0 PID: 5072 Comm: insmod Tainted: G O   
3.19.0-rc4-newest+ #3
[  184.903727] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS 
VirtualBox 12/01/2006
[  184.903730] task: ffff8800130222c0 ti: ffff880013fc8000 task.ti: 
ffff880013fc8000
[  184.903732] RIP: 0010:[<ffffffff81321b48>] [<ffffffff81321b48>] 
strcmp+0x18/0x40
[  184.903737] RSP: 0018:ffff880013fcbbc8  EFLAGS: 00010082
[  184.903739] RAX: 0000000000000000 RBX: ffffffff82566530 RCX: 
0000000000000000
[  184.903741] RDX: ffffffffa0015168 RSI: ffffffffa001509d RDI: 
ffffffffa00110bf
[  184.903743] RBP: ffff880013fcbbc8 R08: 0000000000000000 R09: 
0000000000000000
[  184.903745] R10: fec17ffa5a615168 R11: ffffffff825666f0 R12: 
0000000000000000
[  184.903747] R13: ffffffffa001509d R14: ffffffff825666e0 R15: 
000000000000fec0
[  184.903750] FS:  00007f250e13e740(0000) GS:ffff88003fc00000(0000) 
knlGS:0000000000000000
[  184.903753] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  184.903755] CR2: ffffffffa00110be CR3: 0000000024188000 CR4: 
00000000000006f0
[  184.903763] Stack:
[  184.903765]  ffff880013fcbbf8 ffffffff81893958 ffff8800130222c0 
ffffffff825666e0
[  184.903769]  0000000000000000 ffffffffa0015168 ffff880013fcbc78 
ffffffff8108e539
[  184.903773]  0000000000000000 0000000000000000 0000000013d3629d 
ffff880000000000
[  184.903778] Call Trace:
[  184.903786]  [<ffffffff81893958>] count_matching_names+0x61/0x8e
[  184.903793]  [<ffffffff8108e539>] __lock_acquire.isra.30+0x8f9/0xa10
[  184.903799]  [<ffffffff8132ffff>] ? ioread32+0x2f/0x50
[  184.903803]  [<ffffffff8108edba>] lock_acquire+0xaa/0x130
[  184.903808]  [<ffffffffa0015049>] ? minit+0x15/0x28 [test]
[  184.903813]  [<ffffffff8189f236>] mutex_lock_nested+0x46/0x340
[  184.903816]  [<ffffffffa0015049>] ? minit+0x15/0x28 [test]
[  184.903822]  [<ffffffff810002b8>] ? do_one_initcall+0x78/0x1c0
[  184.903826]  [<ffffffffa0015034>] ? param_set+0x34/0x34 [test]
[  184.903830]  [<ffffffffa0015049>] minit+0x15/0x28 [test]
[  184.903835]  [<ffffffff810002c4>] do_one_initcall+0x84/0x1c0
[  184.903840]  [<ffffffff81156fc2>] ? __vunmap+0xc2/0x110
[  184.903845]  [<ffffffff810c76de>] load_module+0x1cfe/0x2280
[  184.903849]  [<ffffffff810c41c0>] ? m_show+0x1a0/0x1a0
[  184.903855]  [<ffffffff810e2f3c>] ? __audit_syscall_entry+0xac/0x100
[  184.903859]  [<ffffffff810c7d96>] SyS_finit_module+0x76/0x80
[  184.903864]  [<ffffffff818a2b69>] system_call_fastpath+0x12/0x17
[  184.903866] Code: c9 88 4a ff 75 ed 5d c3 66 66 2e 0f 1f 84 00 00 00 
00 00 55 48 89 e5 eb 0e 66 2e 0f 1f 84 00 00 00 00 00 84 c0 74 1c 48 83 
c7 01 <0f> b6 47 ff 48 83 c6 01 3a 46 ff 74 eb 19 c0 83 c8 01 5d c3 0f
[  184.903912] RIP  [<ffffffff81321b48>] strcmp+0x18/0x40
[  184.903916]  RSP <ffff880013fcbbc8>
[  184.903918] CR2: ffffffffa00110be



>
> Cheers,
> Rusty.
>

-- 
Best regards,

Andrey Tsyvarev
Linux Verification Center, ISPRAS
web:http://linuxtesting.org

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

[Rusty Russell]

Andrey Tsyvarev <tsyvarev@ispras.ru> writes:
> 21.01.2015 4:40, Rusty Russell пишет:
>> Andrey Tsyvarev <tsyvarev@ispras.ru> writes:
>>> 20.01.2015 9:37, Rusty Russell пишет:
>>>> Andrey Tsyvarev <tsyvarev@ispras.ru> 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...
>>> As I understand, locks are not allowed to be defined in the module init
>>> section. So, no needs to call lockdep_free_key_range() for it.
>>> This has a sense: objects from that section are allowed to be used only
>>> by module->init() function. But a single function call doesn't require
>>> any synchronization wrt itself.
>> I don't know that we have any __initdata locks; it would be really
>> weird.
>>
>> But change 'static DEFINE_MUTEX(mutex_param);' to 'static __initdata
>> DEFINE_MUTEX(mutex_param);' to test.
> Compiler warns about sections mismatch, but the test works.
>
> According to lockdep_free_key_range() code, lock class is cleared not 
> only according to
> its key(which is equal to lock address in the case of static lock) but 
> also according to its name.

What happens if you later register another lock at that address, since
the memory is freed?

A quick grep revealed no __initdata locks in the kernel, so I don't
think we care anyway.

Cheers,
Rusty.

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

[Andrey Tsyvarev]

22.01.2015 3:40, Rusty Russell пишет:
> Andrey Tsyvarev <tsyvarev@ispras.ru> writes:
>> 21.01.2015 4:40, Rusty Russell пишет:
>>> Andrey Tsyvarev <tsyvarev@ispras.ru> writes:
>>>> 20.01.2015 9:37, Rusty Russell пишет:
>>>>> Andrey Tsyvarev <tsyvarev@ispras.ru> 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...
>>>> As I understand, locks are not allowed to be defined in the module init
>>>> section. So, no needs to call lockdep_free_key_range() for it.
>>>> This has a sense: objects from that section are allowed to be used only
>>>> by module->init() function. But a single function call doesn't require
>>>> any synchronization wrt itself.
>>> I don't know that we have any __initdata locks; it would be really
>>> weird.
>>>
>>> But change 'static DEFINE_MUTEX(mutex_param);' to 'static __initdata
>>> DEFINE_MUTEX(mutex_param);' to test.
>> Compiler warns about sections mismatch, but the test works.
>>
>> According to lockdep_free_key_range() code, lock class is cleared not
>> only according to
>> its key(which is equal to lock address in the case of static lock) but
>> also according to its name.
> What happens if you later register another lock at that address, since
> the memory is freed?
Do you mean that scenario:

1) mutex1 is placed in module1 .init.data section,
2) after module1 is initialized, .init.data section is freed,
3) same memory is reused for module2 .data section,
4) mutex2 is placed in module2 .data section at the same address, as 
mutex1 was?

It seems, mutex2 will share lock class with mutex1. That is, lockdep 
will confused:

[kernel/locking/lockdep.c]
707                 if (class->key == key) {
708                         /*
709                          * Huh! same key, different name? Did 
someone trample
710                          * on some memory? We're most confused.
711                          */
712                         WARN_ON_ONCE(class->name != lock->name);
713                         return class;

Things will go worse, when

5) module1 is exited, and lock class for mutex1 will be cleared

because mutex2 will cache lock class which actually does not exist.

-- 
Best regards,

Andrey Tsyvarev
Linux Verification Center, ISPRAS
web:http://linuxtesting.org

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

[Peter Zijlstra]

On Tue, Jan 20, 2015 at 05:07:19PM +1030, Rusty Russell wrote:
> Andrey Tsyvarev <tsyvarev@ispras.ru> 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.

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

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Jan 20, 2015 at 05:07:19PM +1030, Rusty Russell wrote:
> > Andrey Tsyvarev <tsyvarev@ispras.ru> 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 ...

Thanks,

	Ingo

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@infradead.org> wrote:
> 
> > On Tue, Jan 20, 2015 at 05:07:19PM +1030, Rusty Russell wrote:
> > > Andrey Tsyvarev <tsyvarev@ispras.ru> 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);

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

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> > 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.

> @@ -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();

> +	/* Free lock-classes; relies on the preceding sync_rcu(). */
>  	lockdep_free_key_range(mod->module_core, mod->core_size);

>   free_module:
> +	/* Free lock-classes; relies on the preceding sync_rcu() */
> +	lockdep_free_key_range(mod->module_core, mod->core_size);

Yeah. Looks good to me in principle, without having tested 
it that is as I don't use modules on devel boxes:

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo