module: fix bne2 "gave up waiting for init of module libcrc32c"
in [Kernel]
|
Prev: [regression] Please add bug 15788 and 15969, usb sound / radeon kms broken after snapshot cycle
Next: [PATCH] staging: comedi - correct parameter gainlkup for DAQCard-6024E in driver ni_mio_cs.c
From: Linus Torvalds on 2 Jun 2010 00:40 On Wed, 2 Jun 2010, Rusty Russell wrote: > > > I would like to note that your original "fix things by dropping the lock" > > patch that I hated so much had this exact bug too. Making this a good > > example of _why_ it's basically always wrong to drop locks in the middle - > > even if you claimed you knew and understood the locking. > > And I would like to note that it didn't :) It grabbed references only on > completed modules. Right you are, I misread the patch. You re-did the whole module lookup, something that the original code didn't do. And yes, that should be safe. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 2 Jun 2010 00:50 On Tue, 1 Jun 2010, Brandon Philips wrote: > > FWIW, I tried this same idea initially and it breaks because the > kobject EEXIST is coming from mod_sysfs_init() which happens further > up in load_module() before the list_add_rcu(). Ahh, right. So we'd need to mvoe that down too. As Rusty says, the kobject EEXIST warning should be fairly harmless - albeit annoying. Do things actually _work_ with that thing apart from the kobject warning? > I also tried the obvious variation of moving the list_add_rcu() up > to where the find_module is but got: Yeah, I don't think moving it up will work, because then the module symbol resolver will see itself before having set up the symbols. So I think we need to expose it on the modules list only after having done the "simplify_symbols()" etc. I dunno. Does this work? (Caveat emptor - I tried to make sure the error cases nest correctly, and it all compiles, but it's untested. As usual. Rusty mentioned a "see testing patch", but I didn't see it. Maybe he did the same thing) Linus --- kernel/module.c | 35 +++++++++++++++++++---------------- 1 files changed, 19 insertions(+), 16 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index a1f46a5..247c0aa 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2198,11 +2198,6 @@ static noinline struct module *load_module(void __user *umod, goto free_mod; } - if (find_module(mod->name)) { - err = -EEXIST; - goto free_mod; - } - mod->state = MODULE_STATE_COMING; /* Allow arches to frob section contents and sizes. */ @@ -2293,11 +2288,6 @@ static noinline struct module *load_module(void __user *umod, /* Now we've moved module, initialize linked lists, etc. */ module_unload_init(mod); - /* add kobject, so we can reference it. */ - err = mod_sysfs_init(mod); - if (err) - goto free_unload; - /* Set up license info based on the info section */ set_license(mod, get_modinfo(sechdrs, infoindex, "license")); @@ -2486,16 +2476,28 @@ static noinline struct module *load_module(void __user *umod, * The mutex protects against concurrent writers. */ mutex_lock(&module_mutex); + + if (find_module(mod->name)) { + err = -EEXIST; + /* This will also unlock the mutex */ + goto already_exists; + } + list_add_rcu(&mod->list, &modules); mutex_unlock(&module_mutex); + /* add kobject, so we can reference it. */ + err = mod_sysfs_init(mod); + if (err) + goto unlink; + err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL); if (err < 0) - goto unlink; + goto sysfs_uninit; err = mod_sysfs_setup(mod, mod->kp, mod->num_kp); if (err < 0) - goto unlink; + goto sysfs_uninit; add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs); add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs); @@ -2507,18 +2509,19 @@ static noinline struct module *load_module(void __user *umod, /* Done! */ return mod; + sysfs_uninit: + free_modinfo(mod); + kobject_del(&mod->mkobj.kobj); + kobject_put(&mod->mkobj.kobj); unlink: mutex_lock(&module_mutex); /* Unlink carefully: kallsyms could be walking list. */ list_del_rcu(&mod->list); + already_exists: mutex_unlock(&module_mutex); synchronize_sched(); module_arch_cleanup(mod); cleanup: - free_modinfo(mod); - kobject_del(&mod->mkobj.kobj); - kobject_put(&mod->mkobj.kobj); - free_unload: module_unload_free(mod); #if defined(CONFIG_MODULE_UNLOAD) free_percpu(mod->refptr); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 2 Jun 2010 01:00 On Tue, 1 Jun 2010, Linus Torvalds wrote: > > (Caveat emptor - I tried to make sure the error cases nest correctly, and > it all compiles, but it's untested. As usual. In fact, I didn't nest it right. The "free_modinfo()" pairs with the "setup_modinfo()" call, and should go into the "cleanup" error case, not the "sysfs_uninit" error case. IOW, I moved one too many error case cleanup lines. So in that patch, the "free_modinfo()" call should move back to the cleanup case. Like the appended (still untested - I just stared at the code some more, rather than do anything as mundane as _test_ it) patch. It may still not be right, of course. But it might be closer. (That function _really_ should be peeled like an onion, and split into many smaller functions, so that we don't have ten error cases needing unwinding. I like "goto error", but at some point you can't see the unwinding any more, and that function has passed that point a long time ago, I think) Linus --- kernel/module.c | 33 ++++++++++++++++++--------------- 1 files changed, 18 insertions(+), 15 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index a1f46a5..135577c 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2198,11 +2198,6 @@ static noinline struct module *load_module(void __user *umod, goto free_mod; } - if (find_module(mod->name)) { - err = -EEXIST; - goto free_mod; - } - mod->state = MODULE_STATE_COMING; /* Allow arches to frob section contents and sizes. */ @@ -2293,11 +2288,6 @@ static noinline struct module *load_module(void __user *umod, /* Now we've moved module, initialize linked lists, etc. */ module_unload_init(mod); - /* add kobject, so we can reference it. */ - err = mod_sysfs_init(mod); - if (err) - goto free_unload; - /* Set up license info based on the info section */ set_license(mod, get_modinfo(sechdrs, infoindex, "license")); @@ -2486,16 +2476,28 @@ static noinline struct module *load_module(void __user *umod, * The mutex protects against concurrent writers. */ mutex_lock(&module_mutex); + + if (find_module(mod->name)) { + err = -EEXIST; + /* This will also unlock the mutex */ + goto already_exists; + } + list_add_rcu(&mod->list, &modules); mutex_unlock(&module_mutex); + /* add kobject, so we can reference it. */ + err = mod_sysfs_init(mod); + if (err) + goto unlink; + err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL); if (err < 0) - goto unlink; + goto sysfs_uninit; err = mod_sysfs_setup(mod, mod->kp, mod->num_kp); if (err < 0) - goto unlink; + goto sysfs_uninit; add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs); add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs); @@ -2507,18 +2509,19 @@ static noinline struct module *load_module(void __user *umod, /* Done! */ return mod; + sysfs_uninit: + kobject_del(&mod->mkobj.kobj); + kobject_put(&mod->mkobj.kobj); unlink: mutex_lock(&module_mutex); /* Unlink carefully: kallsyms could be walking list. */ list_del_rcu(&mod->list); + already_exists: mutex_unlock(&module_mutex); synchronize_sched(); module_arch_cleanup(mod); cleanup: free_modinfo(mod); - kobject_del(&mod->mkobj.kobj); - kobject_put(&mod->mkobj.kobj); - free_unload: module_unload_free(mod); #if defined(CONFIG_MODULE_UNLOAD) free_percpu(mod->refptr); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 2 Jun 2010 01:10 On Wed, 2 Jun 2010, Rusty Russell wrote: > > OK, this might be worthwhile. Take the very first mail you sent: > > Hmm. That does seem to be buggy. We can't just drop and re-take the lock: > that may make sense internally as far as resolve_symbol() itself is > concerned, but the caller will its own local variables, and some of those > will no longer be valid if the lock was dropped. > > The implication here is that that I don't know locking, and that this was > done without thought or care. In fact, I did worry about it and decided it > was less risky than a locking reduction given my limited cycles (indeed, > it has been slightly). Well, part of the context here is that the commit had been bisected as being buggy. It turns out the bug was a different issue, but at the same time, it very much looked like the locking was simply known a-priori to be buggy. No? And I do agree with the notion that it might have been a "simpler hack for a quick fix", and potentially less risky (due to being more targeted to the particular problem), when it then causes oopses, the default explanation is that the dubious locking trick really was broken. No? > That commit also changes the return value semantics of "use_module()", > which is an exported interface where the only users seem to be > out-of-kernel (the only in-kernel use is in kernel/module.c itself). That > seems like a really really bad idea too. > > Again with the implication that this was done without consideration. No. The implication was that it's wrong to do and a bad idea. Anything else is just you reading things into it. It's an exported interface, and you changed it with no real reason. Whether you then talked to users or not is immaterial. You could easily have just changed the _internal_ thing, and exported the unchanged interface. Even your second version is just very confused. It renames it, but then exports the renamed version. What does that help? It still means that the external module - for no good reason - needs to have basically a source-level version number check. Why? It's still unclear to me. No reason for that exported interface change seems to exist. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 2 Jun 2010 03:10
On Wed, 2 Jun 2010, Rusty Russell wrote: > > Hmm, you're still missing it. Let me try again. > > You weren't the best person to make the call. That didn't occur to you, did > it? Sure, fair enough. At the same time, I'm the one who has to make the call. And quite often, that call is "this causes more problems than it fixes, we need to revert it". Maybe I was too eager to revert this time. Quite often it ends up the other way, where we end up having broken kernels for too long because people weren't eager _enough_ to revert commits that had been bisected to be troublesome. It's hard to tell beforehand. And quite often, subsystem maintainers are _way_ too eager to not revert the commits they wrote. So I really do end up having to balance that force. Does it always work out? Nope. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |