Revert "module: drop the lock while waiting for module to complete initialization."

This reverts commit 480b02df3aa9f07d1c7df0cd8be7a5ca73893455, since Rafael reports that it causes occasional kernel paging request faults in load_module(). Dropping the module lock and re-taking it deep in the call-chain is definitely not the right thing to do. That just turns the mutex from a lock into a "random non-locking data structure" that doesn't actually protect what it's supposed to protect. Requested-and-tested-by: Rafael J. Wysocki <rjw@sisk.pl> Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: Brandon Philips <brandon@ifup.org> Cc: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Revert "module: drop the lock while waiting for module to complete initialization."
This reverts commit 480b02df3aa9f07d1c7df0cd8be7a5ca73893455, since Rafael reports that it causes occasional kernel paging request faults in load_module(). Dropping the module lock and re-taking it deep in the call-chain is definitely not the right thing to do. That just turns the mutex from a lock into a "random non-locking data structure" that doesn't actually protect what it's supposed to protect. Requested-and-tested-by: Rafael J. Wysocki <rjw@sisk.pl> Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: Brandon Philips <brandon@ifup.org> Cc: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Linus Torvalds
1 parent ec96e2fe95
Showing 1 changed file with 22 additions and 37 deletions Side-by-side Diff
kernel/module.c
@@ -563,26 +563,33 @@
 	struct module_use *use;
 	int no_warn, err;
  
-	if (b == NULL || already_uses(a, b))
-		return 0;
+	if (b == NULL || already_uses(a, b)) return 1;
  
 	/* If we're interrupted or time out, we fail. */
-	err = strong_try_module_get(b);
+	if (wait_event_interruptible_timeout(
+		    module_wq, (err = strong_try_module_get(b)) != -EBUSY,
+		    30 * HZ) <= 0) {
+		printk("%s: gave up waiting for init of module %s.\n",
+		       a->name, b->name);
+		return 0;
+	}
+
+	/* If strong_try_module_get() returned a different error, we fail. */
 	if (err)
-		return err;
+		return 0;
  
 	DEBUGP("Allocating new usage for %s.\n", a->name);
 	use = kmalloc(sizeof(*use), GFP_ATOMIC);
 	if (!use) {
 		printk("%s: out of memory loading\n", a->name);
 		module_put(b);
-		return -ENOMEM;
+		return 0;
 	}
  
 	use->module_which_uses = a;
 	list_add(&use->list, &b->modules_which_use_me);
 	no_warn = sysfs_create_link(b->holders_dir, &a->mkobj.kobj, a->name);
-	return 0;
+	return 1;
 }
 EXPORT_SYMBOL_GPL(use_module);
  
@@ -875,7 +882,7 @@
  
 int use_module(struct module *a, struct module *b)
 {
-	return strong_try_module_get(b);
+	return strong_try_module_get(b) == 0;
 }
 EXPORT_SYMBOL_GPL(use_module);
  
  
  
  
@@ -1046,39 +1053,17 @@
 	struct module *owner;
 	const struct kernel_symbol *sym;
 	const unsigned long *crc;
-	DEFINE_WAIT(wait);
-	int err;
-	long timeleft = 30 * HZ;
  
-again:
 	sym = find_symbol(name, &owner, &crc,
 			  !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);
-	if (!sym)
-		return NULL;
-
-	if (!check_version(sechdrs, versindex, name, mod, crc, owner))
-		return NULL;
-
-	prepare_to_wait(&module_wq, &wait, TASK_INTERRUPTIBLE);
-	err = use_module(mod, owner);
-	if (likely(!err) || err != -EBUSY || signal_pending(current)) {
-		finish_wait(&module_wq, &wait);
-		return err ? NULL : sym;
+	/* use_module can fail due to OOM,
+	   or module initialization or unloading */
+	if (sym) {
+		if (!check_version(sechdrs, versindex, name, mod, crc, owner)
+		    || !use_module(mod, owner))
+			sym = NULL;
 	}
-
-	/* Module is still loading.  Drop lock and wait. */
-	mutex_unlock(&module_mutex);
-	timeleft = schedule_timeout(timeleft);
-	mutex_lock(&module_mutex);
-	finish_wait(&module_wq, &wait);
-
-	/* Module might be gone entirely, or replaced.  Re-lookup. */
-	if (timeleft)
-		goto again;
-
-	printk(KERN_WARNING "%s: gave up waiting for init of module %s.\n",
-	       mod->name, owner->name);
-	return NULL;
+	return sym;
 }
  
 /*
...	...	@@ -563,26 +563,33 @@
563	563	struct module_use *use;
564	564	int no_warn, err;
565	565
566		- if (b == NULL \|\| already_uses(a, b))
567		- return 0;
	566	+ if (b == NULL \|\| already_uses(a, b)) return 1;
568	567
569	568	/* If we're interrupted or time out, we fail. */
570		- err = strong_try_module_get(b);
	569	+ if (wait_event_interruptible_timeout(
	570	+ module_wq, (err = strong_try_module_get(b)) != -EBUSY,
	571	+ 30 * HZ) <= 0) {
	572	+ printk("%s: gave up waiting for init of module %s.\n",
	573	+ a->name, b->name);
	574	+ return 0;
	575	+ }
	576	+
	577	+ /* If strong_try_module_get() returned a different error, we fail. */
571	578	if (err)
572		- return err;
	579	+ return 0;
573	580
574	581	DEBUGP("Allocating new usage for %s.\n", a->name);
575	582	use = kmalloc(sizeof(*use), GFP_ATOMIC);
576	583	if (!use) {
577	584	printk("%s: out of memory loading\n", a->name);
578	585	module_put(b);
579		- return -ENOMEM;
	586	+ return 0;
580	587	}
581	588
582	589	use->module_which_uses = a;
583	590	list_add(&use->list, &b->modules_which_use_me);
584	591	no_warn = sysfs_create_link(b->holders_dir, &a->mkobj.kobj, a->name);
585		- return 0;
	592	+ return 1;
586	593	}
587	594	EXPORT_SYMBOL_GPL(use_module);
588	595
...	...	@@ -875,7 +882,7 @@
875	882
876	883	int use_module(struct module a, struct module b)
877	884	{
878		- return strong_try_module_get(b);
	885	+ return strong_try_module_get(b) == 0;
879	886	}
880	887	EXPORT_SYMBOL_GPL(use_module);
881	888
882	889
883	890
884	891
...	...	@@ -1046,39 +1053,17 @@
1046	1053	struct module *owner;
1047	1054	const struct kernel_symbol *sym;
1048	1055	const unsigned long *crc;
1049		- DEFINE_WAIT(wait);
1050		- int err;
1051		- long timeleft = 30 * HZ;
1052	1056
1053		-again:
1054	1057	sym = find_symbol(name, &owner, &crc,
1055	1058	!(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);
1056		- if (!sym)
1057		- return NULL;
1058		-
1059		- if (!check_version(sechdrs, versindex, name, mod, crc, owner))
1060		- return NULL;
1061		-
1062		- prepare_to_wait(&module_wq, &wait, TASK_INTERRUPTIBLE);
1063		- err = use_module(mod, owner);
1064		- if (likely(!err) \|\| err != -EBUSY \|\| signal_pending(current)) {
1065		- finish_wait(&module_wq, &wait);
1066		- return err ? NULL : sym;
	1059	+ /* use_module can fail due to OOM,
	1060	+ or module initialization or unloading */
	1061	+ if (sym) {
	1062	+ if (!check_version(sechdrs, versindex, name, mod, crc, owner)
	1063	+ \|\| !use_module(mod, owner))
	1064	+ sym = NULL;
1067	1065	}
1068		-
1069		- /* Module is still loading. Drop lock and wait. */
1070		- mutex_unlock(&module_mutex);
1071		- timeleft = schedule_timeout(timeleft);
1072		- mutex_lock(&module_mutex);
1073		- finish_wait(&module_wq, &wait);
1074		-
1075		- /* Module might be gone entirely, or replaced. Re-lookup. */
1076		- if (timeleft)
1077		- goto again;
1078		-
1079		- printk(KERN_WARNING "%s: gave up waiting for init of module %s.\n",
1080		- mod->name, owner->name);
1081		- return NULL;
	1066	+ return sym;
1082	1067	}
1083	1068
1084	1069	/*