[V2 PATCH 1/3] module: use rcu to protect module list read

[V2 PATCH 1/3] module: use rcu to protect module list read

[Cong Wang]

V2:
As Eric pointed out,
* should use rcu_read_lock_sched() instead of rcu_read_lock()
* set_all_modules_text_{rw,ro}() mean to block module insertion/delete,
  so should continue use module_mutex, also  its list_for_each_entry_rcu()
  can be replaced with list_for_each_entry().

Now the read of module list is protected by preempt disable + *_rcu
list operations, this patch replaces the raw preempt_disable()
with rcu_read_lock_sched() wrapper, to use rcu to protect it directly,
the write is still protected by module_mutex.

I tested it with the following test case:

	#!/bin/bash
	i=0
	while [ $i -lt $1 ];
	do
		modprobe dummy && echo success
		modprobe bonding miimon=1000 && echo success
		let i=i+1
	done &

	i=0
	while [ $i -lt $1 ];
	do
		rmmod dummy && echo success
		rmmod bonding && echo success
		let i=i+1
	done
	echo done
	exit 0

for thousands of times, no problems.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Reported-by: <Dennis1.Chen@amd.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 kernel/module.c |   81 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 44 insertions(+), 37 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 2c93276..f0f8d4a 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -91,7 +91,7 @@
 
 /*
  * Mutex protects:
- * 1) List of modules (also safely readable with preempt_disable),
+ * 1) List of modules (also safely readable with rcu_read_lock_sched),
  * 2) module_use links,
  * 3) module_addr_min/module_addr_max.
  * (delete uses stop_machine/add uses RCU list operations). */
@@ -382,7 +382,7 @@ static bool find_symbol_in_section(const struct symsearch *syms,
 }
 
 /* Find a symbol and return it, along with, (optional) crc and
- * (optional) module which owns it.  Needs preempt disabled or module_mutex. */
+ * (optional) module which owns it.  Needs rcu_read_lock_sched. */
 const struct kernel_symbol *find_symbol(const char *name,
 					struct module **owner,
 					const unsigned long **crc,
@@ -413,7 +413,7 @@ struct module *find_module(const char *name)
 {
 	struct module *mod;
 
-	list_for_each_entry(mod, &modules, list) {
+	list_for_each_entry_rcu(mod, &modules, list) {
 		if (strcmp(mod->name, name) == 0)
 			return mod;
 	}
@@ -481,7 +481,7 @@ bool is_module_percpu_address(unsigned long addr)
 	struct module *mod;
 	unsigned int cpu;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 
 	list_for_each_entry_rcu(mod, &modules, list) {
 		if (!mod->percpu_size)
@@ -491,13 +491,13 @@ bool is_module_percpu_address(unsigned long addr)
 
 			if ((void *)addr >= start &&
 			    (void *)addr < start + mod->percpu_size) {
-				preempt_enable();
+				rcu_read_unlock_sched();
 				return true;
 			}
 		}
 	}
 
-	preempt_enable();
+	rcu_read_unlock_sched();
 	return false;
 }
 
@@ -869,11 +869,11 @@ void __symbol_put(const char *symbol)
 {
 	struct module *owner;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 	if (!find_symbol(symbol, &owner, NULL, true, false))
 		BUG();
 	module_put(owner);
-	preempt_enable();
+	rcu_read_unlock_sched();
 }
 EXPORT_SYMBOL(__symbol_put);
 
@@ -1639,7 +1639,7 @@ static void mod_sysfs_teardown(struct module *mod)
 static int __unlink_module(void *_mod)
 {
 	struct module *mod = _mod;
-	list_del(&mod->list);
+	list_del_rcu(&mod->list);
 	module_bug_cleanup(mod);
 	return 0;
 }
@@ -1714,7 +1714,7 @@ void set_all_modules_text_rw(void)
 	struct module *mod;
 
 	mutex_lock(&module_mutex);
-	list_for_each_entry_rcu(mod, &modules, list) {
+	list_for_each_entry(mod, &modules, list) {
 		if ((mod->module_core) && (mod->core_text_size)) {
 			set_page_attributes(mod->module_core,
 						mod->module_core + mod->core_text_size,
@@ -1735,7 +1735,7 @@ void set_all_modules_text_ro(void)
 	struct module *mod;
 
 	mutex_lock(&module_mutex);
-	list_for_each_entry_rcu(mod, &modules, list) {
+	list_for_each_entry(mod, &modules, list) {
 		if ((mod->module_core) && (mod->core_text_size)) {
 			set_page_attributes(mod->module_core,
 						mod->module_core + mod->core_text_size,
@@ -1810,11 +1810,11 @@ void *__symbol_get(const char *symbol)
 	struct module *owner;
 	const struct kernel_symbol *sym;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 	sym = find_symbol(symbol, &owner, NULL, true, true);
+	rcu_read_unlock_sched();
 	if (sym && strong_try_module_get(owner))
 		sym = NULL;
-	preempt_enable();
 
 	return sym ? (void *)sym->value : NULL;
 }
@@ -1844,6 +1844,7 @@ static int verify_export_symbols(struct module *mod)
 #endif
 	};
 
+	rcu_read_lock_sched();
 	for (i = 0; i < ARRAY_SIZE(arr); i++) {
 		for (s = arr[i].sym; s < arr[i].sym + arr[i].num; s++) {
 			if (find_symbol(s->name, &owner, NULL, true, false)) {
@@ -1851,10 +1852,12 @@ static int verify_export_symbols(struct module *mod)
 				       "%s: exports duplicate symbol %s"
 				       " (owned by %s)\n",
 				       mod->name, s->name, module_name(owner));
+				rcu_read_unlock_sched();
 				return -ENOEXEC;
 			}
 		}
 	}
+	rcu_read_unlock_sched();
 	return 0;
 }
 
@@ -3130,7 +3133,7 @@ const char *module_address_lookup(unsigned long addr,
 	struct module *mod;
 	const char *ret = NULL;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 	list_for_each_entry_rcu(mod, &modules, list) {
 		if (within_module_init(addr, mod) ||
 		    within_module_core(addr, mod)) {
@@ -3145,7 +3148,7 @@ const char *module_address_lookup(unsigned long addr,
 		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
 		ret = namebuf;
 	}
-	preempt_enable();
+	rcu_read_unlock_sched();
 	return ret;
 }
 
@@ -3153,7 +3156,7 @@ int lookup_module_symbol_name(unsigned long addr, char *symname)
 {
 	struct module *mod;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 	list_for_each_entry_rcu(mod, &modules, list) {
 		if (within_module_init(addr, mod) ||
 		    within_module_core(addr, mod)) {
@@ -3163,12 +3166,12 @@ int lookup_module_symbol_name(unsigned long addr, char *symname)
 			if (!sym)
 				goto out;
 			strlcpy(symname, sym, KSYM_NAME_LEN);
-			preempt_enable();
+			rcu_read_unlock_sched();
 			return 0;
 		}
 	}
 out:
-	preempt_enable();
+	rcu_read_unlock_sched();
 	return -ERANGE;
 }
 
@@ -3177,7 +3180,7 @@ int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size,
 {
 	struct module *mod;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 	list_for_each_entry_rcu(mod, &modules, list) {
 		if (within_module_init(addr, mod) ||
 		    within_module_core(addr, mod)) {
@@ -3190,12 +3193,12 @@ int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size,
 				strlcpy(modname, mod->name, MODULE_NAME_LEN);
 			if (name)
 				strlcpy(name, sym, KSYM_NAME_LEN);
-			preempt_enable();
+			rcu_read_unlock_sched();
 			return 0;
 		}
 	}
 out:
-	preempt_enable();
+	rcu_read_unlock_sched();
 	return -ERANGE;
 }
 
@@ -3204,7 +3207,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 {
 	struct module *mod;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 	list_for_each_entry_rcu(mod, &modules, list) {
 		if (symnum < mod->num_symtab) {
 			*value = mod->symtab[symnum].st_value;
@@ -3213,12 +3216,12 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 				KSYM_NAME_LEN);
 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
 			*exported = is_exported(name, *value, mod);
-			preempt_enable();
+			rcu_read_unlock_sched();
 			return 0;
 		}
 		symnum -= mod->num_symtab;
 	}
-	preempt_enable();
+	rcu_read_unlock_sched();
 	return -ERANGE;
 }
 
@@ -3241,7 +3244,7 @@ unsigned long module_kallsyms_lookup_name(const char *name)
 	unsigned long ret = 0;
 
 	/* Don't lock: we're in enough trouble already. */
-	preempt_disable();
+	rcu_read_lock_sched();
 	if ((colon = strchr(name, ':')) != NULL) {
 		*colon = '\0';
 		if ((mod = find_module(name)) != NULL)
@@ -3252,7 +3255,7 @@ unsigned long module_kallsyms_lookup_name(const char *name)
 			if ((ret = mod_find_symname(mod, name)) != 0)
 				break;
 	}
-	preempt_enable();
+	rcu_read_unlock_sched();
 	return ret;
 }
 
@@ -3264,14 +3267,18 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 	unsigned int i;
 	int ret;
 
+	rcu_read_lock_sched();
 	list_for_each_entry(mod, &modules, list) {
 		for (i = 0; i < mod->num_symtab; i++) {
 			ret = fn(data, mod->strtab + mod->symtab[i].st_name,
 				 mod, mod->symtab[i].st_value);
-			if (ret != 0)
+			if (ret != 0) {
+				rcu_read_unlock_sched();
 				return ret;
+			}
 		}
 	}
+	rcu_read_unlock_sched();
 	return 0;
 }
 #endif /* CONFIG_KALLSYMS */
@@ -3379,7 +3386,7 @@ const struct exception_table_entry *search_module_extables(unsigned long addr)
 	const struct exception_table_entry *e = NULL;
 	struct module *mod;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 	list_for_each_entry_rcu(mod, &modules, list) {
 		if (mod->num_exentries == 0)
 			continue;
@@ -3390,7 +3397,7 @@ const struct exception_table_entry *search_module_extables(unsigned long addr)
 		if (e)
 			break;
 	}
-	preempt_enable();
+	rcu_read_unlock_sched();
 
 	/* Now, if we found one, we are running inside it now, hence
 	   we cannot unload the module, hence no refcnt needed. */
@@ -3408,9 +3415,9 @@ bool is_module_address(unsigned long addr)
 {
 	bool ret;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 	ret = __module_address(addr) != NULL;
-	preempt_enable();
+	rcu_read_unlock_sched();
 
 	return ret;
 }
@@ -3419,7 +3426,7 @@ bool is_module_address(unsigned long addr)
  * __module_address - get the module which contains an address.
  * @addr: the address.
  *
- * Must be called with preempt disabled or module mutex held so that
+ * Must be called with rcu_read_lock_sched or module mutex held so that
  * module doesn't get freed during this.
  */
 struct module *__module_address(unsigned long addr)
@@ -3449,9 +3456,9 @@ bool is_module_text_address(unsigned long addr)
 {
 	bool ret;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 	ret = __module_text_address(addr) != NULL;
-	preempt_enable();
+	rcu_read_unlock_sched();
 
 	return ret;
 }
@@ -3460,7 +3467,7 @@ bool is_module_text_address(unsigned long addr)
  * __module_text_address - get the module whose code contains an address.
  * @addr: the address.
  *
- * Must be called with preempt disabled or module mutex held so that
+ * Must be called with rcu_read_lock_sched or module mutex held so that
  * module doesn't get freed during this.
  */
 struct module *__module_text_address(unsigned long addr)
@@ -3484,10 +3491,10 @@ void print_modules(void)
 
 	printk(KERN_DEFAULT "Modules linked in:");
 	/* Most callers should already have preempt disabled, but make sure */
-	preempt_disable();
+	rcu_read_lock_sched();
 	list_for_each_entry_rcu(mod, &modules, list)
 		printk(" %s%s", mod->name, module_flags(mod, buf));
-	preempt_enable();
+	rcu_read_unlock_sched();
 	if (last_unloaded_module[0])
 		printk(" [last unloaded: %s]", last_unloaded_module);
 	printk("\n");
-- 
1.7.7.6

[V2 PATCH 2/3] module: avoid exporting module_mutex

[Cong Wang]

Avoid exporting internal locks to modules, export two
functions lock_modules()/unlock_modules() for them instead.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 drivers/gpu/drm/drm_fb_helper.c |    4 ++--
 include/linux/module.h          |    3 ++-
 kernel/kprobes.c                |    4 ++--
 kernel/module.c                 |   24 ++++++++++++++++++------
 4 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index aada26f..c184759 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1451,9 +1451,9 @@ static int __init drm_fb_helper_modinit(void)
 	const char *name = "fbcon";
 	struct module *fbcon;
 
-	mutex_lock(&module_mutex);
+	lock_modules();
 	fbcon = find_module(name);
-	mutex_unlock(&module_mutex);
+	unlock_modules();
 
 	if (!fbcon)
 		request_module_nowait(name);
diff --git a/include/linux/module.h b/include/linux/module.h
index 4598bf0..5cf6428 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -374,7 +374,8 @@ struct module
 #define MODULE_ARCH_INIT {}
 #endif
 
-extern struct mutex module_mutex;
+extern void lock_modules(void);
+extern void unlock_modules(void);
 
 /* FIXME: It'd be nice to isolate modules during init, too, so they
    aren't used before they (may) fail.  But presently too much code
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index c62b854..0670fb1 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -562,7 +562,7 @@ static __kprobes void kprobe_optimizer(struct work_struct *work)
 	LIST_HEAD(free_list);
 
 	/* Lock modules while optimizing kprobes */
-	mutex_lock(&module_mutex);
+	lock_modules();
 	mutex_lock(&kprobe_mutex);
 
 	/*
@@ -587,7 +587,7 @@ static __kprobes void kprobe_optimizer(struct work_struct *work)
 	do_free_cleaned_kprobes(&free_list);
 
 	mutex_unlock(&kprobe_mutex);
-	mutex_unlock(&module_mutex);
+	unlock_modules();
 
 	/* Step 5: Kick optimizer again if needed */
 	if (!list_empty(&optimizing_list) || !list_empty(&unoptimizing_list))
diff --git a/kernel/module.c b/kernel/module.c
index f0f8d4a..db402a8 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -95,8 +95,20 @@
  * 2) module_use links,
  * 3) module_addr_min/module_addr_max.
  * (delete uses stop_machine/add uses RCU list operations). */
-DEFINE_MUTEX(module_mutex);
-EXPORT_SYMBOL_GPL(module_mutex);
+static DEFINE_MUTEX(module_mutex);
+
+void lock_modules(void)
+{
+	mutex_lock(&module_mutex);
+}
+EXPORT_SYMBOL(lock_modules);
+
+void unlock_modules(void)
+{
+	mutex_unlock(&module_mutex);
+}
+EXPORT_SYMBOL(unlock_modules);
+
 static LIST_HEAD(modules);
 #ifdef CONFIG_KGDB_KDB
 struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */
@@ -1713,7 +1725,7 @@ void set_all_modules_text_rw(void)
 {
 	struct module *mod;
 
-	mutex_lock(&module_mutex);
+	lock_modules();
 	list_for_each_entry(mod, &modules, list) {
 		if ((mod->module_core) && (mod->core_text_size)) {
 			set_page_attributes(mod->module_core,
@@ -1726,7 +1738,7 @@ void set_all_modules_text_rw(void)
 						set_memory_rw);
 		}
 	}
-	mutex_unlock(&module_mutex);
+	unlock_modules();
 }
 
 /* Iterate through all modules and set each module's text as RO */
@@ -1734,7 +1746,7 @@ void set_all_modules_text_ro(void)
 {
 	struct module *mod;
 
-	mutex_lock(&module_mutex);
+	lock_modules();
 	list_for_each_entry(mod, &modules, list) {
 		if ((mod->module_core) && (mod->core_text_size)) {
 			set_page_attributes(mod->module_core,
@@ -1747,7 +1759,7 @@ void set_all_modules_text_ro(void)
 						set_memory_ro);
 		}
 	}
-	mutex_unlock(&module_mutex);
+	unlock_modules();
 }
 #else
 static inline void set_section_ro_nx(void *base, unsigned long text_size, unsigned long ro_size, unsigned long total_size) { }
-- 
1.7.7.6

[V2 PATCH 3/3] module: dd missing synchronize_sched()

[Cong Wang]

I am not sure about this patch, my understanding is that
after every list del/add, we should call synchronize_sched()
to mark the end of update. Does this patch make sense?

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 kernel/module.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index db402a8..e1d8fae 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1785,6 +1785,7 @@ static void free_module(struct module *mod)
 	mutex_lock(&module_mutex);
 	stop_machine(__unlink_module, mod, NULL);
 	mutex_unlock(&module_mutex);
+	synchronize_sched();
 	mod_sysfs_teardown(mod);
 
 	/* Remove dynamic debug info */
@@ -2935,6 +2936,7 @@ static struct module *load_module(void __user *umod,
 	module_bug_finalize(info.hdr, info.sechdrs, mod);
 	list_add_rcu(&mod->list, &modules);
 	mutex_unlock(&module_mutex);
+	synchronize_sched();
 
 	/* Module is ready to execute: parsing args may do that. */
 	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
-- 
1.7.7.6

Re: [V2 PATCH 1/3] module: use rcu to protect module list read

[Eric Dumazet]

Le dimanche 11 mars 2012 à 18:54 +0800, Cong Wang a écrit :
> V2:
...
>  
> @@ -1639,7 +1639,7 @@ static void mod_sysfs_teardown(struct module *mod)
>  static int __unlink_module(void *_mod)
>  {
>  	struct module *mod = _mod;
> -	list_del(&mod->list);
> +	list_del_rcu(&mod->list);
>  	module_bug_cleanup(mod);
>  	return 0;
>  }
> @@

You mix too many different things in a single patch. 

For example, lets review this __unlink_module() change...

If this was really needed, it should be a single patch so that it can be
a stable submission.

As it is not needed (since its called from stop_machine()), this makes
your whole patch looking suspicious.

I suggest you make a 100% cleanup patch, changing the title as well,
because module code _already_ uses RCU.

"module: use rcu to protect module list read" makes no sense at all.

To meet current RCU API best pratices, you change preempt_enable() by
rcu_read_unlock_sched() and preempt_disable() by rcu_read_lock_sched()

Then, if you believe other changes are needed, submit them with an
explicit changelog explaining the change, not hiding them in a big
cleanup patch.

Ref: Documentation/SubmittingPatches section 3)

Re: [V2 PATCH 1/3] module: use rcu to protect module list read

[Cong Wang]

On 03/12/2012 01:38 AM, Eric Dumazet wrote:
> Le dimanche 11 mars 2012 à 18:54 +0800, Cong Wang a écrit :
>> V2:
> ...
>>
>> @@ -1639,7 +1639,7 @@ static void mod_sysfs_teardown(struct module *mod)
>>   static int __unlink_module(void *_mod)
>>   {
>>   	struct module *mod = _mod;
>> -	list_del(&mod->list);
>> +	list_del_rcu(&mod->list);
>>   	module_bug_cleanup(mod);
>>   	return 0;
>>   }
>> @@
>
> You mix too many different things in a single patch.
>
> For example, lets review this __unlink_module() change...

Oops, sorry I missed this part...

>
> If this was really needed, it should be a single patch so that it can be
> a stable submission.
>
> As it is not needed (since its called from stop_machine()), this makes
> your whole patch looking suspicious.
>
> I suggest you make a 100% cleanup patch, changing the title as well,
> because module code _already_ uses RCU.
>
> "module: use rcu to protect module list read" makes no sense at all.
>
> To meet current RCU API best pratices, you change preempt_enable() by
> rcu_read_unlock_sched() and preempt_disable() by rcu_read_lock_sched()
>
> Then, if you believe other changes are needed, submit them with an
> explicit changelog explaining the change, not hiding them in a big
> cleanup patch.
>
>

Fair enough, Rusty gave me the same suggestion, I will split this patch.

Thanks!