[PATCH] Re-xmit: module code tidyup

Rusty Russell (rusty@rustcorp.com.au)
Tue, 01 Apr 2003 09:21:27 +1000


Linus, please apply.

Three patches (applicable with one patch -p1):

o Make module_text_address return the module found, not just true/false.
o Remove struct kernel_symbol_group: just iterate through the modules.
o Remove struct exception_table: just iterate through the modules.

These cleanups made possible by the fact that symbol and exception
table traversal now entirely within kernel/module.c.

Thanks,
Rusty.

--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: module_text_address returns the module Author: Rusty Russell Status: Trivial

D: By making module_text_address return the module it found, we D: simplify symbol_put_addr significantly.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .19470-linux-2.5.66-bk2/include/linux/module.h .19470-linux-2.5.66-bk2.updated/include/linux/module.h --- .19470-linux-2.5.66-bk2/include/linux/module.h 2003-03-25 12:17:31.000000000 +1100 +++ .19470-linux-2.5.66-bk2.updated/include/linux/module.h 2003-03-27 15:08:54.000000000 +1100 @@ -268,7 +268,7 @@ static inline int module_is_live(struct } /* Is this address in a module? */ -int module_text_address(unsigned long addr); +struct module *module_text_address(unsigned long addr); #ifdef CONFIG_MODULE_UNLOAD @@ -361,9 +361,9 @@ search_module_extables(unsigned long add } /* Is this address in a module? */ -static inline int module_text_address(unsigned long addr) +static inline struct module *module_text_address(unsigned long addr) { - return 0; + return NULL; } /* Get/put a kernel symbol (calls should be symmetric) */ diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .19470-linux-2.5.66-bk2/kernel/extable.c .19470-linux-2.5.66-bk2.updated/kernel/extable.c --- .19470-linux-2.5.66-bk2/kernel/extable.c 2003-02-07 19:20:44.000000000 +1100 +++ .19470-linux-2.5.66-bk2.updated/kernel/extable.c 2003-03-27 15:08:54.000000000 +1100 @@ -38,5 +38,5 @@ int kernel_text_address(unsigned long ad addr <= (unsigned long)_etext) return 1; - return module_text_address(addr); + return module_text_address(addr) != NULL; } diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .19470-linux-2.5.66-bk2/kernel/module.c .19470-linux-2.5.66-bk2.updated/kernel/module.c --- .19470-linux-2.5.66-bk2/kernel/module.c 2003-03-18 05:01:52.000000000 +1100 +++ .19470-linux-2.5.66-bk2.updated/kernel/module.c 2003-03-27 15:09:34.000000000 +1100 @@ -552,23 +552,14 @@ EXPORT_SYMBOL(__symbol_put); void symbol_put_addr(void *addr) { - struct kernel_symbol_group *ks; unsigned long flags; spin_lock_irqsave(&modlist_lock, flags); - list_for_each_entry(ks, &symbols, list) { - unsigned int i; + if (!kernel_text_address((unsigned long)addr)) + BUG(); - for (i = 0; i < ks->num_syms; i++) { - if (ks->syms[i].value == (unsigned long)addr) { - module_put(ks->owner); - spin_unlock_irqrestore(&modlist_lock, flags); - return; - } - } - } + module_put(module_text_address((unsigned long)addr)); spin_unlock_irqrestore(&modlist_lock, flags); - BUG(); } EXPORT_SYMBOL_GPL(symbol_put_addr); @@ -1545,15 +1536,15 @@ const struct exception_table_entry *sear } /* Is this a valid kernel address? We don't grab the lock: we are oopsing. */ -int module_text_address(unsigned long addr) +struct module *module_text_address(unsigned long addr) { struct module *mod; list_for_each_entry(mod, &modules, list) if (within(addr, mod->module_init, mod->init_size) || within(addr, mod->module_core, mod->core_size)) - return 1; - return 0; + return mod; + return NULL; } /* Provided by the linker */

Name: Symbol list removal Author: Rusty Russell Status: Tested on 2.5.66-bk2 Depends: Module/module-text-address.patch.gz

D: This removes the symbol list, and the concept of kernel symbol groups, D: in favour of just iterating through the modules. Now all iteration is D: within kernel/module.c, this is a fairly trivial cleanup.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .21809-linux-2.5.61-bk1/include/linux/module.h .21809-linux-2.5.61-bk1.updated/include/linux/module.h --- .21809-linux-2.5.61-bk1/include/linux/module.h 2003-02-17 15:47:09.000000000 +1100 +++ .21809-linux-2.5.61-bk1.updated/include/linux/module.h 2003-02-17 15:47:42.000000000 +1100 @@ -115,22 +115,6 @@ extern const struct gtype##_id __mod_##g #define MODULE_DEVICE_TABLE(type,name) \ MODULE_GENERIC_TABLE(type##_device,name) -struct kernel_symbol_group -{ - /* Links us into the global symbol list */ - struct list_head list; - - /* Module which owns it (if any) */ - struct module *owner; - - /* Are we internal use only? */ - int gplonly; - - unsigned int num_syms; - const struct kernel_symbol *syms; - const unsigned long *crcs; -}; - /* Given an address, look for it in the exception tables */ const struct exception_table_entry *search_exception_tables(unsigned long add); @@ -214,10 +198,14 @@ struct module char name[MODULE_NAME_LEN]; /* Exported symbols */ - struct kernel_symbol_group symbols; + const struct kernel_symbol *syms; + unsigned int num_ksyms; + const unsigned long *crcs; /* GPL-only exported symbols. */ - struct kernel_symbol_group gpl_symbols; + const struct kernel_symbol *gpl_syms; + unsigned int num_gpl_syms; + const unsigned long *gpl_crcs; /* Exception tables */ struct exception_table extable; @@ -411,8 +399,6 @@ extern struct module __this_module; struct module __this_module __attribute__((section(".gnu.linkonce.this_module"))) = { .name = __stringify(KBUILD_MODNAME), - .symbols = { .owner = &__this_module }, - .gpl_symbols = { .owner = &__this_module, .gplonly = 1 }, .init = init_module, #ifdef CONFIG_MODULE_UNLOAD .exit = cleanup_module, @@ -472,14 +458,6 @@ static inline void __deprecated _MOD_INC #define EXPORT_NO_SYMBOLS #define __MODULE_STRING(x) __stringify(x) -/* - * The exception and symbol tables, and the lock - * to protect them. - */ -extern spinlock_t modlist_lock; -extern struct list_head extables; -extern struct list_head symbols; - /* Use symbol_get and symbol_put instead. You'll thank me. */ #define HAVE_INTER_MODULE extern void inter_module_register(const char *, struct module *, const void *); diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .21809-linux-2.5.61-bk1/kernel/module.c .21809-linux-2.5.61-bk1.updated/kernel/module.c --- .21809-linux-2.5.61-bk1/kernel/module.c 2003-02-17 15:47:09.000000000 +1100 +++ .21809-linux-2.5.61-bk1.updated/kernel/module.c 2003-02-17 15:48:45.000000000 +1100 @@ -58,7 +58,6 @@ static spinlock_t modlist_lock = SPIN_LO /* List of modules, protected by module_mutex AND modlist_lock */ static DECLARE_MUTEX(module_mutex); static LIST_HEAD(modules); -static LIST_HEAD(symbols); static LIST_HEAD(extables); /* We require a truly strong try_module_get() */ @@ -76,25 +75,60 @@ int init_module(void) } EXPORT_SYMBOL(init_module); -/* Find a symbol, return value and the symbol group */ +/* Provided by the linker */ +extern const struct kernel_symbol __start___ksymtab[]; +extern const struct kernel_symbol __stop___ksymtab[]; +extern const struct kernel_symbol __start___ksymtab_gpl[]; +extern const struct kernel_symbol __stop___ksymtab_gpl[]; +extern const unsigned long __start___kcrctab[]; +extern const unsigned long __start___kcrctab_gpl[]; + +#ifndef CONFIG_MODVERSIONS +#define symversion(base, idx) NULL +#else +#define symversion(base, idx) ((base) ? ((base) + (idx)) : NULL) +#endif + +/* Find a symbol, return value, crc and module which owns it */ static unsigned long __find_symbol(const char *name, - struct kernel_symbol_group **group, - unsigned int *symidx, + struct module **owner, + const unsigned long **crc, int gplok) { - struct kernel_symbol_group *ks; - - list_for_each_entry(ks, &symbols, list) { - unsigned int i; + struct module *mod; + unsigned int i; - if (ks->gplonly && !gplok) - continue; - for (i = 0; i < ks->num_syms; i++) { - if (strcmp(ks->syms[i].name, name) == 0) { - *group = ks; - if (symidx) - *symidx = i; - return ks->syms[i].value; + /* Core kernel first. */ + *owner = NULL; + for (i = 0; __start___ksymtab+i < __stop___ksymtab; i++) { + if (strcmp(__start___ksymtab[i].name, name) == 0) { + *crc = symversion(__start___kcrctab, i); + return __start___ksymtab[i].value; + } + } + if (gplok) { + for (i = 0; __start___ksymtab_gpl+i<__stop___ksymtab_gpl; i++) + if (strcmp(__start___ksymtab_gpl[i].name, name) == 0) { + *crc = symversion(__start___kcrctab_gpl, i); + return __start___ksymtab_gpl[i].value; + } + } + + /* Now try modules. */ + list_for_each_entry(mod, &modules, list) { + *owner = mod; + for (i = 0; i < mod->num_ksyms; i++) + if (strcmp(mod->syms[i].name, name) == 0) { + *crc = symversion(mod->crcs, i); + return mod->syms[i].value; + } + + if (gplok) { + for (i = 0; i < mod->num_gpl_syms; i++) { + if (strcmp(mod->gpl_syms[i].name, name) == 0) { + *crc = symversion(mod->crcs, i); + return mod->gpl_syms[i].value; + } } } } @@ -520,13 +554,14 @@ static void print_unload_info(struct seq void __symbol_put(const char *symbol) { - struct kernel_symbol_group *ksg; + struct module *owner; unsigned long flags; + const unsigned long *crc; spin_lock_irqsave(&modlist_lock, flags); - if (!__find_symbol(symbol, &ksg, NULL, 1)) + if (!__find_symbol(symbol, &owner, &crc, 1)) BUG(); - module_put(ksg->owner); + module_put(owner); spin_unlock_irqrestore(&modlist_lock, flags); } EXPORT_SYMBOL(__symbol_put); @@ -724,19 +759,15 @@ static int check_version(Elf_Shdr *sechd unsigned int versindex, const char *symname, struct module *mod, - struct kernel_symbol_group *ksg, - unsigned int symidx) + const unsigned long *crc) { - unsigned long crc; unsigned int i, num_versions; struct modversion_info *versions; /* Exporting module didn't supply crcs? OK, we're already tainted. */ - if (!ksg->crcs) + if (!crc) return 1; - crc = ksg->crcs[symidx]; - versions = (void *) sechdrs[versindex].sh_addr; num_versions = sechdrs[versindex].sh_size / sizeof(struct modversion_info); @@ -745,12 +776,12 @@ static int check_version(Elf_Shdr *sechd if (strcmp(versions[i].name, symname) != 0) continue; - if (versions[i].crc == crc) + if (versions[i].crc == *crc) return 1; printk("%s: disagrees about version of symbol %s\n", mod->name, symname); DEBUGP("Found checksum %lX vs module %lX\n", - crc, versions[i].crc); + *crc, versions[i].crc); return 0; } /* Not in module's version table. OK, but that taints the kernel. */ @@ -786,8 +817,7 @@ static inline int check_version(Elf_Shdr unsigned int versindex, const char *symname, struct module *mod, - struct kernel_symbol_group *ksg, - unsigned int symidx) + const unsigned long *crc) { return 1; } @@ -812,17 +842,16 @@ static unsigned long resolve_symbol(Elf_ const char *name, struct module *mod) { - struct kernel_symbol_group *ksg; + struct module *owner; unsigned long ret; - unsigned int symidx; + const unsigned long *crc; spin_lock_irq(&modlist_lock); - ret = __find_symbol(name, &ksg, &symidx, mod->license_gplok); + ret = __find_symbol(name, &owner, &crc, mod->license_gplok); if (ret) { /* use_module can fail due to OOM, or module unloading */ - if (!check_version(sechdrs, versindex, name, mod, - ksg, symidx) || - !use_module(mod, ksg->owner)) + if (!check_version(sechdrs, versindex, name, mod, crc) || + !use_module(mod, owner)) ret = 0; } spin_unlock_irq(&modlist_lock); @@ -835,8 +864,6 @@ static void free_module(struct module *m /* Delete from various lists */ spin_lock_irq(&modlist_lock); list_del(&mod->list); - list_del(&mod->symbols.list); - list_del(&mod->gpl_symbols.list); list_del(&mod->extable.list); spin_unlock_irq(&modlist_lock); @@ -853,12 +880,13 @@ static void free_module(struct module *m void *__symbol_get(const char *symbol) { - struct kernel_symbol_group *ksg; + struct module *owner; unsigned long value, flags; + const unsigned long *crc; spin_lock_irqsave(&modlist_lock, flags); - value = __find_symbol(symbol, &ksg, NULL, 1); - if (value && !strong_try_module_get(ksg->owner)) + value = __find_symbol(symbol, &owner, &crc, 1); + if (value && !strong_try_module_get(owner)) value = 0; spin_unlock_irqrestore(&modlist_lock, flags); @@ -1297,21 +1325,17 @@ static struct module *load_module(void * goto cleanup; /* Set up EXPORTed & EXPORT_GPLed symbols (section 0 is 0 length) */ - mod->symbols.num_syms = (sechdrs[exportindex].sh_size - / sizeof(*mod->symbols.syms)); - mod->symbols.syms = (void *)sechdrs[exportindex].sh_addr; + mod->num_syms = sechdrs[exportindex].sh_size / sizeof(*mod->syms); + mod->syms = (void *)sechdrs[exportindex].sh_addr; if (crcindex) - mod->symbols.crcs = (void *)sechdrs[crcindex].sh_addr; - - mod->gpl_symbols.num_syms = (sechdrs[gplindex].sh_size - / sizeof(*mod->symbols.syms)); - mod->gpl_symbols.syms = (void *)sechdrs[gplindex].sh_addr; + mod->crcs = (void *)sechdrs[crcindex].sh_addr; + mod->num_gpl_syms = sechdrs[gplindex].sh_size / sizeof(*mod->gpl_syms); + mod->gpl_syms = (void *)sechdrs[gplindex].sh_addr; if (gplcrcindex) - mod->gpl_symbols.crcs = (void *)sechdrs[gplcrcindex].sh_addr; + mod->gpl_crcs = (void *)sechdrs[gplcrcindex].sh_addr; #ifdef CONFIG_MODVERSIONS - if ((mod->symbols.num_syms && !crcindex) - || (mod->gpl_symbols.num_syms && !gplcrcindex)) { + if ((mod->num_ksyms&&!crcindex) || (mod->num_gpl_syms&&!gplcrcindex)) { printk(KERN_WARNING "%s: No versions for exported symbols." " Tainting kernel.\n", mod->name); tainted |= TAINT_FORCED_MODULE; @@ -1420,8 +1444,6 @@ sys_init_module(void *umod, strong_try_module_get() will fail. */ spin_lock_irq(&modlist_lock); list_add(&mod->extable.list, &extables); - list_add_tail(&mod->symbols.list, &symbols); - list_add_tail(&mod->gpl_symbols.list, &symbols); list_add(&mod->list, &modules); spin_unlock_irq(&modlist_lock); @@ -1614,39 +1636,6 @@ struct module *module_text_address(unsig return NULL; } -/* Provided by the linker */ -extern const struct kernel_symbol __start___ksymtab[]; -extern const struct kernel_symbol __stop___ksymtab[]; -extern const struct kernel_symbol __start___ksymtab_gpl[]; -extern const struct kernel_symbol __stop___ksymtab_gpl[]; -extern const unsigned long __start___kcrctab[]; -extern const unsigned long __stop___kcrctab[]; -extern const unsigned long __start___kcrctab_gpl[]; -extern const unsigned long __stop___kcrctab_gpl[]; - -static struct kernel_symbol_group kernel_symbols, kernel_gpl_symbols; - -static int __init symbols_init(void) -{ - /* Add kernel symbols to symbol table */ - kernel_symbols.num_syms = (__stop___ksymtab - __start___ksymtab); - kernel_symbols.syms = __start___ksymtab; - kernel_symbols.crcs = __start___kcrctab; - kernel_symbols.gplonly = 0; - list_add(&kernel_symbols.list, &symbols); - - kernel_gpl_symbols.num_syms = (__stop___ksymtab_gpl - - __start___ksymtab_gpl); - kernel_gpl_symbols.syms = __start___ksymtab_gpl; - kernel_gpl_symbols.crcs = __start___kcrctab_gpl; - kernel_gpl_symbols.gplonly = 1; - list_add(&kernel_gpl_symbols.list, &symbols); - - return 0; -} - -__initcall(symbols_init); - #ifdef CONFIG_MODVERSIONS /* Generate the signature for struct module here, too, for modversions. */ void struct_module(struct module *mod) { return; } b_gpl; - kernel_gpl_symbols.gplonly = 1; - list_add(&kernel_gpl_symbols.list, &symbols); - - return 0; + return mod; + return NULL; } - -__initcall(symbols_init);

Name: Extable list removal Author: Rusty Russell Status: Tested on 2.5.66-bk2 Depends: Module/symbol-list.patch.gz

D: This removes the extable list, and the struct exception_table, in D: favour of just iterating through the modules. Now all iteration is D: within kernel/module.c, this is a fairly trivial cleanup.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .21085-2.5.60-bk3-extable-list.pre/include/linux/module.h .21085-2.5.60-bk3-extable-list/include/linux/module.h --- .21085-2.5.60-bk3-extable-list.pre/include/linux/module.h 2003-02-14 23:30:39.000000000 +1100 +++ .21085-2.5.60-bk3-extable-list/include/linux/module.h 2003-02-14 23:30:39.000000000 +1100 @@ -18,7 +18,6 @@ #include <linux/stringify.h> #include <asm/module.h> -#include <asm/uaccess.h> /* For struct exception_table_entry */ /* Not Yet Implemented */ #define MODULE_AUTHOR(name) @@ -51,6 +50,8 @@ extern int init_module(void); extern void cleanup_module(void); /* Archs provide a method of finding the correct exception table. */ +struct exception_table_entry; + const struct exception_table_entry * search_extable(const struct exception_table_entry *first, const struct exception_table_entry *last, @@ -116,15 +117,6 @@ extern const struct gtype##_id __mod_##g /* Given an address, look for it in the exception tables */ const struct exception_table_entry *search_exception_tables(unsigned long add); -struct exception_table -{ - struct list_head list; - - unsigned int num_entries; - const struct exception_table_entry *entry; -}; - - #ifdef CONFIG_MODULES /* Get/put a kernel symbol (calls must be symmetric) */ @@ -205,8 +197,9 @@ struct module unsigned int num_gpl_syms; const unsigned long *gpl_crcs; - /* Exception tables */ - struct exception_table extable; + /* Exception table */ + unsigned int num_exentries; + const struct exception_table_entry *extable; /* Startup function. */ int (*init)(void); diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .21085-2.5.60-bk3-extable-list.pre/kernel/extable.c .21085-2.5.60-bk3-extable-list/kernel/extable.c --- .21085-2.5.60-bk3-extable-list.pre/kernel/extable.c 2003-02-14 23:30:39.000000000 +1100 +++ .21085-2.5.60-bk3-extable-list/kernel/extable.c 2003-02-14 23:30:39.000000000 +1100 @@ -16,6 +16,7 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ #include <linux/module.h> +#include <asm/uaccess.h> extern const struct exception_table_entry __start___ex_table[]; extern const struct exception_table_entry __stop___ex_table[]; diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .21085-2.5.60-bk3-extable-list.pre/kernel/module.c .21085-2.5.60-bk3-extable-list/kernel/module.c --- .21085-2.5.60-bk3-extable-list.pre/kernel/module.c 2003-02-14 23:30:39.000000000 +1100 +++ .21085-2.5.60-bk3-extable-list/kernel/module.c 2003-02-14 23:30:39.000000000 +1100 @@ -51,13 +51,12 @@ #define symbol_is(literal, string) \ (strcmp(MODULE_SYMBOL_PREFIX literal, (string)) == 0) -/* Protects extables and symbols lists */ +/* Protects module list */ static spinlock_t modlist_lock = SPIN_LOCK_UNLOCKED; /* List of modules, protected by module_mutex AND modlist_lock */ static DECLARE_MUTEX(module_mutex); static LIST_HEAD(modules); -static LIST_HEAD(extables); /* We require a truly strong try_module_get() */ static inline int strong_try_module_get(struct module *mod) @@ -845,7 +841,6 @@ static void free_module(struct module *m /* Delete from various lists */ spin_lock_irq(&modlist_lock); list_del(&mod->list); - list_del(&mod->extable.list); spin_unlock_irq(&modlist_lock); /* Module unload stuff */ @@ -1312,14 +1308,9 @@ static struct module *load_module(void * } #endif - /* Set up exception table */ - if (exindex) { - /* FIXME: Sort exception table. */ - mod->extable.num_entries = (sechdrs[exindex].sh_size - / sizeof(struct - exception_table_entry)); - mod->extable.entry = (void *)sechdrs[exindex].sh_addr; - } + /* Set up exception table */ + mod->num_exentries = sechdrs[exindex].sh_size / sizeof(*mod->extable); + mod->extable = (void *)sechdrs[exindex].sh_addr; /* Now handle each section. */ for (i = 1; i < hdr->e_shnum; i++) { @@ -1421,7 +1411,6 @@ sys_init_module(void *umod, /* Now sew it into the lists. They won't access us, since strong_try_module_get() will fail. */ spin_lock_irq(&modlist_lock); - list_add(&mod->extable.list, &extables); list_add(&mod->list, &modules); spin_unlock_irq(&modlist_lock); @@ -1584,14 +1573,16 @@ const struct exception_table_entry *sear { unsigned long flags; const struct exception_table_entry *e = NULL; - struct exception_table *i; + struct module *mod; spin_lock_irqsave(&modlist_lock, flags); - list_for_each_entry(i, &extables, list) { - if (i->num_entries == 0) + list_for_each_entry(mod, &modules, list) { + if (mod->num_exentries == 0) continue; - e = search_extable(i->entry, i->entry+i->num_entries-1, addr); + e = search_extable(mod->extable, + mod->extable + mod->num_exentries - 1, + addr); if (e) break; } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/