Skip to contentSkip to conversation

module loader should not complain about unknown symbol

PATCH In review Jan Glauber 5 participants 8 messages Nov 12, 2007 – Nov 14, 2007

lore
Share & tools
lore

Jan Glauber

author
#0 lore raw

PATCH 1/1 module loader should not complain about unknown symbol

If module A depends on module B and module B has not yet finished its init() the module loader may print a warning about an unknown symbol.

This happens if module B is still in state MODULE_STATE_COMING, as module A runs into resolve_symbol() for a symbol from module B. resolve_symbol() return 0 in that case and causes the warning.

---

Signed-off-by Jan Glauber <[email protected]>
kernel/module.c +2 -2
Index: linux-2.6/kernel/module.c
===================================================================
@@ -1411,8 +1411,8 @@
 			if (ELF_ST_BIND(sym[i].st_info) == STB_WEAK)
 				break;
 
-			printk(KERN_WARNING "%s: Unknown symbol %s\n",
-			       mod->name, strtab + sym[i].st_name);
+			DEBUGP("%s: Unknown symbol %s\n", mod->name,
+			       strtab + sym[i].st_name);
 			ret = -ENOENT;
 			break;
#1 lore raw
Quoted 6 lines · Jan Glauber

If module A depends on module B and module B has not yet finished its init() the module loader may print a warning about an unknown symbol.

This happens if module B is still in state MODULE_STATE_COMING, as module A runs into resolve_symbol() for a symbol from module B. resolve_symbol() return 0 in that case and causes the warning.

I don't think that patch is a good idea. It points to a kernel problem of some sort that needs to be resolved. Making it fail silently is not an improvement.

Also how else should the user know about "legitimate" (not caused by races) unresolved symbols?

-Andi

#2 lore raw
In reply to Jan Glauber
Quoted 6 lines · On Tuesday 13 November 2007 01:18:27 Jan Glauber wrote:

If module A depends on module B and module B has not yet finished its init() the module loader may print a warning about an unknown symbol.

This happens if module B is still in state MODULE_STATE_COMING, as module A runs into resolve_symbol() for a symbol from module B. resolve_symbol() return 0 in that case and causes the warning.

It also happens when a symbol is undefined, so this patch is a bad idea.

To do this properly you would change strong_try_module_get to return -EBUSY and -ENOENT, then change use_module(), then finally resolve_symbol(), then only print a warning when it returns something other than -EBUSY.

Better might be to put in a waitqueue and wake it up whenever a module is deleted or changes status. Then use_module() can wait if strong_try_module_get() returns -EBUSY (up to 30 seconds, then print a warning and fail).

Hope that helps,
Rusty.

#3 lore raw
Quoted 4 lines · On Tuesday 13 November 2007 09:23:12 Rusty Russell wrote:

Better might be to put in a waitqueue and wake it up whenever a module is deleted or changes status. Then use_module() can wait if strong_try_module_get() returns -EBUSY (up to 30 seconds, then print a warning and fail).

And here it is. Does it work for you Jan?

==
modules: wait for dependent modules doing init.

There have been reports of modules failing to load because the modules they depend on are still loading. This changes the modules to wait for a reasonable length of time in that case. We time out eventually, because there can be module loops or broken modules.

Signed-off-by Rusty Russell <[email protected]>
kernel/module.c +23 -5
diff -r a38c2f5e95f8 kernel/module.c
@@ -67,6 +67,9 @@ static DEFINE_MUTEX(module_mutex);
 static DEFINE_MUTEX(module_mutex);
 static LIST_HEAD(modules);
 
+/* Waiting for a module to finish initializing? */
+static DECLARE_WAIT_QUEUE_HEAD(module_wq);
+
 static BLOCKING_NOTIFIER_HEAD(module_notify_list);
 
 int register_module_notifier(struct notifier_block * nb)

@@ -86,8 +89,11 @@ static inline int strong_try_module_get(
 static inline int strong_try_module_get(struct module *mod)
 {
 	if (mod && mod->state == MODULE_STATE_COMING)
+		return -EBUSY;
+	if (try_module_get(mod))
 		return 0;
-	return try_module_get(mod);
+	else
+		return -ENOENT;
 }
 
 static inline void add_taint_module(struct module *mod, unsigned flag)

@@ -539,11 +545,21 @@ static int use_module(struct module *a, 
 static int use_module(struct module *a, struct module *b)
 {
 	struct module_use *use;
-	int no_warn;
+	int no_warn, err;
 
 	if (b == NULL || already_uses(a, b)) return 1;
 
-	if (!strong_try_module_get(b))
+	/* If we're interrupted or time out, we fail. */
+	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 0;
 
 	DEBUGP("Allocating new usage for %s.\n", a->name);

@@ -814,7 +830,7 @@ static inline void module_unload_free(st
 
 static inline int use_module(struct module *a, struct module *b)
 {
-	return strong_try_module_get(b);
+	return strong_try_module_get(b) == 0;
 }
 
 static inline void module_unload_init(struct module *mod)

@@ -1330,7 +1346,7 @@ void *__symbol_get(const char *symbol)
 
 	preempt_disable();
 	value = __find_symbol(symbol, &owner, &crc, 1);
-	if (value && !strong_try_module_get(owner))
+	if (value && strong_try_module_get(owner) != 0)
 		value = 0;
 	preempt_enable();
 

@@ -2133,6 +2149,7 @@ sys_init_module(void __user *umod,
 		mutex_lock(&module_mutex);
 		free_module(mod);
 		mutex_unlock(&module_mutex);
+		wake_up(&module_wq);
 		return ret;
 	}
 

@@ -2147,6 +2164,7 @@ sys_init_module(void __user *umod,
 	mod->init_size = 0;
 	mod->init_text_size = 0;
 	mutex_unlock(&module_mutex);
+	wake_up(&module_wq);
 
 	return 0;
 }
#4 lore raw
Quoted 6 lines · Rusty Russell
Quoted 4 lines · On Tuesday 13 November 2007 09:23:12 Rusty Russell wrote:

Better might be to put in a waitqueue and wake it up whenever a module is deleted or changes status. Then use_module() can wait if strong_try_module_get() returns -EBUSY (up to 30 seconds, then print a warning and fail).

Will it ever go into a deadlock (or likewise DOS scenario) if modA dep on modB, but modB also dep on modA?

#5 lore raw
Quoted 9 lines · On Tuesday 13 November 2007 15:08:35 Peter Teoh wrote:
Quoted 5 lines · Rusty Russell
Quoted 4 lines · On Tuesday 13 November 2007 09:23:12 Rusty Russell wrote:

Better might be to put in a waitqueue and wake it up whenever a module is deleted or changes status. Then use_module() can wait if strong_try_module_get() returns -EBUSY (up to 30 seconds, then print a warning and fail).

Will it ever go into a deadlock (or likewise DOS scenario) if modA dep on modB, but modB also dep on modA?

No, only if modB requests modA in its init function. And then only for 30 seconds.

modprobe will sort out the A needs B needs A case IIRC.

Cheers,
Rusty.

#6 lore raw

Rusty,

I read your previous email and thought I'd play with it...and then by the time I got around to having some time tonight you'd already done it. You're a machine :-) We actually have some other stuff failing at work due to this exact bug so I will test against RHEL5 tomorrow also.

Jon.

author
#7 lore raw
In reply to Rusty Russell
Quoted 7 lines · Rusty Russell
Quoted 4 lines · On Tuesday 13 November 2007 09:23:12 Rusty Russell wrote:

Better might be to put in a waitqueue and wake it up whenever a module is deleted or changes status. Then use_module() can wait if strong_try_module_get() returns -EBUSY (up to 30 seconds, then print a warning and fail).

And here it is. Does it work for you Jan?

No, it hangs at boot time after starting udev. All CPUs are in cpu_idle(). Unfortunately I've had no time for further debugging and I'll be offline for the next 10 days...

-jang