[PATCH] call sysrq_timer_list_show from a workqueue

[PATCH] call sysrq_timer_list_show from a workqueue

[Kyle McMartin]

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 ]

Signed-off-by: Kyle McMartin <kyle@mcmartin.ca>
---
diff --git a/drivers/char/sysrq.c b/drivers/char/sysrq.c
index de60e1e..09bb030 100644
--- a/drivers/char/sysrq.c
+++ b/drivers/char/sysrq.c
@@ -159,10 +159,16 @@ static struct sysrq_key_op sysrq_sync_op = {
 	.enable_mask	= SYSRQ_ENABLE_SYNC,
 };
 
-static void sysrq_handle_show_timers(int key, struct tty_struct *tty)
+static void sysrq_show_timers_callback(struct work_struct *whocares)
 {
 	sysrq_timer_list_show();
 }
+static DECLARE_WORK(show_timers_work, sysrq_show_timers_callback);
+
+static void sysrq_handle_show_timers(int key, struct tty_struct *tty)
+{
+	schedule_work(&show_timers_work);
+}
 
 static struct sysrq_key_op sysrq_show_timers_op = {
 	.handler	= sysrq_handle_show_timers,

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Ingo Molnar]

(Rusty Cc:-ed)

* Kyle McMartin <kyle@mcmartin.ca> 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?

	Ingo

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@mcmartin.ca> 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@rustcorp.com.au>

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 */

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Ingo Molnar]

* Rusty Russell <rusty@rustcorp.com.au> wrote:

> +/* FIXME: Risky: returns a pointer into a module w/o lock */

stupid question: since module unloads are so rare, why isnt this via the 
same mechanism that CPU hotplug uses to securely unregister CPUs? I.e. 
quiet all CPUs, disable irqs on all of them, then unlink the module. 
This would make module unload fundamentally safe, and symbols could be 
looked up lockless. (with the exception of crashing within NMIs, but 
that is not lock safe anyway.)

Or is this a rathole for some reason?

	Ingo

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Rusty Russell]

On Tuesday 08 January 2008 22:33:23 Ingo Molnar wrote:
> * Rusty Russell <rusty@rustcorp.com.au> wrote:
> > +/* FIXME: Risky: returns a pointer into a module w/o lock */
>
> stupid question: since module unloads are so rare, why isnt this via the
> same mechanism that CPU hotplug uses to securely unregister CPUs? I.e.
> quiet all CPUs, disable irqs on all of them, then unlink the module.

That's what we do.  This old locking stuff is legacy.

And here's the patch for the FIXME (which I put in to remind myself):

Make module_address_lookup safe

module_address_lookup releases preemption then returns a pointer into
the module space.  The only user (kallsyms) copies the result, so just
do that under the preempt disable.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r 27c34f677af7 include/linux/module.h
--- a/include/linux/module.h	Tue Jan 08 22:27:02 2008 +1100
+++ b/include/linux/module.h	Tue Jan 08 22:44:14 2008 +1100
@@ -446,11 +446,14 @@ static inline void __module_get(struct m
 	__mod ? __mod->name : "kernel";		\
 })
 
-/* For kallsyms to ask for address resolution.  NULL means not found. */
-const char *module_address_lookup(unsigned long addr,
-				  unsigned long *symbolsize,
-				  unsigned long *offset,
-				  char **modname);
+/* For kallsyms to ask for address resolution.  namebuf should be at
+ * least KSYM_NAME_LEN long: a pointer to namebuf is returned if
+ * found, otherwise NULL. */
+char *module_address_lookup(unsigned long addr,
+			    unsigned long *symbolsize,
+			    unsigned long *offset,
+			    char **modname,
+			    char *namebuf);
 int lookup_module_symbol_name(unsigned long addr, char *symname);
 int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
 
@@ -516,10 +519,11 @@ static inline void module_put(struct mod
 #define module_name(mod) "kernel"
 
 /* For kallsyms to ask for address resolution.  NULL means not found. */
-static inline const char *module_address_lookup(unsigned long addr,
-						unsigned long *symbolsize,
-						unsigned long *offset,
-						char **modname)
+static inline char *module_address_lookup(unsigned long addr,
+					  unsigned long *symbolsize,
+					  unsigned long *offset,
+					  char **modname,
+					  char *namebuf)
 {
 	return NULL;
 }
diff -r 27c34f677af7 kernel/kallsyms.c
--- a/kernel/kallsyms.c	Tue Jan 08 22:27:02 2008 +1100
+++ b/kernel/kallsyms.c	Tue Jan 08 22:44:14 2008 +1100
@@ -233,10 +233,11 @@ int kallsyms_lookup_size_offset(unsigned
 int kallsyms_lookup_size_offset(unsigned long addr, unsigned long *symbolsize,
 				unsigned long *offset)
 {
+	char namebuf[KSYM_NAME_LEN];
 	if (is_ksym_addr(addr))
 		return !!get_symbol_pos(addr, symbolsize, offset);
 
-	return !!module_address_lookup(addr, symbolsize, offset, NULL);
+	return !!module_address_lookup(addr, symbolsize, offset, NULL, namebuf);
 }
 
 /*
@@ -251,8 +252,6 @@ const char *kallsyms_lookup(unsigned lon
 			    unsigned long *offset,
 			    char **modname, char *namebuf)
 {
-	const char *msym;
-
 	namebuf[KSYM_NAME_LEN - 1] = 0;
 	namebuf[0] = 0;
 
@@ -268,10 +267,8 @@ const char *kallsyms_lookup(unsigned lon
 	}
 
 	/* see if it's in a module */
-	msym = module_address_lookup(addr, symbolsize, offset, modname);
-	if (msym)
-		return strncpy(namebuf, msym, KSYM_NAME_LEN - 1);
-
+	return module_address_lookup(addr, symbolsize, offset, modname,
+				     namebuf);
 	return NULL;
 }
 
diff -r 27c34f677af7 kernel/module.c
--- a/kernel/module.c	Tue Jan 08 22:27:02 2008 +1100
+++ b/kernel/module.c	Tue Jan 08 22:44:14 2008 +1100
@@ -2235,14 +2235,13 @@ static const char *get_ksymbol(struct mo
 	return mod->strtab + mod->symtab[best].st_name;
 }
 
-/* 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)
+/* For kallsyms to ask for address resolution.  NULL means not found.  Careful
+ * not to lock to avoid deadlock on oopses, simply disable preemption. */
+char *module_address_lookup(unsigned long addr,
+			    unsigned long *size,
+			    unsigned long *offset,
+			    char **modname,
+			    char *namebuf)
 {
 	struct module *mod;
 	const char *ret = NULL;
@@ -2256,6 +2255,11 @@ const char *module_address_lookup(unsign
 			ret = get_ksymbol(mod, addr, size, offset);
 			break;
 		}
+	}
+	/* Make a copy in here where it's safe */
+	if (ret) {
+		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
+		ret = namebuf;
 	}
 	preempt_enable();
 	return ret;

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Ingo Molnar]

* Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Tuesday 08 January 2008 22:33:23 Ingo Molnar wrote:
> > * Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > +/* FIXME: Risky: returns a pointer into a module w/o lock */
> >
> > stupid question: since module unloads are so rare, why isnt this via 
> > the same mechanism that CPU hotplug uses to securely unregister 
> > CPUs? I.e. quiet all CPUs, disable irqs on all of them, then unlink 
> > the module.
> 
> That's what we do.  This old locking stuff is legacy.

oh, wonderful :-)

> And here's the patch for the FIXME (which I put in to remind myself):
> 
> Make module_address_lookup safe

cool :-)

finally we have a _really_ sane symbols subsystem. Kudos :)

	Ingo

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Rusty Russell]

On Wednesday 09 January 2008 02:51:19 Ingo Molnar wrote:
> cool :-)
>
> finally we have a _really_ sane symbols subsystem. Kudos :)

Actually, the interface is horribly scattered.  It'd be nice for someone to go 
through and look at what the minimum we actually need is and come up with a 
system to organize them.

BTW, do you want this fix in 2.6.24?
Rusty.

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Ingo Molnar]

* Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Wednesday 09 January 2008 02:51:19 Ingo Molnar wrote:
> > cool :-)
> >
> > finally we have a _really_ sane symbols subsystem. Kudos :)
> 
> Actually, the interface is horribly scattered.  It'd be nice for 
> someone to go through and look at what the minimum we actually need is 
> and come up with a system to organize them.
> 
> BTW, do you want this fix in 2.6.24?

hm, i'm not sure it should go into v2.6.24 without first being exposed 
in v2.6.25 for some time - symbol lookup has been historically a bit 
fragile (hopefully this will change now) , and the worst thing that can 
happen during a crash printout is for the debug code to regress. No 
strong feelings though.

	Ingo

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Andrew Morton]

On Tue, 8 Jan 2008 22:50:06 +1100
Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Tuesday 08 January 2008 22:33:23 Ingo Molnar wrote:
> > * Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > +/* FIXME: Risky: returns a pointer into a module w/o lock */
> >
> > stupid question: since module unloads are so rare, why isnt this via the
> > same mechanism that CPU hotplug uses to securely unregister CPUs? I.e.
> > quiet all CPUs, disable irqs on all of them, then unlink the module.
> 
> That's what we do.  This old locking stuff is legacy.
> 
> And here's the patch for the FIXME (which I put in to remind myself):
> 
> Make module_address_lookup safe
> 
> module_address_lookup releases preemption then returns a pointer into
> the module space.  The only user (kallsyms) copies the result, so just
> do that under the preempt disable.
> 
> ...
>
> -/* 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)
> +/* For kallsyms to ask for address resolution.  NULL means not found.  Careful
> + * not to lock to avoid deadlock on oopses, simply disable preemption. */
> +char *module_address_lookup(unsigned long addr,
> +			    unsigned long *size,
> +			    unsigned long *offset,
> +			    char **modname,
> +			    char *namebuf)
>  {
>  	struct module *mod;
>  	const char *ret = NULL;
> @@ -2256,6 +2255,11 @@ const char *module_address_lookup(unsign
>  			ret = get_ksymbol(mod, addr, size, offset);
>  			break;
>  		}
> +	}
> +	/* Make a copy in here where it's safe */
> +	if (ret) {
> +		strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
> +		ret = namebuf;
>  	}
>  	preempt_enable();
>  	return ret;

The string handling in here has become a bit scruffy.

afacit the `namebuf[KSYM_NAME_LEN - 1] = 0;' would be unneeded if we were
to use strlcpy() and I suspect the `namebuf[0] = 0;' isn't needed either. 

And the use of strlcpy() means we don't need to subtract 1 from
KSYM_NAME_LEN and we don't need to fret about weird strncpy semantics when
the input string is too large.


And the fact that incoming arg `namebuf' MUST point at a
KSYM_NAME_LEN-sized buffer could be better communicated by using a
dedicated struct for this, or by giving the arg a type of `char
namebuf[KSYM_NAME_LEN]'.  Or by adding a comment. Or by just ignoring
me and doing something more useful.

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Rusty Russell]

On Wednesday 09 January 2008 11:21:59 Andrew Morton wrote:
> The string handling in here has become a bit scruffy.

Yes, that patch also evokes a const warning.  Fixed below.  I assume you've
queued these because you're thinking of applying them before 2.6.24?  I'd say
only modules-de-mutex-more-symbol-lookup-paths-in-the-module-code.patch
warrants that (the other is unlikely and not a regression).

> afacit the `namebuf[KSYM_NAME_LEN - 1] = 0;' would be unneeded if we were
> to use strlcpy() and I suspect the `namebuf[0] = 0;' isn't needed either.
>
> And the use of strlcpy() means we don't need to subtract 1 from
> KSYM_NAME_LEN and we don't need to fret about weird strncpy semantics when
> the input string is too large.
>
>
> And the fact that incoming arg `namebuf' MUST point at a
> KSYM_NAME_LEN-sized buffer could be better communicated by using a
> dedicated struct for this, or by giving the arg a type of `char
> namebuf[KSYM_NAME_LEN]'.  Or by adding a comment. Or by just ignoring
> me and doing something more useful.

Or better, rework all the name lookup interfaces, rather than having: 

struct module *module_text_address(unsigned long addr);
struct module *__module_text_address(unsigned long addr);
int is_module_address(unsigned long addr);
int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
			char *name, char *module_name, int *exported);
char *module_address_lookup(unsigned long addr,
			    unsigned long *symbolsize,
			    unsigned long *offset,
			    char **modname,
			    char *namebuf);
int lookup_module_symbol_name(unsigned long addr, char *symname);
int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size,
			       unsigned long *offset, char *modname, char *name);
unsigned long module_kallsyms_lookup_name(const char *name);

unsigned long kallsyms_lookup_name(const char *name);
extern int kallsyms_lookup_size_offset(unsigned long addr,
				  unsigned long *symbolsize,
				  unsigned long *offset);
const char *kallsyms_lookup(unsigned long addr,
			    unsigned long *symbolsize,
			    unsigned long *offset,
			    char **modname, char *namebuf);
extern int sprint_symbol(char *buffer, unsigned long address);
extern void __print_symbol(const char *fmt, unsigned long address);
int lookup_symbol_name(unsigned long addr, char *symname);
int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
		        unsigned long *offset, char *modname, char *name);

Cheers,
Rusty.

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Andrew Morton]

On Wed, 9 Jan 2008 14:20:18 +1100 Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Wednesday 09 January 2008 11:21:59 Andrew Morton wrote:
> > The string handling in here has become a bit scruffy.
> 
> Yes, that patch also evokes a const warning.  Fixed below.

No patch was included.

>  I assume you've
> queued these because you're thinking of applying them before 2.6.24?  I'd say
> only modules-de-mutex-more-symbol-lookup-paths-in-the-module-code.patch
> warrants that (the other is unlikely and not a regression).

Actually I was thinking 2.6.25 on both.

<looks>

   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.

Ah.  sysrq_timer_list_show() is called from interrupt.

<fixes changelog, thwaps its author>

OK, 2.6.24 seems reasonable.

> > afacit the `namebuf[KSYM_NAME_LEN - 1] = 0;' would be unneeded if we were
> > to use strlcpy() and I suspect the `namebuf[0] = 0;' isn't needed either.
> >
> > And the use of strlcpy() means we don't need to subtract 1 from
> > KSYM_NAME_LEN and we don't need to fret about weird strncpy semantics when
> > the input string is too large.
> >
> >
> > And the fact that incoming arg `namebuf' MUST point at a
> > KSYM_NAME_LEN-sized buffer could be better communicated by using a
> > dedicated struct for this, or by giving the arg a type of `char
> > namebuf[KSYM_NAME_LEN]'.  Or by adding a comment. Or by just ignoring
> > me and doing something more useful.
> 
> Or better, rework all the name lookup interfaces, rather than having: 
> 
> struct module *module_text_address(unsigned long addr);
> struct module *__module_text_address(unsigned long addr);
> int is_module_address(unsigned long addr);
> int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> 			char *name, char *module_name, int *exported);
> char *module_address_lookup(unsigned long addr,
> 			    unsigned long *symbolsize,
> 			    unsigned long *offset,
> 			    char **modname,
> 			    char *namebuf);
> int lookup_module_symbol_name(unsigned long addr, char *symname);
> int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size,
> 			       unsigned long *offset, char *modname, char *name);
> unsigned long module_kallsyms_lookup_name(const char *name);
> 
> unsigned long kallsyms_lookup_name(const char *name);
> extern int kallsyms_lookup_size_offset(unsigned long addr,
> 				  unsigned long *symbolsize,
> 				  unsigned long *offset);
> const char *kallsyms_lookup(unsigned long addr,
> 			    unsigned long *symbolsize,
> 			    unsigned long *offset,
> 			    char **modname, char *namebuf);
> extern int sprint_symbol(char *buffer, unsigned long address);
> extern void __print_symbol(const char *fmt, unsigned long address);
> int lookup_symbol_name(unsigned long addr, char *symname);
> int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
> 		        unsigned long *offset, char *modname, char *name);

Yes, it could all do with a revisit.

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Rusty Russell]

On Wednesday 09 January 2008 14:33:50 Andrew Morton wrote:
> On Wed, 9 Jan 2008 14:20:18 +1100 Rusty Russell <rusty@rustcorp.com.au> 
wrote:
> > On Wednesday 09 January 2008 11:21:59 Andrew Morton wrote:
> > > The string handling in here has become a bit scruffy.
> >
> > Yes, that patch also evokes a const warning.  Fixed below.
>
> No patch was included.

Yes, I decided it's a secret.  Mine, all mine!

> >  I assume you've
> > queued these because you're thinking of applying them before 2.6.24?  I'd
> > say only
> > modules-de-mutex-more-symbol-lookup-paths-in-the-module-code.patch
> > warrants that (the other is unlikely and not a regression).
>
> Actually I was thinking 2.6.25 on both.

Then, you should get them next time you grab my series, no?  Or is that 
particular lever not working yet?

Hmm, I see my link was not updated (damn, ln -sfn, not ln -sf!).  Fixed now:
	http://ozlabs.org/~rusty/kernel/rr-latest/

More goodies there than a UK comedy convention...

> OK, 2.6.24 seems reasonable.

Kyle acked it at least...

> Yes, it could all do with a revisit.

And it goes without saying that glory awaits they who succeed...
Rusty.

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Andrew Morton]

On Wed, 9 Jan 2008 15:27:59 +1100 Rusty Russell <rusty@rustcorp.com.au> wrote:

> > >  I assume you've
> > > queued these because you're thinking of applying them before 2.6.24?  I'd
> > > say only
> > > modules-de-mutex-more-symbol-lookup-paths-in-the-module-code.patch
> > > warrants that (the other is unlikely and not a regression).
> >
> > Actually I was thinking 2.6.25 on both.
> 
> Then, you should get them next time you grab my series, no?  Or is that 
> particular lever not working yet?
> 
> Hmm, I see my link was not updated (damn, ln -sfn, not ln -sf!).  Fixed now:
> 	http://ozlabs.org/~rusty/kernel/rr-latest/
> 
> More goodies there than a UK comedy convention...

My 850-email backlog is down to 759.  You're in there somewhere.  I'm
wondering if I can spin it out to next Christmas.  

I may end up throwing up my hands, trolling it all for bugfixes and then
having an accident with the rest.

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Paulo Marques]

Rusty Russell wrote:
> On Wednesday 09 January 2008 11:21:59 Andrew Morton wrote:
>> [...]
>> And the fact that incoming arg `namebuf' MUST point at a
>> KSYM_NAME_LEN-sized buffer could be better communicated by using a
>> dedicated struct for this, or by giving the arg a type of `char
>> namebuf[KSYM_NAME_LEN]'.  Or by adding a comment. Or by just ignoring
>> me and doing something more useful.
> 
> Or better, rework all the name lookup interfaces, rather than having: 

Yes, there is some rework we can do here....

> struct module *module_text_address(unsigned long addr);
> struct module *__module_text_address(unsigned long addr);
> int is_module_address(unsigned long addr);
> int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> 			char *name, char *module_name, int *exported);
> char *module_address_lookup(unsigned long addr,
> 			    unsigned long *symbolsize,
> 			    unsigned long *offset,
> 			    char **modname,
> 			    char *namebuf);
> int lookup_module_symbol_name(unsigned long addr, char *symname);
> int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size,
> 			       unsigned long *offset, char *modname, char *name);
> unsigned long module_kallsyms_lookup_name(const char *name);

All of these are somewhat less important, because most users call the 
kallsyms generic functions which will in turn call these functions if 
the symbol isn't global.

So, they should suffer basically the same transformations as the 
kallsyms_ counterparts and can be considered almost "internal" to the 
kallsyms infrastructure.

> unsigned long kallsyms_lookup_name(const char *name);

This one look fine, as there is no duplication in any other function.

> extern int kallsyms_lookup_size_offset(unsigned long addr,
> 				  unsigned long *symbolsize,
> 				  unsigned long *offset);
> const char *kallsyms_lookup(unsigned long addr,
> 			    unsigned long *symbolsize,
> 			    unsigned long *offset,
> 			    char **modname, char *namebuf);
> int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
> 		        unsigned long *offset, char *modname, char *name);
> int lookup_symbol_name(unsigned long addr, char *symname);

These 4 functions can probably be condensed into just one, by allowing 
NULL pointer arguments to mean "don't need this result":

kallsyms_lookup_size_offset(a,s,o) <=> kallsyms_lookup(a,s,o,NULL,NULL)
lookup_symbol_attrs(a,s,o,m,n) <=> kallsyms_lookup(a,s,o,m,n)
lookup_symbol_name(a,n) <=> kallsyms_lookup(a,NULL,NULL,NULL,n)

> extern int sprint_symbol(char *buffer, unsigned long address);
> extern void __print_symbol(const char *fmt, unsigned long address);

These 2 are probably fine.

There is a difference in the way the module name is passed, because 
kallsyms_lookup assumes it can return just a pointer to the module name.

However, we should probably change that interface so that the caller 
provides the buffer to hold the module name, to avoid races. The 
stop_machine should help already, but returning a pointer that can be 
stale just a little bit later isn't pretty anyway.

I can do a patch for this, but this will touch a few subsystems that use 
these interfaces (there are not a lot of them, though). The major change 
would probably be the allocation of a small buffer (56~60 bytes) in some 
of the callers to hold the module name.

-- 
Paulo Marques - www.grupopie.com

"There cannot be a crisis today; my schedule is already full."

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Rusty Russell]

On Thursday 10 January 2008 02:24:40 Paulo Marques wrote:
> Rusty Russell wrote:
> > Or better, rework all the name lookup interfaces, rather than having:
>
> Yes, there is some rework we can do here....

Hi Paulo,

	Yes, it just needs some thought...

> > extern int sprint_symbol(char *buffer, unsigned long address);
> > extern void __print_symbol(const char *fmt, unsigned long address);
>
> These 2 are probably fine.

Except they're awful for the !CONFIG_KALLSYMS case.  You really want something 
that that prints the name if available and the address otherwise.

> I can do a patch for this, but this will touch a few subsystems that use
> these interfaces (there are not a lot of them, though). The major change
> would probably be the allocation of a small buffer (56~60 bytes) in some
> of the callers to hold the module name.

Indeed.

I look forward to your patch!
Rusty.

Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Kyle McMartin]

On Tue, Jan 08, 2008 at 10:28:04PM +1100, Rusty Russell wrote:
> 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@rustcorp.com.au>
> 

ACK that it (obviously) fixes the lock checker spew.

Signed-off-by: Kyle McMartin <kyle@mcmartin.ca>

cheers, Kyle