RFC/Preliminary patch - simpler raceless module unloading

RFC/Preliminary patch - simpler raceless module unloading

[Adam J. Richter]

[-- Attachment #1: Type: text/plain, Size: 2356 bytes --]

	Here is a completely untested patch to illustrate a simpler
way to raceless module unloading.  Code that uses this scheme does
not have to deal with the possibilty of try_module_get failing, and,
if all users of the old mechanism are converted, we will be able
to delete the new most fundamental type of locking primitive code
in kernel/module.c.

	I've kept this code mostly separated in <linux/module_rwsem.h>
and kernel/module_rwsem.c so that it will not necessitate pulling
<linux/module.h> into more include files and it should be more
portable across different module loading schems.

	The basic theory of operation is that the data structure for
looking up a given type of driver by name or number should be
protected by an rw_semaphore (e.g., registering a file system type,
finding a file system type by name).  When a driver is registered, it
call register_module_rwsem to associate to tell the module system that
it needs to acquire a write lock on the given rwsem before attempting
to remove the module (or checking its reference count for that
purpose).  The get_foo_driver_by_name(name) and put_foo_driver(driver)
routines take a read lock on the rw_semaphore (or assume that they are
called with the read lock held) while they look up the driver and
manipulate its module reference count.  register_foo_driver a takes a
write lock on the rw_semaphore (or requires the caller to be holding a
write lock on it).  unregister_foo_driver assumes that the caller
already holds a write lock on the semaphore because sys_delete_module
already acquirs all of the rwsem's that were registered for a given
module by register_module_rwsem.  As a result, no attempt to look
up a driver by name or modify its reference count can overlap
with the part of sys_delete_module that checks the module reference
count through completion of the module's unload routine.

	I plan to port net_device, filesystem, file_operations,
block_device_operations, and device_driver to use it.  So if what
I've described above is too verbose, it should become clearer once
I post some ports of code to it.

-- 
Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

[-- Attachment #2: mod_rwsems.diff --]
[-- Type: text/plain, Size: 6996 bytes --]

--- linux-2.5.52/include/linux/module.h	2002-12-15 18:08:14.000000000 -0800
+++ linux/include/linux/module.h	2002-12-23 04:43:47.000000000 -0800
@@ -157,6 +157,11 @@
 	/* Reference counts */
 	struct module_ref ref[NR_CPUS];
 
+	/* Which rwsems to grab before trying to unload the module.
+	   This list of struct module_rwsem is in order sorted by
+	   module_rwsem.rwsem address. */
+	struct list_head rwsems;
+
 	/* What modules depend on me? */
 	struct list_head modules_which_use_me;
 
--- linux-2.5.52/include/linux/module_rwsem.h	1969-12-31 16:00:00.000000000 -0800
+++ linux/include/linux/module_rwsem.h	2002-12-23 04:44:02.000000000 -0800
@@ -0,0 +1,23 @@
+#ifndef _LINUX_MODULE_RWSEM_H
+#define _LINUX_MODULE_RWSEM_H
+
+/* This is separate from module.h so that most headers will not need
+   to include <linux/module.h>. */
+
+#include <linux/list.h>
+
+struct module;
+struct rw_semaphore;
+
+struct rw_semaphore;
+struct module_rwsem {
+	struct rw_semaphore	*rwsem;
+	struct module		*module;
+	struct list_head	same_module;
+};
+
+extern void register_module_rwsem(struct module_rwsem *);
+extern int module_rwsems_down(struct module *);
+extern void module_rwsems_up(struct module *);
+
+#endif
--- linux-2.5.52/kernel/module.c	2002-12-15 18:08:13.000000000 -0800
+++ linux/kernel/module.c	2002-12-23 04:43:27.000000000 -0800
@@ -17,6 +17,7 @@
 */
 #include <linux/config.h>
 #include <linux/module.h>
+#include <linux/module_rwsem.h>
 #include <linux/moduleloader.h>
 #include <linux/init.h>
 #include <linux/slab.h>
@@ -116,6 +117,7 @@
 {
 	unsigned int i;
 
+	INIT_LIST_HEAD(&mod->rwsems);
 	INIT_LIST_HEAD(&mod->modules_which_use_me);
 	for (i = 0; i < NR_CPUS; i++)
 		atomic_set(&mod->ref[i].count, 0);
@@ -377,9 +379,13 @@
 	mod = find_module(name);
 	if (!mod) {
 		ret = -ENOENT;
-		goto out;
+		goto out_no_rwsems;
 	}
 
+	ret = module_rwsems_down(mod);
+	if (ret)
+		goto out_no_rwsems;
+
 	if (!list_empty(&mod->modules_which_use_me)) {
 		/* Other modules depend on us: get rid of them first. */
 		ret = -EWOULDBLOCK;
@@ -457,6 +463,9 @@
 	free_module(mod);
 
  out:
+	module_rwsems_up(mod);
+
+ out_no_rwsems:
 	up(&module_mutex);
 	return ret;
 }
--- linux-2.5.52/kernel/module_rwsem.c	1969-12-31 16:00:00.000000000 -0800
+++ linux/kernel/module_rwsem.c	2002-12-23 04:43:21.000000000 -0800
@@ -0,0 +1,163 @@
+#include <linux/config.h>
+#include <linux/module_rwsem.h>
+#include <linux/module.h>
+#include <linux/rwsem.h>
+
+#ifdef CONFIG_MODULE_UNLOAD
+
+#define list_add_before(list, new)	list_add_tail(list, new)
+
+/**
+ * register_module_rwsem - Require that sys_delete module take an
+ *			   grab an exclusive lock on an rwsem when trying
+ *			   to unload this module.
+ * @module_rwsem:   Contains module, rw_sempahore and pointers.  This
+ *		    structure should be statically allocated in the module
+ *		    itself, as it is referenced after the module_exit routine
+ *		    ends.
+ *	.module		The module to associate with the rw_semaphore.
+ *	.rwsem		The rw_semaphore to associate with the module.
+ *	.same_module	(Filled in by register_module_rwsem)
+ *
+ * Description:
+ *  Make sys_delete_module take an exclusive lock on the rwsem when trying
+ *  to unload the given module.  The module's remove function is then
+ *  responsible for releasing each rwsem, usually via
+ *  unregister_module_rwsem.
+ *
+ *  You can safely associate an rwsem with the same module multiple times.
+ *  sys_delete_module will grab the rwsem only once, and
+ *  unregister_module_rwsem will only release the rwsem if it is the
+ *  last remaining reference to it in the module's rwsem list.
+ **/
+void register_module_rwsem(struct module_rwsem *mod_rwsem)
+{
+	struct list_head *list_ptr;
+
+	if (mod_rwsem->module == NULL)
+		return;
+
+	BUG_ON(mod_rwsem->rwsem == NULL);
+
+	list_for_each(list_ptr, &mod_rwsem->module->rwsems) {
+		struct module_rwsem *tmp_mod_rwsem =
+			list_entry(list_ptr, struct module_rwsem, same_module);
+		if (tmp_mod_rwsem->rwsem > mod_rwsem->rwsem) {
+			list_add_before(&mod_rwsem->same_module, list_ptr);
+			return;
+		}
+	}
+
+	list_add_tail(&mod_rwsem->same_module, &mod_rwsem->module->rwsems);
+}
+EXPORT_SYMBOL(register_module_rwsem);
+
+/* Kludge until somebody implements down_write_interruptible. */
+static inline int down_write_interruptible(struct rw_semaphore *rwsem)
+{
+	down_write(rwsem);
+	return 0;
+}
+
+static void
+module_rwsems_up_partial(struct module *module, struct rw_semaphore *end)
+{
+	struct rw_semaphore *prev_rwsem = NULL;
+	struct module_rwsem *mod_rwsem;
+
+	list_for_each_entry(mod_rwsem, &module->rwsems, same_module) {
+		struct rw_semaphore *rwsem = mod_rwsem->rwsem;
+		if (rwsem == end)
+			return;
+		if (rwsem != prev_rwsem) {
+			up_write(rwsem);
+			prev_rwsem = rwsem;
+		}
+	}
+}
+
+void module_rwsems_up(struct module *module)
+{
+	module_rwsems_up_partial(module, NULL);
+}
+
+int module_rwsems_down(struct module *module)
+{
+	struct rw_semaphore *prev_rwsem = NULL;
+	struct module_rwsem *mod_rwsem;
+
+	list_for_each_entry(mod_rwsem, &module->rwsems, same_module) {
+		struct rw_semaphore *rwsem = mod_rwsem->rwsem;
+		if (rwsem != prev_rwsem) {
+			int err = down_write_interruptible(rwsem);
+			if (err) {
+				module_rwsems_up_partial(module, rwsem);
+				return err;
+			}
+			prev_rwsem = mod_rwsem->rwsem;
+		}
+	}
+	return 0;
+}
+
+# if 0
+
+static int
+same_rwsem(struct list_head *list_ptr, struct module_rwsem *mod_rwsem)
+{
+	struct module_rwsem *other_mod_rwsem;
+
+	if (list_ptr == &mod_rwsem->module->rwsems)
+		return 0;
+
+	other_mod_rwsem = list_entry(list_ptr,
+				     struct module_rwsem, same_module);
+	return (other_mod_rwsem->rwsem == mod_rwsem->rwsem);
+}
+
+/**
+ * unregister_module_rwsem - Dissociate an rwsem from a module.
+ * @module_rwsem:   The rwsem/module association.
+ *
+ * Description:
+ *  Do not use.  Instead, up_write is called on all module rwsem's
+ *  automatically after the module's module_exit routine returns.
+ *
+ *  This routine exists only in case somebody really needs to remove
+ *  a module/rwsem association before the module's module_exit routine
+ *  completes (for example, because the storage of the struct
+ *  module_rwsem will be deallocated before then or because the
+ *  rwsem must be released before then).  This does not appear to
+ *  even be necessary, but here is an untested implementation in
+ *  case it is needed in the future.
+ **/
+void unregister_module_rwsem(struct module_rwsem *mod_rwsem)
+{
+	int used_again = 
+		same_rwsem(use->same_module.next, use) ||
+		same_rwsem(use->same_module.prev, use);
+
+	list_del(&use->same_module);
+	if (!used_again)
+		up_write(use->rwsem);
+}
+EXPORT_SYMBOL(unregister_module_rwsem);
+
+# endif /* 0 */
+
+#else /* !CONFIG_MODULE_UNLOAD */
+
+void register_module_rwsem(struct module_rwsem *)
+{
+}
+
+int module_rwsems_down(struct module *)
+{
+	return 0;
+}
+
+static inline void module_rwsems_up(struct module *)
+{
+}
+
+#endif /* CONFIG_MODULE_UNLOAD */

Re: RFC/Preliminary patch - simpler raceless module unloading

[Adam J. Richter]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

I wrote:
> 	I plan to port net_device, filesystem, file_operations,
> block_device_operations, and device_driver to use it.  So if what
> I've described above is too verbose, it should become clearer once
> I post some ports of code to it.

	Here is a preliminary port of fs/filesystems.c, although I have
not done the clean-ups it enables.  I am running it now and have verified
that I can at least load and unload a filesystem module and that I'm
still prevented from unloading a filesystem module that is in use.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

[-- Attachment #2: fs.diff --]
[-- Type: text/plain, Size: 4004 bytes --]

--- linux-2.5.52/include/linux/fs.h	2002-12-15 18:07:50.000000000 -0800
+++ linux/include/linux/fs.h	2002-12-23 07:00:02.000000000 -0800
@@ -19,6 +19,7 @@
 #include <linux/stat.h>
 #include <linux/cache.h>
 #include <linux/radix-tree.h>
+#include <linux/module_rwsem.h>
 #include <asm/atomic.h>
 
 struct iovec;
@@ -953,7 +954,8 @@
 	int fs_flags;
 	struct super_block *(*get_sb) (struct file_system_type *, int, char *, void *);
 	void (*kill_sb) (struct super_block *);
-	struct module *owner;
+	struct module *owner;	/* Will be subsumed into mod_rwsem. */
+	struct module_rwsem	mod_rwsem;
 	struct file_system_type * next;
 	struct list_head fs_supers;
 };
--- linux-2.5.52/fs/filesystems.c	2002-12-15 18:08:14.000000000 -0800
+++ linux/fs/filesystems.c	2002-12-23 06:56:40.000000000 -0800
@@ -26,7 +26,7 @@
  */
 
 static struct file_system_type *file_systems;
-static rwlock_t file_systems_lock = RW_LOCK_UNLOCKED;
+static DECLARE_RWSEM(file_systems_rwsem);
 
 /* WARNING: This can be used only if we _already_ own a reference */
 void get_filesystem(struct file_system_type *fs)
@@ -81,13 +81,17 @@
 	if (fs->next)
 		return -EBUSY;
 	INIT_LIST_HEAD(&fs->fs_supers);
-	write_lock(&file_systems_lock);
+	down_write(&file_systems_rwsem);
 	p = find_filesystem(fs->name);
 	if (*p)
 		res = -EBUSY;
-	else
+	else {
 		*p = fs;
-	write_unlock(&file_systems_lock);
+		fs->mod_rwsem.module = fs->owner;
+		fs->mod_rwsem.rwsem = &file_systems_rwsem;
+		register_module_rwsem(&fs->mod_rwsem);
+	}
+	up_write(&file_systems_rwsem);
 	return res;
 }
 
@@ -107,18 +111,18 @@
 {
 	struct file_system_type ** tmp;
 
-	write_lock(&file_systems_lock);
+	/* sys_delete_module has already done a
+	   down_write(&file_systems_rwsem) for us. */
 	tmp = &file_systems;
 	while (*tmp) {
 		if (fs == *tmp) {
 			*tmp = fs->next;
 			fs->next = NULL;
-			write_unlock(&file_systems_lock);
+			up_write(&file_systems_rwsem);
 			return 0;
 		}
 		tmp = &(*tmp)->next;
 	}
-	write_unlock(&file_systems_lock);
 	return -EINVAL;
 }
 
@@ -134,14 +138,14 @@
 		return err;
 
 	err = -EINVAL;
-	read_lock(&file_systems_lock);
+	down_read(&file_systems_rwsem);
 	for (tmp=file_systems, index=0 ; tmp ; tmp=tmp->next, index++) {
 		if (strcmp(tmp->name,name) == 0) {
 			err = index;
 			break;
 		}
 	}
-	read_unlock(&file_systems_lock);
+	up_read(&file_systems_rwsem);
 	putname(name);
 	return err;
 }
@@ -151,11 +155,11 @@
 	struct file_system_type * tmp;
 	int len, res;
 
-	read_lock(&file_systems_lock);
+	down_read(&file_systems_rwsem);
 	for (tmp = file_systems; tmp; tmp = tmp->next, index--)
 		if (index <= 0 && try_inc_mod_count(tmp->owner))
 				break;
-	read_unlock(&file_systems_lock);
+	up_read(&file_systems_rwsem);
 	if (!tmp)
 		return -EINVAL;
 
@@ -171,10 +175,10 @@
 	struct file_system_type * tmp;
 	int index;
 
-	read_lock(&file_systems_lock);
+	down_read(&file_systems_rwsem);
 	for (tmp = file_systems, index = 0 ; tmp ; tmp = tmp->next, index++)
 		;
-	read_unlock(&file_systems_lock);
+	up_read(&file_systems_rwsem);
 	return index;
 }
 
@@ -206,7 +210,7 @@
 	int len = 0;
 	struct file_system_type * tmp;
 
-	read_lock(&file_systems_lock);
+	down_read(&file_systems_rwsem);
 	tmp = file_systems;
 	while (tmp && len < PAGE_SIZE - 80) {
 		len += sprintf(buf+len, "%s\t%s\n",
@@ -214,7 +218,7 @@
 			tmp->name);
 		tmp = tmp->next;
 	}
-	read_unlock(&file_systems_lock);
+	up_read(&file_systems_rwsem);
 	return len;
 }
 
@@ -222,17 +226,17 @@
 {
 	struct file_system_type *fs;
 
-	read_lock(&file_systems_lock);
+	down_read(&file_systems_rwsem);
 	fs = *(find_filesystem(name));
 	if (fs && !try_inc_mod_count(fs->owner))
 		fs = NULL;
-	read_unlock(&file_systems_lock);
+	up_read(&file_systems_rwsem);
 	if (!fs && (request_module(name) == 0)) {
-		read_lock(&file_systems_lock);
+		down_read(&file_systems_rwsem);
 		fs = *(find_filesystem(name));
 		if (fs && !try_inc_mod_count(fs->owner))
 			fs = NULL;
-		read_unlock(&file_systems_lock);
+		up_read(&file_systems_rwsem);
 	}
 	return fs;
 }