[V3 PATCH 1/7] module: replace preempt_disable with rcu_read_lock_sched

[V3 PATCH 1/7] module: replace preempt_disable with rcu_read_lock_sched

[Cong Wang]

V3: split the patches again and fix some typos

Currently we use preempt_disable() + *_rcu list operation to read module
list, it is more explicit to use rcu_read_lock_sched() directly. This change
should be trivial as rcu_read_lock_sched() is a wrapper of preempt_disable().

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 |   66 +++++++++++++++++++++++++++---------------------------
 1 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 2c93276..6f6a3fd 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 or module_mutex. */
 const struct kernel_symbol *find_symbol(const char *name,
 					struct module **owner,
 					const unsigned long **crc,
@@ -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);
 
@@ -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);
 	if (sym && strong_try_module_get(owner))
 		sym = NULL;
-	preempt_enable();
+	rcu_read_unlock_sched();
 
 	return sym ? (void *)sym->value : NULL;
 }
@@ -3130,7 +3130,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 +3145,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 +3153,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 +3163,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 +3177,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 +3190,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 +3204,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 +3213,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 +3241,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 +3252,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;
 }
 
@@ -3379,7 +3379,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 +3390,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 +3408,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 +3419,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 +3449,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 +3460,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)
@@ -3483,11 +3483,11 @@ void print_modules(void)
 	char buf[8];
 
 	printk(KERN_DEFAULT "Modules linked in:");
-	/* Most callers should already have preempt disabled, but make sure */
-	preempt_disable();
+	/* Most callers should already have rcu_read_lock_sched, but make sure */
+	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

[PATCH 2/7] module: take rcu_read_lock_sched for find_symbol

[Cong Wang]

find_symbol() iterates module list with list_for_each_entry_rcu()
should its caller should hold rcu_read_lock_sched().

This patch changes the module_mutex to rcu_read_lock_sched in
resolve_symbol() and adds rcu_read_lock_sched in verify_export_symbols().

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 |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 6f6a3fd..b31b23f 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1174,7 +1174,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 	const unsigned long *crc;
 	int err;
 
-	mutex_lock(&module_mutex);
+	rcu_read_lock_sched();
 	sym = find_symbol(name, &owner, &crc,
 			  !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);
 	if (!sym)
@@ -1196,7 +1196,7 @@ getname:
 	/* We must make copy under the lock if we failed to get ref. */
 	strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
 unlock:
-	mutex_unlock(&module_mutex);
+	rcu_read_unlock_sched();
 	return sym;
 }
 
@@ -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;
 }
 
-- 
1.7.7.6

[PATCH 3/7] module: take rcu_read_lock_sched for find_module

[Cong Wang]

Similar to find_symbol(), find_module() also iterates module list,
should use list_for_each_entry_rcu() too, thus its callers should
hold rcu_read_lock_sched before calling it.

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 |   42 +++++++++++++++++++++++-------------------
 1 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index b31b23f..f210d74 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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;
 	}
@@ -778,14 +778,16 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 		return -EFAULT;
 	name[MODULE_NAME_LEN-1] = '\0';
 
-	if (mutex_lock_interruptible(&module_mutex) != 0)
-		return -EINTR;
-
+	rcu_read_lock_sched();
 	mod = find_module(name);
 	if (!mod) {
-		ret = -ENOENT;
-		goto out;
+		rcu_read_unlock_sched();
+		return -ENOENT;
 	}
+	rcu_read_unlock_sched();
+
+	if (mutex_lock_interruptible(&module_mutex) != 0)
+		return -EINTR;
 
 	if (!list_empty(&mod->source_list)) {
 		/* Other modules depend on us: get rid of them first. */
@@ -2899,18 +2901,13 @@ static struct module *load_module(void __user *umod,
 	/* Mark state as coming so strong_try_module_get() ignores us. */
 	mod->state = MODULE_STATE_COMING;
 
-	/* Now sew it into the lists so we can get lockdep and oops
-	 * info during argument parsing.  No one should access us, since
-	 * strong_try_module_get() will fail.
-	 * lockdep/oops can run asynchronous, so use the RCU list insertion
-	 * function to insert in a way safe to concurrent readers.
-	 * The mutex protects against concurrent writers.
-	 */
-	mutex_lock(&module_mutex);
+	rcu_read_lock_sched();
 	if (find_module(mod->name)) {
 		err = -EEXIST;
-		goto unlock;
+		rcu_read_unlock_sched();
+		goto free_args;
 	}
+	rcu_read_unlock_sched();
 
 	/* This has to be done once we're sure module name is unique. */
 	dynamic_debug_setup(info.debug, info.num_debug);
@@ -2920,6 +2917,14 @@ static struct module *load_module(void __user *umod,
 	if (err < 0)
 		goto ddebug;
 
+	/* Now sew it into the lists so we can get lockdep and oops
+	 * info during argument parsing.  No one should access us, since
+	 * strong_try_module_get() will fail.
+	 * lockdep/oops can run asynchronous, so use the RCU list insertion
+	 * function to insert in a way safe to concurrent readers.
+	 * The mutex protects against concurrent writers.
+	 */
+	mutex_lock(&module_mutex);
 	module_bug_finalize(info.hdr, info.sechdrs, mod);
 	list_add_rcu(&mod->list, &modules);
 	mutex_unlock(&module_mutex);
@@ -2946,12 +2951,11 @@ static struct module *load_module(void __user *umod,
 	/* Unlink carefully: kallsyms could be walking list. */
 	list_del_rcu(&mod->list);
 	module_bug_cleanup(mod);
-
- ddebug:
-	dynamic_debug_remove(info.debug);
- unlock:
 	mutex_unlock(&module_mutex);
 	synchronize_sched();
+ ddebug:
+	dynamic_debug_remove(info.debug);
+ free_args:
 	kfree(mod->args);
  free_arch_cleanup:
 	module_arch_cleanup(mod);
-- 
1.7.7.6

[PATCH 4/7] module: take rcu_read_lock_sched in module_kallsyms_on_each_symbol()

[Cong Wang]

module_kallsyms_on_each_symbol() should iterate the module list
with list_for_each_entry_rcu() after holding rcu_read_lock_sched().

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 |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index f210d74..73e1f95 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3271,14 +3271,18 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 	unsigned int i;
 	int ret;
 
-	list_for_each_entry(mod, &modules, list) {
+	rcu_read_lock_sched();
+	list_for_each_entry_rcu(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 */
-- 
1.7.7.6

[PATCH 5/7] module: take rcu_read_lock_sched() for /proc read

[Cong Wang]

In the seq file operations, it still hold module_mutex for just reading
the module list, switch to rcu_read_lock_sched() too.

A new function seq_list_start_rcu() is introduced for this.

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>

---
 fs/seq_file.c            |   12 ++++++++++++
 include/linux/seq_file.h |    2 ++
 kernel/module.c          |    6 +++---
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 4023d6b..5b01e04 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -674,6 +674,18 @@ struct list_head *seq_list_start(struct list_head *head, loff_t pos)
 }
 EXPORT_SYMBOL(seq_list_start);
 
+struct list_head *seq_list_start_rcu(struct list_head *head, loff_t pos)
+{
+	struct list_head *lh = head;
+
+	list_for_each_continue_rcu(lh, head)
+		if (pos-- == 0)
+			return lh;
+
+	return NULL;
+}
+EXPORT_SYMBOL(seq_list_start_rcu);
+
 struct list_head *seq_list_start_head(struct list_head *head, loff_t pos)
 {
 	if (!pos)
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 44f1514..7848189 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -130,6 +130,8 @@ int seq_release_private(struct inode *, struct file *);
 
 extern struct list_head *seq_list_start(struct list_head *head,
 		loff_t pos);
+extern struct list_head *seq_list_start_rcu(struct list_head *head,
+		loff_t pos);
 extern struct list_head *seq_list_start_head(struct list_head *head,
 		loff_t pos);
 extern struct list_head *seq_list_next(void *v, struct list_head *head,
diff --git a/kernel/module.c b/kernel/module.c
index 73e1f95..06e7dc5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3313,8 +3313,8 @@ static char *module_flags(struct module *mod, char *buf)
 /* Called by the /proc file system to return a list of modules. */
 static void *m_start(struct seq_file *m, loff_t *pos)
 {
-	mutex_lock(&module_mutex);
-	return seq_list_start(&modules, *pos);
+	rcu_read_lock_sched();
+	return seq_list_start_rcu(&modules, *pos);
 }
 
 static void *m_next(struct seq_file *m, void *p, loff_t *pos)
@@ -3324,7 +3324,7 @@ static void *m_next(struct seq_file *m, void *p, loff_t *pos)
 
 static void m_stop(struct seq_file *m, void *p)
 {
-	mutex_unlock(&module_mutex);
+	rcu_read_unlock_sched();
 }
 
 static int m_show(struct seq_file *m, void *p)
-- 
1.7.7.6

[PATCH 6/7] module: add two missing synchronize_sched()

[Cong Wang]

In RCU doc, it said synchronize_sched() is used to mark the end
of update, so two synchronize_sched() are missing after
module list add/del.

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 06e7dc5..2e7a5c5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1775,6 +1775,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 */
@@ -2928,6 +2929,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

[PATCH 7/7] module: avoid exporting module_mutex

[Cong Wang]

drm_fb_helper_modinit() calls find_module() which only
needs rcu_read_lock_sched().

The rest seems still have to hold module_mutex to prevent
module insertion/deletion. Instead of exporting module_mutex
for them, export two functions lock_modules()/unlock_modules()
and hide module_mutex.

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>
---
 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..a3aee0e 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);
+	rcu_read_lock_sched();
 	fbcon = find_module(name);
-	mutex_unlock(&module_mutex);
+	rcu_read_unlock_sched();
 
 	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 2e7a5c5..b60f882 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 */
@@ -1715,7 +1727,7 @@ void set_all_modules_text_rw(void)
 {
 	struct module *mod;
 
-	mutex_lock(&module_mutex);
+	lock_modules();
 	list_for_each_entry_rcu(mod, &modules, list) {
 		if ((mod->module_core) && (mod->core_text_size)) {
 			set_page_attributes(mod->module_core,
@@ -1728,7 +1740,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 */
@@ -1736,7 +1748,7 @@ void set_all_modules_text_ro(void)
 {
 	struct module *mod;
 
-	mutex_lock(&module_mutex);
+	lock_modules();
 	list_for_each_entry_rcu(mod, &modules, list) {
 		if ((mod->module_core) && (mod->core_text_size)) {
 			set_page_attributes(mod->module_core,
@@ -1749,7 +1761,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

Re: [PATCH 5/7] module: take rcu_read_lock_sched() for /proc read

[Eric Dumazet]

On Thu, 2012-03-15 at 22:48 +0800, Cong Wang wrote:
> In the seq file operations, it still hold module_mutex for just reading
> the module list, switch to rcu_read_lock_sched() too.
> 

Why is it needed ?

1) insmod/rmmod/lsmod is hardly in fast path.

2) after a mutex_lock(), a task is still preemptible
   after rcu_read_lock_sched(), preemption is disabled

   with NR_CPUS=4096, getting module refcounts is _very_ slow

RCU doesnt fit every workload.

Re: [PATCH 6/7] module: add two missing synchronize_sched()

[Eric Dumazet]

On Thu, 2012-03-15 at 22:49 +0800, Cong Wang wrote:
> In RCU doc, it said synchronize_sched() is used to mark the end
> of update, so two synchronize_sched() are missing after
> module list add/del.
> 
> 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 06e7dc5..2e7a5c5 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1775,6 +1775,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 */
> @@ -2928,6 +2929,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);

Have a look at stop_machine() first

after list_add_rcu(), there is absolutely no such requirement.

Re: [PATCH 3/7] module: take rcu_read_lock_sched for find_module

[Eric Dumazet]

On Thu, 2012-03-15 at 22:48 +0800, Cong Wang wrote:
> Similar to find_symbol(), find_module() also iterates module list,
> should use list_for_each_entry_rcu() too, thus its callers should
> hold rcu_read_lock_sched before calling it.
> 
> 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 |   42 +++++++++++++++++++++++-------------------
>  1 files changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index b31b23f..f210d74 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -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;
>  	}
> @@ -778,14 +778,16 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  		return -EFAULT;
>  	name[MODULE_NAME_LEN-1] = '\0';
>  
> -	if (mutex_lock_interruptible(&module_mutex) != 0)
> -		return -EINTR;
> -
> +	rcu_read_lock_sched();
>  	mod = find_module(name);
>  	if (!mod) {
> -		ret = -ENOENT;
> -		goto out;
> +		rcu_read_unlock_sched();
> +		return -ENOENT;
>  	}
> +	rcu_read_unlock_sched();
> +

Thats a bug.

> +	if (mutex_lock_interruptible(&module_mutex) != 0)
> +		return -EINTR;

mod might already had disappear.

>  
>  	if (!list_empty(&mod->source_list)) {
>  		/* Other modules depend on us: get rid of them first. */