call sysrq_timer_list_show from a workqueue

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 ]

(Rusty Cc:-ed)

Quoted 4 lines · 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.

Quoted 4 lines

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?

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

Quoted 1 line · Rusty Russell

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

Quoted 7 lines · On Tuesday 08 January 2008 22:33:23 Ingo Molnar wrote:

Quoted 1 line · Rusty Russell

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

cool :-)

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

Quoted 3 lines · 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?

Quoted 6 lines · On Tuesday 08 January 2008 22:33:23 Ingo Molnar wrote:

Quoted 1 line · Rusty Russell

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

...

kernel/module.c +7 -8

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

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.

Quoted 1 line · 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).

Quoted 8 lines

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.

Quoted 5 lines

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

On Wed, 9 Jan 2008 14:20:18 +1100 Rusty Russell <[email protected]>

Quoted 4 lines · On Wednesday 09 January 2008 11:21:59 Andrew Morton wrote:

Quoted 1 line

The string handling in here has become a bit scruffy.

Yes, that patch also evokes a const warning. Fixed below.

No patch was included.

Quoted 5 lines

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.

OK, 2.6.24 seems reasonable.

Yes, it could all do with a revisit.

Quoted 7 lines

Quoted 5 lines

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

Quoted 6 lines · 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:

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

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

Quoted 1 line · Rusty Russell

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

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

Quoted 2 lines

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

These 2 are probably fine.

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.

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 <[email protected]>