Name: Module mutex removal Author: Rusty Russell Status: Tested on 2.5.56: Crashes Under stress_modules.sh Depends: D: This removes the module mutex, and relies on the modlist_lock D: spinlock entirely. This means that the lock is not held over the D: module_exit() function, which is good if that oopses (which D: previous renders all future module operations useless). diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .13848-linux-2.5.65/include/linux/module.h .13848-linux-2.5.65.updated/include/linux/module.h --- .13848-linux-2.5.65/include/linux/module.h 2003-03-19 07:58:22.000000000 +1100 +++ .13848-linux-2.5.65.updated/include/linux/module.h 2003-03-19 07:58:24.000000000 +1100 @@ -168,9 +168,10 @@ struct module_ref enum module_state { - MODULE_STATE_LIVE, - MODULE_STATE_COMING, - MODULE_STATE_GOING, + MODULE_STATE_LIVE, /* Active */ + MODULE_STATE_COMING, /* Inside module_init function */ + MODULE_STATE_GOING, /* Inside module_exit function */ + MODULE_STATE_GOING_SLOWLY, /* Waiting for refcount to hit zero */ }; struct module @@ -249,7 +250,8 @@ struct module (IDE & SCSI) require entry into the module during init.*/ static inline int module_is_live(struct module *mod) { - return mod->state != MODULE_STATE_GOING; + return mod->state == MODULE_STATE_COMING + || mod->state == MODULE_STATE_LIVE; } /* Is this address in a module? */ diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .13848-linux-2.5.65/kernel/module.c .13848-linux-2.5.65.updated/kernel/module.c --- .13848-linux-2.5.65/kernel/module.c 2003-03-19 07:58:22.000000000 +1100 +++ .13848-linux-2.5.65.updated/kernel/module.c 2003-03-19 08:01:20.000000000 +1100 @@ -1,6 +1,6 @@ /* Rewritten by Rusty Russell, on the backs of many others... Copyright (C) 2002 Richard Henderson - Copyright (C) 2001 Rusty Russell, 2002 Rusty Russell IBM. + Copyright (C) 2001 Rusty Russell, 2002, 2003 Rusty Russell IBM. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -54,9 +54,6 @@ /* 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); /* We require a truly strong try_module_get() */ @@ -169,7 +166,7 @@ static unsigned long find_local_symbol(E return 0; } -/* Search for module by name: must hold module_mutex. */ +/* Search for module by name: must hold modlist_lock. */ static struct module *find_module(const char *name) { struct module *mod; @@ -340,6 +337,9 @@ static int stop_refcounts(void) unsigned long old_allowed; int ret = 0; + /* No CPUs can come up or down during this, nor other stop_refcounts */ + down(&cpucontrol); + /* One thread per cpu. We'll do our own. */ cpu = smp_processor_id(); @@ -351,9 +351,6 @@ static int stop_refcounts(void) stopref_num_threads = 0; stopref_state = STOPREF_WAIT; - /* No CPUs can come up or down during this. */ - down(&cpucontrol); - for (i = 0; i < NR_CPUS; i++) { if (i == cpu || !cpu_online(i)) continue; @@ -451,8 +448,14 @@ sys_delete_module(const char *name_user, return -EFAULT; name[MODULE_NAME_LEN-1] = '\0'; - if (down_interruptible(&module_mutex) != 0) - return -EINTR; + /* Stop the machine so refcounts can't move: irqs disabled. */ + DEBUGP("Stopping refcounts...\n"); + ret = stop_refcounts(); + if (ret != 0) + return ret; + + /* This is actually gratuitous, as machine is stopped. */ + spin_lock(&modlist_lock); mod = find_module(name); if (!mod) { @@ -495,31 +498,27 @@ sys_delete_module(const char *name_user, goto out; } } - /* Stop the machine so refcounts can't move: irqs disabled. */ - DEBUGP("Stopping refcounts...\n"); - ret = stop_refcounts(); - if (ret != 0) - goto out; /* If it's not unused, quit unless we are told to block. */ if ((flags & O_NONBLOCK) && module_refcount(mod) != 0) { forced = try_force(flags); - if (!forced) + if (!forced) { ret = -EWOULDBLOCK; - } else { - mod->waiter = current; - mod->state = MODULE_STATE_GOING; + goto out; + } } - restart_refcounts(); + mod->waiter = current; + if (flags & O_NONBLOCK) + mod->state = MODULE_STATE_GOING; + else + mod->state = MODULE_STATE_GOING_SLOWLY; - if (ret != 0) - goto out; + spin_unlock(&modlist_lock); + restart_refcounts(); if (forced) goto destroy; - /* Since we might sleep for some time, drop the semaphore first */ - up(&module_mutex); for (;;) { DEBUGP("Looking at refcount...\n"); set_current_state(TASK_UNINTERRUPTIBLE); @@ -529,16 +528,14 @@ sys_delete_module(const char *name_user, } current->state = TASK_RUNNING; - DEBUGP("Regrabbing mutex...\n"); - down(&module_mutex); - destroy: /* Final destruction now noone is using it. */ mod->exit(); free_module(mod); - + return 0; out: - up(&module_mutex); + spin_unlock(&modlist_lock); + restart_refcounts(); return ret; } @@ -853,8 +850,7 @@ static inline int same_magic(const char } #endif /* CONFIG_MODVERSIONS */ -/* Resolve a symbol for this module. I.e. if we find one, record usage. - Must be holding module_mutex. */ +/* Resolve a symbol for this module. I.e. if we find one, record usage. */ static unsigned long resolve_symbol(Elf_Shdr *sechdrs, unsigned int versindex, const char *name, @@ -876,14 +872,8 @@ static unsigned long resolve_symbol(Elf_ return ret; } -/* Free a module, remove from lists, etc (must hold module mutex). */ -static void free_module(struct module *mod) +static void __free_module(struct module *mod) { - /* Delete from various lists */ - spin_lock_irq(&modlist_lock); - list_del(&mod->list); - spin_unlock_irq(&modlist_lock); - /* Module unload stuff */ module_unload_free(mod); @@ -895,6 +885,16 @@ static void free_module(struct module *m module_free(mod, mod->module_core); } +/* Free a module, remove from lists, etc. */ +static void free_module(struct module *mod) +{ + /* Delete from various lists */ + spin_lock_irq(&modlist_lock); + list_del(&mod->list); + spin_unlock_irq(&modlist_lock); + __free_module(mod); +} + void *__symbol_get(const char *symbol) { struct module *owner; @@ -1332,6 +1332,14 @@ static struct module *load_module(void * else return ptr; } +/* We want to load this module: drop lock and wait if neccessary. */ +static int module_already_exists(const char *name) +{ + if (find_module(name)) + return 1; + return 0; +} + /* This is where the real work happens */ asmlinkage long sys_init_module(void *umod, @@ -1345,16 +1353,10 @@ sys_init_module(void *umod, if (!capable(CAP_SYS_MODULE)) return -EPERM; - /* Only one module load at a time, please */ - if (down_interruptible(&module_mutex) != 0) - return -EINTR; - /* Do all the hard work */ mod = load_module(umod, len, uargs); - if (IS_ERR(mod)) { - up(&module_mutex); + if (IS_ERR(mod)) return PTR_ERR(mod); - } /* Flush the instruction cache, since we've played with text */ if (mod->module_init) @@ -1367,37 +1369,38 @@ 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); + if (module_already_exists(mod->name)) { + __free_module(mod); + spin_unlock_irq(&modlist_lock); + return -EBUSY; + } list_add(&mod->list, &modules); spin_unlock_irq(&modlist_lock); - /* Drop lock so they can recurse */ - up(&module_mutex); - /* Start the module */ ret = mod->init(); + + spin_lock_irq(&modlist_lock); if (ret < 0) { /* Init routine failed: abort. Try to protect us from buggy refcounters. */ mod->state = MODULE_STATE_GOING; + spin_unlock_irq(&modlist_lock); synchronize_kernel(); if (mod->unsafe) printk(KERN_ERR "%s: module is now stuck!\n", mod->name); - else { - down(&module_mutex); + else free_module(mod); - up(&module_mutex); - } return ret; } /* Now it's a first class citizen! */ - down(&module_mutex); mod->state = MODULE_STATE_LIVE; module_free(mod, mod->module_init); mod->module_init = NULL; mod->init_size = 0; - up(&module_mutex); + spin_unlock_irq(&modlist_lock); return 0; } @@ -1471,7 +1474,7 @@ static void *m_start(struct seq_file *m, struct list_head *i; loff_t n = 0; - down(&module_mutex); + spin_lock_irq(&modlist_lock); list_for_each(i, &modules) { if (n++ == *pos) break; @@ -1492,7 +1495,7 @@ static void *m_next(struct seq_file *m, static void m_stop(struct seq_file *m, void *p) { - up(&module_mutex); + spin_unlock_irq(&modlist_lock); } static int m_show(struct seq_file *m, void *p)