Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Rusty Russell]

On Tuesday 08 January 2008 20:17:42 Ingo Molnar wrote:
> (Rusty Cc:-ed)
>
> * Kyle McMartin <kyle@xxxxxxxxxxx> wrote:
> > handle_sysrq can be called from interrupt context.
> > sysrq_timer_list_show eventually starts poking at module symbols which
> > take the module mutex.
> >
> > so instead, let's just kick off a workqueue.
> >
> > [ doesn't happen on my laptop with the keyboard, but does when
> >   triggered from /proc/sysrq-trigger ]
>
> hmmm. This really shouldnt be happening - how come we hit the module
> mutex on simple things like symbol lookups?

We did start demutexing paths to try to get more info out for oopsen, but
lookup_module_symbol_name was not among them.

Hmm, the module symbol lookup interfaces are out of control, but this
should do it:

De-mutex more symbol lookup paths in the module code

Kyle McMartin reports sysrq_timer_list_show() can hit the module
mutex; these paths don't need to though, since we long ago changed all
the module list manipulation to occur via stop_machine().

Disabling preemption is enough.

Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>

diff -r f52abc2cbb4e kernel/module.c
--- a/kernel/module.c	Tue Jan 08 17:39:23 2008 +1100
+++ b/kernel/module.c	Tue Jan 08 22:27:01 2008 +1100
@@ -2238,29 +2238,34 @@ static const char *get_ksymbol(struct mo
 /* For kallsyms to ask for address resolution.  NULL means not found.
    We don't lock, as this is used for oops resolution and races are a
    lesser concern. */
+/* FIXME: Risky: returns a pointer into a module w/o lock */
 const char *module_address_lookup(unsigned long addr,
 				  unsigned long *size,
 				  unsigned long *offset,
 				  char **modname)
 {
 	struct module *mod;
+	const char *ret = NULL;
 
+	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
 		if (within(addr, mod->module_init, mod->init_size)
 		    || within(addr, mod->module_core, mod->core_size)) {
 			if (modname)
 				*modname = mod->name;
-			return get_ksymbol(mod, addr, size, offset);
+			ret = get_ksymbol(mod, addr, size, offset);
+			break;
 		}
 	}
-	return NULL;
+	preempt_enable();
+	return ret;
 }
 
 int lookup_module_symbol_name(unsigned long addr, char *symname)
 {
 	struct module *mod;
 
-	mutex_lock(&module_mutex);
+	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
 		if (within(addr, mod->module_init, mod->init_size) ||
 		    within(addr, mod->module_core, mod->core_size)) {
@@ -2270,12 +2275,12 @@ int lookup_module_symbol_name(unsigned l
 			if (!sym)
 				goto out;
 			strlcpy(symname, sym, KSYM_NAME_LEN);
-			mutex_unlock(&module_mutex);
+			preempt_enable();
 			return 0;
 		}
 	}
 out:
-	mutex_unlock(&module_mutex);
+	preempt_enable();
 	return -ERANGE;
 }
 
@@ -2284,7 +2289,7 @@ int lookup_module_symbol_attrs(unsigned 
 {
 	struct module *mod;
 
-	mutex_lock(&module_mutex);
+	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
 		if (within(addr, mod->module_init, mod->init_size) ||
 		    within(addr, mod->module_core, mod->core_size)) {
@@ -2297,12 +2302,12 @@ int lookup_module_symbol_attrs(unsigned 
 				strlcpy(modname, mod->name, MODULE_NAME_LEN);
 			if (name)
 				strlcpy(name, sym, KSYM_NAME_LEN);
-			mutex_unlock(&module_mutex);
+			preempt_enable();
 			return 0;
 		}
 	}
 out:
-	mutex_unlock(&module_mutex);
+	preempt_enable();
 	return -ERANGE;
 }
 
@@ -2311,7 +2316,7 @@ int module_get_kallsym(unsigned int symn
 {
 	struct module *mod;
 
-	mutex_lock(&module_mutex);
+	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
 		if (symnum < mod->num_symtab) {
 			*value = mod->symtab[symnum].st_value;
@@ -2320,12 +2325,12 @@ int module_get_kallsym(unsigned int symn
 				KSYM_NAME_LEN);
 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
 			*exported = is_exported(name, mod);
-			mutex_unlock(&module_mutex);
+			preempt_enable();
 			return 0;
 		}
 		symnum -= mod->num_symtab;
 	}
-	mutex_unlock(&module_mutex);
+	preempt_enable();
 	return -ERANGE;
 }
 
@@ -2348,6 +2353,7 @@ unsigned long module_kallsyms_lookup_nam
 	unsigned long ret = 0;
 
 	/* Don't lock: we're in enough trouble already. */
+	preempt_disable();
 	if ((colon = strchr(name, ':')) != NULL) {
 		*colon = '\0';
 		if ((mod = find_module(name)) != NULL)
@@ -2358,6 +2364,7 @@ unsigned long module_kallsyms_lookup_nam
 			if ((ret = mod_find_symname(mod, name)) != 0)
 				break;
 	}
+	preempt_enable();
 	return ret;
 }
 #endif /* CONFIG_KALLSYMS */
--
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/