From a2da4052f1df6bc77749f84496fe731ab8b458f7 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Jan 2008 17:13:17 -0500 Subject: [PATCH 1/8] module: Don't report discarded init pages as kernel text. Current code could cause a bug in symbol_put_addr() if an arch used kmalloc module text: we might think the symbol belongs to the core kernel. The downside is that this might make backtraces through (discarded) init functions harder to read on some archs, but we already have that issue for modules and noone has complained. Signed-off-by: Rusty Russell --- kernel/extable.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/extable.c b/kernel/extable.c index 7fe262855317..a26cb2e17023 100644 --- a/kernel/extable.c +++ b/kernel/extable.c @@ -46,7 +46,8 @@ int core_kernel_text(unsigned long addr) addr <= (unsigned long)_etext) return 1; - if (addr >= (unsigned long)_sinittext && + if (system_state == SYSTEM_BOOTING && + addr >= (unsigned long)_sinittext && addr <= (unsigned long)_einittext) return 1; return 0; From c9a3ba55bb5da03fc7d707709a7fe078fe1aa0a0 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Jan 2008 17:13:18 -0500 Subject: [PATCH 2/8] module: wait for dependent modules doing init. There have been reports of modules failing to load because the modules they depend on are still loading. This changes the modules to wait for a reasonable length of time in that case. We time out eventually, because there can be module loops or broken modules. Signed-off-by: Rusty Russell --- kernel/module.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index 1bb4c5e0d56e..17314691d3cc 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -65,6 +65,9 @@ static DEFINE_MUTEX(module_mutex); static LIST_HEAD(modules); +/* Waiting for a module to finish initializing? */ +static DECLARE_WAIT_QUEUE_HEAD(module_wq); + static BLOCKING_NOTIFIER_HEAD(module_notify_list); int register_module_notifier(struct notifier_block * nb) @@ -84,8 +87,11 @@ EXPORT_SYMBOL(unregister_module_notifier); static inline int strong_try_module_get(struct module *mod) { if (mod && mod->state == MODULE_STATE_COMING) + return -EBUSY; + if (try_module_get(mod)) return 0; - return try_module_get(mod); + else + return -ENOENT; } static inline void add_taint_module(struct module *mod, unsigned flag) @@ -539,11 +545,21 @@ static int already_uses(struct module *a, struct module *b) static int use_module(struct module *a, struct module *b) { struct module_use *use; - int no_warn; + int no_warn, err; if (b == NULL || already_uses(a, b)) return 1; - if (!strong_try_module_get(b)) + /* If we're interrupted or time out, we fail. */ + if (wait_event_interruptible_timeout( + module_wq, (err = strong_try_module_get(b)) != -EBUSY, + 30 * HZ) <= 0) { + printk("%s: gave up waiting for init of module %s.\n", + a->name, b->name); + return 0; + } + + /* If strong_try_module_get() returned a different error, we fail. */ + if (err) return 0; DEBUGP("Allocating new usage for %s.\n", a->name); @@ -816,7 +832,7 @@ static inline void module_unload_free(struct module *mod) static inline int use_module(struct module *a, struct module *b) { - return strong_try_module_get(b); + return strong_try_module_get(b) == 0; } static inline void module_unload_init(struct module *mod) @@ -1326,7 +1342,7 @@ void *__symbol_get(const char *symbol) preempt_disable(); value = __find_symbol(symbol, &owner, &crc, 1); - if (value && !strong_try_module_get(owner)) + if (value && strong_try_module_get(owner) != 0) value = 0; preempt_enable(); @@ -2132,6 +2148,7 @@ sys_init_module(void __user *umod, mutex_lock(&module_mutex); free_module(mod); mutex_unlock(&module_mutex); + wake_up(&module_wq); return ret; } @@ -2146,6 +2163,7 @@ sys_init_module(void __user *umod, mod->init_size = 0; mod->init_text_size = 0; mutex_unlock(&module_mutex); + wake_up(&module_wq); return 0; } From efa5345e39d01deef349c120f55ac6b6eabe7457 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Jan 2008 17:13:20 -0500 Subject: [PATCH 3/8] module: Fix gratuitous sprintf in module.c Andrew sent an older version of this patch: we shouldn't use sprintf to copy a string. Signed-off-by: Rusty Russell --- kernel/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/module.c b/kernel/module.c index 17314691d3cc..276abd7b7ff7 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -738,7 +738,7 @@ sys_delete_module(const char __user *name_user, unsigned int flags) mutex_lock(&module_mutex); } /* Store the name of the last unloaded module for diagnostic purposes */ - sprintf(last_unloaded_module, mod->name); + strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module)); free_module(mod); out: From bb9d3d56e792d2619cc0903df4ac01d86ac1261d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Jan 2008 17:13:21 -0500 Subject: [PATCH 4/8] module: better OOPS and lockdep coverage for loading modules If we put the module in the linked list *before* calling into to, we get the module name and functions in the OOPS (is_module_address can find the module). It also helps lockdep in a similar way. Acked-and-tested-by: Joern Engel Tested-by: Erez Zadok Signed-off-by: Rusty Russell --- kernel/module.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index 276abd7b7ff7..12067ff34d01 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1293,6 +1293,17 @@ static void mod_kobject_remove(struct module *mod) kobject_put(&mod->mkobj.kobj); } +/* + * link the module with the whole machine is stopped with interrupts off + * - this defends against kallsyms not taking locks + */ +static int __link_module(void *_mod) +{ + struct module *mod = _mod; + list_add(&mod->list, &modules); + return 0; +} + /* * unlink the module with the whole machine is stopped with interrupts off * - this defends against kallsyms not taking locks @@ -2035,6 +2046,11 @@ static struct module *load_module(void __user *umod, printk(KERN_WARNING "%s: Ignoring obsolete parameters\n", mod->name); + /* Now sew it into the lists so we can get lockdep and oops + * info during argument parsing. Noone should access us, since + * strong_try_module_get() will fail. */ + stop_machine_run(__link_module, mod, NR_CPUS); + /* Size of section 0 is 0, so this works well if no params */ err = parse_args(mod->name, mod->args, (struct kernel_param *) @@ -2043,7 +2059,7 @@ static struct module *load_module(void __user *umod, / sizeof(struct kernel_param), NULL); if (err < 0) - goto arch_cleanup; + goto unlink; err = mod_sysfs_setup(mod, (struct kernel_param *) @@ -2051,7 +2067,7 @@ static struct module *load_module(void __user *umod, sechdrs[setupindex].sh_size / sizeof(struct kernel_param)); if (err < 0) - goto arch_cleanup; + goto unlink; add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs); add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs); @@ -2066,7 +2082,8 @@ static struct module *load_module(void __user *umod, /* Done! */ return mod; - arch_cleanup: + unlink: + stop_machine_run(__unlink_module, mod, NR_CPUS); module_arch_cleanup(mod); cleanup: kobject_del(&mod->mkobj.kobj); @@ -2091,17 +2108,6 @@ static struct module *load_module(void __user *umod, goto free_hdr; } -/* - * link the module with the whole machine is stopped with interrupts off - * - this defends against kallsyms not taking locks - */ -static int __link_module(void *_mod) -{ - struct module *mod = _mod; - list_add(&mod->list, &modules); - return 0; -} - /* This is where the real work happens */ asmlinkage long sys_init_module(void __user *umod, @@ -2126,10 +2132,6 @@ sys_init_module(void __user *umod, return PTR_ERR(mod); } - /* Now sew it into the lists. They won't access us, since - strong_try_module_get() will fail. */ - stop_machine_run(__link_module, mod, NR_CPUS); - /* Drop lock so they can recurse */ mutex_unlock(&module_mutex); From 6dd06c9fbe025f542bce4cdb91790c0f91962722 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Jan 2008 17:13:22 -0500 Subject: [PATCH 5/8] module: 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 --- include/linux/module.h | 22 +++++++++++++--------- kernel/kallsyms.c | 11 ++++------- kernel/module.c | 22 +++++++++++++--------- 3 files changed, 30 insertions(+), 25 deletions(-) diff --git a/include/linux/module.h b/include/linux/module.h index c97bdb7eb957..aedc06be1de8 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -446,11 +446,14 @@ static inline void __module_get(struct module *module) __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 module *module) #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 --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 2fc25810509e..7dadc71ce516 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -233,10 +233,11 @@ static unsigned long get_symbol_pos(unsigned long addr, 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 long addr, 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 long addr, } /* 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 --git a/kernel/module.c b/kernel/module.c index 12067ff34d01..e814cd7da634 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2230,14 +2230,13 @@ static const char *get_ksymbol(struct module *mod, 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; @@ -2252,8 +2251,13 @@ const char *module_address_lookup(unsigned long addr, 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; + return (char *)ret; } int lookup_module_symbol_name(unsigned long addr, char *symname) From 8686c99875f3849047660a5b6d02438443f22ce7 Mon Sep 17 00:00:00 2001 From: Denis Cheng Date: Mon, 21 Jan 2008 17:08:25 +0800 Subject: [PATCH 6/8] module: fix the module name length in param_sysfs_builtin the original code use KOBJ_NAME_LEN for built-in module name length, that's defined to 20 in linux/kobject.h, but this is not enough appearntly, many module names are longer than this; #define KOBJ_NAME_LEN 20 another macro is MODULE_NAME_LEN defined in linux/module.h, I think this is enough for module names: #define MODULE_NAME_LEN (64 - sizeof(unsigned long)) Signed-off-by: Denis Cheng Signed-off-by: Rusty Russell --- kernel/params.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/kernel/params.c b/kernel/params.c index 67f65ee7211d..42fe5e6126c0 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -376,8 +376,6 @@ int param_get_string(char *buffer, struct kernel_param *kp) extern struct kernel_param __start___param[], __stop___param[]; -#define MAX_KBUILD_MODNAME KOBJ_NAME_LEN - struct param_attribute { struct module_attribute mattr; @@ -587,7 +585,7 @@ static void __init param_sysfs_builtin(void) { struct kernel_param *kp, *kp_begin = NULL; unsigned int i, name_len, count = 0; - char modname[MAX_KBUILD_MODNAME + 1] = ""; + char modname[MODULE_NAME_LEN + 1] = ""; for (i=0; i < __stop___param - __start___param; i++) { char *dot; @@ -595,12 +593,12 @@ static void __init param_sysfs_builtin(void) kp = &__start___param[i]; max_name_len = - min_t(size_t, MAX_KBUILD_MODNAME, strlen(kp->name)); + min_t(size_t, MODULE_NAME_LEN, strlen(kp->name)); dot = memchr(kp->name, '.', max_name_len); if (!dot) { DEBUGP("couldn't find period in first %d characters " - "of %s\n", MAX_KBUILD_MODNAME, kp->name); + "of %s\n", MODULE_NAME_LEN, kp->name); continue; } name_len = dot - kp->name; From 0aa5bd52d0c49ca56d24584c646e6544ccbb3dc9 Mon Sep 17 00:00:00 2001 From: Jon Masters Date: Mon, 21 Jan 2008 20:43:41 +0000 Subject: [PATCH 7/8] module: add module taint on ndiswrapper The struct module taints member is supposed to store per-module taint data. The kernel knows about certain specific external modules that will taint the kernel, such as ndiswrapper. Use of ndiswrapper possibly should set the per-module taint in addition to the global kernel taint flag, unless we're arguing not because wrapper module itself is not what actually causes the kernel to be tainted as such? Signed-off-by: Jon Masters Signed-off-by: Rusty Russell --- kernel/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/module.c b/kernel/module.c index e814cd7da634..af3f81a94745 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1916,7 +1916,7 @@ static struct module *load_module(void __user *umod, set_license(mod, get_modinfo(sechdrs, infoindex, "license")); if (strcmp(mod->name, "ndiswrapper") == 0) - add_taint(TAINT_PROPRIETARY_MODULE); + add_taint_module(mod, TAINT_PROPRIETARY_MODULE); if (strcmp(mod->name, "driverloader") == 0) add_taint_module(mod, TAINT_PROPRIETARY_MODULE); From 6494a93d55fad586238cc1940e846c6d03e1aaf6 Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Sun, 27 Jan 2008 15:38:40 -0800 Subject: [PATCH 8/8] Module: check to see if we have a built in module with the same name When trying to load a module with the same name as a built-in one, a scary kobject backtrace comes up. Prevent that from checking for this condition and warning the user as to what exactly is going on. Cc: Rusty Russell Cc: Linus Torvalds Cc: Andrew Morton Signed-off-by: Greg Kroah-Hartman Signed-off-by: Rusty Russell --- kernel/module.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/kernel/module.c b/kernel/module.c index af3f81a94745..f6a4e721fd49 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1230,6 +1230,7 @@ void module_remove_modinfo_attrs(struct module *mod) int mod_sysfs_init(struct module *mod) { int err; + struct kobject *kobj; if (!module_sysfs_initialized) { printk(KERN_ERR "%s: module sysfs not initialized\n", @@ -1237,6 +1238,15 @@ int mod_sysfs_init(struct module *mod) err = -EINVAL; goto out; } + + kobj = kset_find_obj(module_kset, mod->name); + if (kobj) { + printk(KERN_ERR "%s: module is already loaded\n", mod->name); + kobject_put(kobj); + err = -EINVAL; + goto out; + } + mod->mkobj.mod = mod; memset(&mod->mkobj.kobj, 0, sizeof(mod->mkobj.kobj));