In message <200211281616.gASGGOE6012229@bytesex.org> you write:
> Smells like a deadlock due to request_module() in some modules init
> function or something like this.
This doesn't happen on my test box for any of my modules, so I can't
really check. But I'd appreciate it if you would try the patch below.
Thanks for the report!
Rusty.
-- Anyone who quotes me in their sig is an idiot. -- Rusty Russell.Name: Module init reentry fix Author: Rusty Russell Status: Tested on 2.5.50
D: This changes the code to drop the module_mutex() before calling the D: module's init function, so module init functions can call D: request_module(). This was trivial before someone broke the module D: code to start non-live. Now it requires us to keep info on the D: exact module state.
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.50/include/linux/module.h working-2.5.50-initlock/include/linux/module.h --- linux-2.5.50/include/linux/module.h 2002-11-25 08:44:18.000000000 +1100 +++ working-2.5.50-initlock/include/linux/module.h 2002-11-29 18:57:25.000000000 +1100 @@ -100,12 +100,18 @@ void *__symbol_get_gpl(const char *symbo struct module_ref { atomic_t count; -} ____cacheline_aligned; +} ____cacheline_aligned_in_smp; + +enum module_state +{ + MODULE_STATE_LIVE, + MODULE_STATE_COMING, + MODULE_STATE_GOING, +}; struct module { - /* Am I live (yet)? */ - int live; + enum module_state state; /* Member of list of modules */ struct list_head list; @@ -163,6 +169,14 @@ struct module char args[0]; }; +/* FIXME: It'd be nice to isolate modules during init, too, so they + aren't used before they (may) fail. But presently too much code + (IDE & SCSI) require entry into the module during init.*/ +static inline int module_is_live(struct module *mod) +{ + return mod->state != MODULE_STATE_GOING; +} + #ifdef CONFIG_MODULE_UNLOAD void __symbol_put(const char *symbol); @@ -181,7 +195,7 @@ static inline int try_module_get(struct if (module) { unsigned int cpu = get_cpu(); - if (likely(module->live)) + if (likely(module_is_live(module))) local_inc(&module->ref[cpu].count); else ret = 0; @@ -196,7 +210,7 @@ static inline void module_put(struct mod unsigned int cpu = get_cpu(); local_dec(&module->ref[cpu].count); /* Maybe they're waiting for us to drop reference? */ - if (unlikely(!module->live)) + if (unlikely(!module_is_live(module))) wake_up_process(module->waiter); put_cpu(); } @@ -205,7 +219,7 @@ static inline void module_put(struct mod #else /*!CONFIG_MODULE_UNLOAD*/ static inline int try_module_get(struct module *module) { - return !module || module->live; + return !module || module_is_live(module); } static inline void module_put(struct module *module) { diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.50/kernel/module.c working-2.5.50-initlock/kernel/module.c --- linux-2.5.50/kernel/module.c 2002-11-25 08:44:19.000000000 +1100 +++ working-2.5.50-initlock/kernel/module.c 2002-11-29 18:59:48.000000000 +1100 @@ -41,6 +41,14 @@ static DECLARE_MUTEX(module_mutex); LIST_HEAD(modules); /* FIXME: Accessed w/o lock on oops by some archs */ +/* We require a truly strong try_module_get() */ +static inline int strong_try_module_get(struct module *mod) +{ + if (mod && mod->state == MODULE_STATE_COMING) + return 0; + return try_module_get(mod); +} + /* Convenient structure for holding init and core sizes */ struct sizes { @@ -375,7 +383,7 @@ sys_delete_module(const char *name_user, } /* Already dying? */ - if (!mod->live) { + if (mod->state == MODULE_STATE_GOING) { /* FIXME: if (force), slam module count and wake up waiter --RR */ DEBUGP("%s already dying\n", mod->name); @@ -383,6 +391,16 @@ sys_delete_module(const char *name_user, goto out; } + /* Coming up? Allow force on stuck modules. */ + if (mod->state == MODULE_STATE_COMING) { + forced = try_force(flags); + if (!forced) { + /* This module can't be removed */ + ret = -EBUSY; + goto out; + } + } + if (!mod->exit || mod->unsafe) { forced = try_force(flags); if (!forced) { @@ -404,7 +422,7 @@ sys_delete_module(const char *name_user, ret = -EWOULDBLOCK; } else { mod->waiter = current; - mod->live = 0; + mod->state = MODULE_STATE_GOING; } restart_refcounts(); @@ -504,7 +523,7 @@ static inline void module_unload_free(st static inline int use_module(struct module *a, struct module *b) { - return try_module_get(b); + return strong_try_module_get(b); } static inline void module_unload_init(struct module *mod) @@ -575,7 +594,7 @@ void *__symbol_get(const char *symbol) spin_lock_irqsave(&modlist_lock, flags); value = __find_symbol(symbol, &ksg); - if (value && !try_module_get(ksg->owner)) + if (value && !strong_try_module_get(ksg->owner)) value = 0; spin_unlock_irqrestore(&modlist_lock, flags); @@ -932,12 +951,8 @@ static struct module *load_module(void * goto free_mod; } - /* Initialize the lists, since they will be list_del'd if init fails */ - INIT_LIST_HEAD(&mod->extable.list); - INIT_LIST_HEAD(&mod->list); - INIT_LIST_HEAD(&mod->symbols.list); mod->symbols.owner = mod; - mod->live = 0; + mod->state = MODULE_STATE_COMING; module_unload_init(mod); /* How much space will we need? (Common area in first) */ @@ -1094,51 +1109,39 @@ sys_init_module(void *umod, flush_icache_range((unsigned long)mod->module_core, (unsigned long)mod->module_core + mod->core_size); - /* Now sew it into exception list (just in case...). */ + /* 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_tail(&mod->symbols.list, &symbols); spin_unlock_irq(&modlist_lock); + list_add(&mod->list, &modules); + + /* Drop lock so they can recurse */ + up(&module_mutex); - /* Note, setting the mod->live to 1 here is safe because we haven't - * linked the module into the system's kernel symbol table yet, - * which means that the only way any other kernel code can call - * into this module right now is if this module hands out entry - * pointers to the other code. We assume that no module hands out - * entry pointers to the rest of the kernel unless it is ready to - * have them used. - */ - mod->live = 1; /* Start the module */ ret = mod->init ? mod->init() : 0; if (ret < 0) { /* Init routine failed: abort. Try to protect us from - buggy refcounters. */ + buggy refcounters. */ + mod->state = MODULE_STATE_GOING; synchronize_kernel(); - if (mod->unsafe) { + if (mod->unsafe) printk(KERN_ERR "%s: module is now stuck!\n", mod->name); - /* Mark it "live" so that they can force - deletion later, and we don't keep getting - woken on every decrement. */ - } else { - mod->live = 0; + else { + down(&module_mutex); free_module(mod); - } - up(&module_mutex); + up(&module_mutex); return ret; } /* Now it's a first class citizen! */ - spin_lock_irq(&modlist_lock); - list_add_tail(&mod->symbols.list, &symbols); - spin_unlock_irq(&modlist_lock); - list_add(&mod->list, &modules); - + mod->state = MODULE_STATE_LIVE; module_free(mod, mod->module_init); mod->module_init = NULL; - /* All ok! */ - up(&module_mutex); return 0; } - 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/
This archive was generated by hypermail 2b29 : Sat Nov 30 2002 - 22:00:22 EST