- Lock down the ``module'' structure by adding an SX lock that is used by

all the global bits of ``module'' data.  This commit adds a few generic
  macros, MOD_SLOCK, MOD_XLOCK, etc., that are meant to be used as ways
  of accessing the SX lock.  It is also the first step in helping to lock
  down the kernel linker and module systems.

Reviewed by: jhb, jake, smp@

sys/kern/kern_linker.c

@@ -37,6 +37,7 @@
 #include <sys/proc.h>
 #include <sys/lock.h>
 #include <sys/mutex.h>
+#include <sys/sx.h>
 #include <sys/module.h>
 #include <sys/linker.h>
 #include <sys/fcntl.h>
@@ -45,7 +46,6 @@
 #include <sys/vnode.h>
 #include <sys/sysctl.h>
 
-
 #include "linker_if.h"
 
 #ifdef KLD_DEBUG
@@ -274,11 +274,6 @@ linker_file_register_modules(linker_file_t lf)
 		moddata = (*mdp)->md_data;
 		KLD_DPF(FILE, ("Registering module %s in %s\n",
 		    moddata->name, lf->filename));
-		if (module_lookupbyname(moddata->name) != NULL) {
-			printf("Warning: module %s already exists\n",
-			    moddata->name);
-			continue;	/* or return a error ? */
-		}
 		error = module_register(moddata, lf);
 		if (error)
 			printf("Module %s failed to register: %d\n",
@@ -447,8 +442,10 @@ linker_file_unload(linker_file_t file)
 		/*
 		 * Inform any modules associated with this file.
 		 */
+		MOD_XLOCK;
 		for (mod = TAILQ_FIRST(&file->modules); mod; mod = next) {
 			next = module_getfnext(mod);
+			MOD_XUNLOCK;
 
 			/*
 			 * Give the module a chance to veto the unload.
@@ -458,9 +455,11 @@ linker_file_unload(linker_file_t file)
 				    " vetoes unload\n", mod));
 				lockmgr(&lock, LK_RELEASE, 0, curthread);
 				goto out;
-			}
+			} else
+				MOD_XLOCK;
 			module_release(mod);
 		}
+		MOD_XUNLOCK;
 	}
 	file->refs--;
 	if (file->refs > 0) {
@@ -900,11 +899,13 @@ kldfirstmod(struct thread *td, struct kldfirstmod_args *uap)
 	mtx_lock(&Giant);
 	lf = linker_find_file_by_id(SCARG(uap, fileid));
 	if (lf) {
+		MOD_SLOCK;
 		mp = TAILQ_FIRST(&lf->modules);
 		if (mp != NULL)
 			td->td_retval[0] = module_getid(mp);
 		else
 			td->td_retval[0] = 0;
+		MOD_SUNLOCK;
 	} else
 		error = ENOENT;
 	mtx_unlock(&Giant);

sys/kern/kern_module.c

@@ -33,11 +33,12 @@
 #include <sys/malloc.h>
 #include <sys/sysproto.h>
 #include <sys/sysent.h>
-#include <sys/module.h>
-#include <sys/linker.h>
 #include <sys/proc.h>
 #include <sys/lock.h>
 #include <sys/mutex.h>
+#include <sys/sx.h>
+#include <sys/module.h>
+#include <sys/linker.h>
 
 static MALLOC_DEFINE(M_MODULE, "module", "module data structures");
 
@@ -57,6 +58,7 @@ struct module {
 #define MOD_EVENT(mod, type)	(mod)->handler((mod), (type), (mod)->arg)
 
 static modulelist_t modules;
+struct sx modules_sx;
 static int nextid = 1;
 static void module_shutdown(void *, int);
 
@@ -66,11 +68,11 @@ modevent_nop(module_t mod, int what, void *arg)
 	return (0);
 }
 
-
 static void
 module_init(void *arg)
 {
 
+	sx_init(&modules_sx, "module subsystem sx lock");
 	TAILQ_INIT(&modules);
 	EVENTHANDLER_REGISTER(shutdown_post_sync, module_shutdown, NULL,
 	    SHUTDOWN_PRI_DEFAULT);
@@ -83,8 +85,10 @@ module_shutdown(void *arg1, int arg2)
 {
 	module_t mod;
 
+	MOD_SLOCK;
 	TAILQ_FOREACH(mod, &modules, link)
 		MOD_EVENT(mod, MOD_SHUTDOWN);
+	MOD_SUNLOCK;
 }
 
 void
@@ -94,14 +98,18 @@ module_register_init(const void *arg)
 	int error;
 	module_t mod;
 
+	MOD_SLOCK;
 	mod = module_lookupbyname(data->name);
 	if (mod == NULL)
 		panic("module_register_init: module named %s not found\n",
 		    data->name);
+	MOD_SUNLOCK;
 	error = MOD_EVENT(mod, MOD_LOAD);
 	if (error) {
 		MOD_EVENT(mod, MOD_UNLOAD);
+		MOD_XLOCK;
 		module_release(mod);
+		MOD_XUNLOCK;
 		printf("module_register_init: MOD_LOAD (%s, %p, %p) error"
 		    " %d\n", data->name, (void *)data->evhand, data->priv,
 		    error); 
@@ -114,16 +122,20 @@ module_register(const moduledata_t *data, linker_file_t container)
 	size_t namelen;
 	module_t newmod;
 
+	MOD_SLOCK;
 	newmod = module_lookupbyname(data->name);
 	if (newmod != NULL) {
+		MOD_SUNLOCK;
 		printf("module_register: module %s already exists!\n",
 		    data->name);
 		return (EEXIST);
 	}
+	MOD_SUNLOCK;
 	namelen = strlen(data->name) + 1;
 	newmod = malloc(sizeof(struct module) + namelen, M_MODULE, M_WAITOK);
 	if (newmod == NULL)
 		return (ENOMEM);
+	MOD_XLOCK;
 	newmod->refs = 1;
 	newmod->id = nextid++;
 	newmod->name = (char *)(newmod + 1);
@@ -136,6 +148,7 @@ module_register(const moduledata_t *data, linker_file_t container)
 	if (container)
 		TAILQ_INSERT_TAIL(&container->modules, newmod, flink);
 	newmod->file = container;
+	MOD_XUNLOCK;
 	return (0);
 }
 
@@ -143,6 +156,8 @@ void
 module_reference(module_t mod)
 {
 
+	MOD_XLOCK_ASSERT;
+
 	MOD_DPF(REFS, ("module_reference: before, refs=%d\n", mod->refs));
 	mod->refs++;
 }
@@ -151,17 +166,21 @@ void
 module_release(module_t mod)
 {
 
+	MOD_XLOCK_ASSERT;
+
 	if (mod->refs <= 0)
 		panic("module_release: bad reference count");
 
 	MOD_DPF(REFS, ("module_release: before, refs=%d\n", mod->refs));
-
+	
 	mod->refs--;
 	if (mod->refs == 0) {
 		TAILQ_REMOVE(&modules, mod, link);
 		if (mod->file)
 			TAILQ_REMOVE(&mod->file->modules, mod, flink);
+		MOD_XUNLOCK;
 		free(mod, M_MODULE);
+		MOD_XLOCK;
 	}
 }
 
@@ -171,6 +190,8 @@ module_lookupbyname(const char *name)
 	module_t mod;
 	int err;
 
+	MOD_LOCK_ASSERT;
+
 	TAILQ_FOREACH(mod, &modules, link) {
 		err = strcmp(mod->name, name);
 		if (err == 0)
@@ -182,26 +203,28 @@ module_lookupbyname(const char *name)
 module_t
 module_lookupbyid(int modid)
 {
-	module_t mod;
+        module_t mod;
 
-	TAILQ_FOREACH(mod, &modules, link) {
+        MOD_LOCK_ASSERT;
-		if (mod->id == modid)
+
-			return (mod);
+        TAILQ_FOREACH(mod, &modules, link)
-	}
+                if (mod->id == modid)
-	return (NULL);
+                        return(mod);
+        return (NULL);
 }
 
 int
 module_unload(module_t mod)
 {
 
-	return (MOD_EVENT(mod, MOD_UNLOAD));
+        return (MOD_EVENT(mod, MOD_UNLOAD));
 }
 
 int
 module_getid(module_t mod)
 {
 
+	MOD_LOCK_ASSERT;
 	return (mod->id);
 }
 
@@ -209,6 +232,7 @@ module_t
 module_getfnext(module_t mod)
 {
 
+	MOD_LOCK_ASSERT;
 	return (TAILQ_NEXT(mod, flink));
 }
 
@@ -216,6 +240,7 @@ void
 module_setspecific(module_t mod, modspecific_t *datap)
 {
 
+	MOD_XLOCK_ASSERT;
 	mod->data = *datap;
 }
 
@@ -234,6 +259,7 @@ modnext(struct thread *td, struct modnext_args *uap)
 	mtx_lock(&Giant);
 
 	td->td_retval[0] = -1;
+	MOD_SLOCK;
 	if (SCARG(uap, modid) == 0) {
 		mod = TAILQ_FIRST(&modules);
 		if (mod)
@@ -252,6 +278,7 @@ modnext(struct thread *td, struct modnext_args *uap)
 	else
 		td->td_retval[0] = 0;
 done2:
+	MOD_SUNLOCK;
 	mtx_unlock(&Giant);
 	return (error);
 }
@@ -269,6 +296,7 @@ modfnext(struct thread *td, struct modfnext_args *uap)
 
 	mtx_lock(&Giant);
 
+	MOD_SLOCK;
 	mod = module_lookupbyid(SCARG(uap, modid));
 	if (mod == NULL) {
 		error = ENOENT;
@@ -279,6 +307,7 @@ modfnext(struct thread *td, struct modfnext_args *uap)
 		else
 			td->td_retval[0] = 0;
 	}
+	MOD_SUNLOCK;
 	mtx_unlock(&Giant);
 	return (error);
 }
@@ -297,18 +326,26 @@ int
 modstat(struct thread *td, struct modstat_args *uap)
 {
 	module_t mod;
+	modspecific_t data;
 	int error = 0;
-	int namelen;
+	int id, namelen, refs, version;
-	int version;
 	struct module_stat *stat;
+	char *name;
 
 	mtx_lock(&Giant);
 
+	MOD_SLOCK;
 	mod = module_lookupbyid(SCARG(uap, modid));
 	if (mod == NULL) {
+		MOD_SUNLOCK;
 		error = ENOENT;
 		goto out;
 	}
+	id = mod->id;
+	refs = mod->refs;
+	name = mod->name;
+	data = mod->data;
+	MOD_SUNLOCK;
 	stat = SCARG(uap, stat);
 
 	/*
@@ -324,20 +361,20 @@ modstat(struct thread *td, struct modstat_args *uap)
 	namelen = strlen(mod->name) + 1;
 	if (namelen > MAXMODNAME)
 		namelen = MAXMODNAME;
-	if ((error = copyout(mod->name, &stat->name[0], namelen)) != 0)
+	if ((error = copyout(name, &stat->name[0], namelen)) != 0)
 		goto out;
 
-	if ((error = copyout(&mod->refs, &stat->refs, sizeof(int))) != 0)
+	if ((error = copyout(&refs, &stat->refs, sizeof(int))) != 0)
 		goto out;
-	if ((error = copyout(&mod->id, &stat->id, sizeof(int))) != 0)
+	if ((error = copyout(&id, &stat->id, sizeof(int))) != 0)
 		goto out;
 
 	/*
 	 * >v1 stat includes module data.
 	 */
 	if (version == sizeof(struct module_stat)) {
-		if ((error = copyout(&mod->data, &stat->data, 
+		if ((error = copyout(&data, &stat->data, 
-		    sizeof(mod->data))) != 0)
+		    sizeof(data))) != 0)
 			goto out;
 	}
 	td->td_retval[0] = 0;
@@ -360,11 +397,13 @@ modfind(struct thread *td, struct modfind_args *uap)
 		goto out;
 
 	mtx_lock(&Giant);
+	MOD_SLOCK;
 	mod = module_lookupbyname(name);
 	if (mod == NULL)
 		error = ENOENT;
 	else
-		td->td_retval[0] = mod->id;
+		td->td_retval[0] = module_getid(mod);
+	MOD_SUNLOCK;
 	mtx_unlock(&Giant);
 out:
 	return (error);

sys/kern/kern_syscalls.c

@@ -30,6 +30,9 @@
 #include <sys/sysproto.h>
 #include <sys/sysent.h>
 #include <sys/syscall.h>
+#include <sys/lock.h>
+#include <sys/mutex.h>
+#include <sys/sx.h>
 #include <sys/module.h>
 
 /*
@@ -96,7 +99,9 @@ syscall_module_handler(struct module *mod, int what, void *arg)
                if (error)
                        return error;
 	       ms.intval = *data->offset;
+	       MOD_XLOCK;
 	       module_setspecific(mod, &ms);
+	       MOD_XUNLOCK;
                if (data->chainevh)
                        error = data->chainevh(mod, what, data->chainarg);
                return error;

sys/sys/module.h

@@ -125,6 +125,15 @@ struct mod_metadata {
 	MODULE_METADATA(_##module##_version, MDT_VERSION,		\
 	    &_##module##_version, #module)
 
+extern struct sx modules_sx;
+
+#define	MOD_XLOCK	sx_xlock(&modules_sx)
+#define	MOD_SLOCK	sx_slock(&modules_sx)
+#define	MOD_XUNLOCK	sx_xunlock(&modules_sx)
+#define	MOD_SUNLOCK	sx_sunlock(&modules_sx)
+#define	MOD_LOCK_ASSERT	sx_assert(&modules_sx, SX_LOCKED)
+#define	MOD_XLOCK_ASSERT	sx_assert(&modules_sx, SX_XLOCKED)
+
 struct linker_file;
 
 void	module_register_init(const void *);