| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 11 Apr 2013 11:55:37 +0200
From: Veaceslav Falico <vfalico@...hat.com>
To: Rusty Russell <rusty@...tcorp.com.au>
Cc: linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
gregkh@...uxfoundation.org, bhelgaas@...gle.com
Subject: Re: [PATCH] module: add kset_obj_exists() and use it
On Wed, Apr 10, 2013 at 04:47:34PM +0930, Rusty Russell wrote:
>Veaceslav Falico <vfalico@...hat.com> writes:
>> Add a new function, kset_obj_exists(), which is identical to
>> kset_find_obj() but doesn't take a reference to the kobject
>> found and only returns bool if found/not found.
>>
>> The main purpose would be to avoid the possible race scenario,
>> when we could get the reference in between the kref_put() and
>> kobject_release() functions (i.e. kref_put() already ran,
>> refcount became 0, but the kobject_release() function still
>> didn't run, and we try to get via kobject_get() and thus ending
>> up with a released kobject).
>
>What? If refcount is zero, there should be no more references to it.
>That's what 0 means.
>
>If you want 0-and-then-cleanup, you need atomic_dec_and_lock().
Yep, you're right, I was trying to fix consequences instead of fixing the
cause. What I've encountered was:
rmmod:
#5 [ffff88005a073d78] __delay at ffffffff813a940f
#6 [ffff88005a073d80] do_raw_spin_lock at ffffffff813b1d2e
#7 [ffff88005a073db0] _raw_spin_lock at ffffffff816f8e76
#8 [ffff88005a073de0] kobj_kset_leave at ffffffff8139ee5a
#9 [ffff88005a073e00] kobject_del at ffffffff8139eeb2
#10 [ffff88005a073e20] kobject_cleanup at ffffffff8139ef32
#11 [ffff88005a073e50] kobject_release at ffffffff8139f08d
#12 [ffff88005a073e60] kobject_put at ffffffff8139eddc
#13 [ffff88005a073e80] mod_sysfs_teardown at ffffffff810ed9cc
#14 [ffff88005a073eb0] free_module at ffffffff810edf57
#15 [ffff88005a073ed0] sys_delete_module at ffffffff810ee508
#16 [ffff88005a073f80] system_call_fastpath at ffffffff81703819
insmod:
[exception RIP: kobject_get+63]
#7 [ffff88005a0f3db0] kset_find_obj at ffffffff8139f139
#8 [ffff88005a0f3de0] mod_sysfs_init at ffffffff810ed5f3
#9 [ffff88005a0f3e20] mod_sysfs_setup at ffffffff810f1872
#10 [ffff88005a0f3e80] load_module at ffffffff810f2003
#11 [ffff88005a0f3ef0] sys_init_module at ffffffff810f22e4
#12 [ffff88005a0f3f80] system_call_fastpath at ffffffff81703819
So I've thought it was a race in kset_find_obj(), and I've been wrong (I've
replaced the WARN_ON(refcount) with BUG_ON - we'd anyway BUG somewhere
further in kobject_put() ).
However, I think my patch still adds something good, cause now we have 2
cases where we basically do:
k = kset_find_obj();
if (!k)
return;
kobject_put(k);
which adds useless overhead (by using kobject_get()/kobject_put(), and
kobject_release() - which is called from kobject_put()) - where we should
only verify if there exists a kobject with the specified name.
Should I resend it with a properly fixed commit message, or it's really not
needed?
>
>> It can be triggered, for example,
>> by running insmod/rmmod bonding in parallel, which ends up in
>> a race between kset_obj_find() in mod_sysfs_init() and rmmod's
>> mod_sysfs_fini()/kobject_put().
>
>That's a bug. We should be cleaning up sysfs before we unlike the
>removed module from the list.
>
>Because the same thing applies to ddebug info, which is also keyed by
>module name.
>
>Something like this (untested!):
Sorry for the late response - I wanted to test it for a longer time.
Your patch works flawlessly and fixes this race, with just a small
addition, cause otherwise we could BUG() in show_initstate().
Can you apply this patch or should I (re-)send it somehow?
Thank you!
diff --git a/kernel/module.c b/kernel/module.c
index d0afe23..8be6e97 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1063,6 +1063,9 @@ static ssize_t show_initstate(struct module_attribute *mattr,
case MODULE_STATE_GOING:
state = "going";
break;
+ case MODULE_STATE_UNFORMED:
+ state = "unformed";
+ break;
default:
BUG();
}
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 3c2c72d..8d4de82 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -1862,10 +1862,10 @@ static void free_module(struct module *mod)
> {
> trace_module_free(mod);
>
>- /* Delete from various lists */
>- mutex_lock(&module_mutex);
>- stop_machine(__unlink_module, mod, NULL);
>- mutex_unlock(&module_mutex);
>+ /* We leave it in list to prevent duplicate loads while we clean
>+ * up sysfs, ddebug and any other external exposure. */
>+ mod->state = MODULE_STATE_UNFORMED;
>+
> mod_sysfs_teardown(mod);
>
> /* Remove dynamic debug info */
>@@ -1880,6 +1880,11 @@ static void free_module(struct module *mod)
> /* Free any allocated parameters. */
> destroy_params(mod->kp, mod->num_kp);
>
>+ /* Now we can delete it from the lists */
>+ mutex_lock(&module_mutex);
>+ stop_machine(__unlink_module, mod, NULL);
>+ mutex_unlock(&module_mutex);
>+
> /* This may be NULL, but that's OK */
> unset_module_init_ro_nx(mod);
> module_free(mod, mod->module_init);
>
>> It also saves us some CPU time in several places where we don't
>> need the kobject itself and only need to verify if it actually
>> exists, by not taking the kref (and thus we don't need to
>> kobject_put() afterwards).
>>
>> Signed-off-by: Veaceslav Falico <vfalico@...hat.com>
>> ---
>> drivers/pci/slot.c | 5 +----
>> include/linux/kobject.h | 1 +
>> kernel/module.c | 5 +----
>> lib/kobject.c | 28 ++++++++++++++++++++++++++++
>> 4 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
>> index ac6412f..3988d75 100644
>> --- a/drivers/pci/slot.c
>> +++ b/drivers/pci/slot.c
>> @@ -154,11 +154,8 @@ static char *make_slot_name(const char *name)
>> dup = 1;
>>
>> for (;;) {
>> - struct kobject *dup_slot;
>> - dup_slot = kset_find_obj(pci_slots_kset, new_name);
>> - if (!dup_slot)
>> + if (!kset_obj_exists(pci_slots_kset, new_name))
>> break;
>> - kobject_put(dup_slot);
>> if (dup == max) {
>> len++;
>> max *= 10;
>> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
>> index 939b112..7cde72c 100644
>> --- a/include/linux/kobject.h
>> +++ b/include/linux/kobject.h
>> @@ -191,6 +191,7 @@ static inline struct kobj_type *get_ktype(struct kobject *kobj)
>> }
>>
>> extern struct kobject *kset_find_obj(struct kset *, const char *);
>> +extern bool kset_obj_exists(struct kset *, const char *);
>>
>> /* The global /sys/kernel/ kobject for people to chain off of */
>> extern struct kobject *kernel_kobj;
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 0925c9a..1df13a3 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1606,7 +1606,6 @@ static void module_remove_modinfo_attrs(struct module *mod)
>> static int mod_sysfs_init(struct module *mod)
>> {
>> int err;
>> - struct kobject *kobj;
>>
>> if (!module_sysfs_initialized) {
>> printk(KERN_ERR "%s: module sysfs not initialized\n",
>> @@ -1615,10 +1614,8 @@ static int mod_sysfs_init(struct module *mod)
>> goto out;
>> }
>>
>> - kobj = kset_find_obj(module_kset, mod->name);
>> - if (kobj) {
>> + if (kset_obj_exists(module_kset, mod->name)) {
>> printk(KERN_ERR "%s: module is already loaded\n", mod->name);
>> - kobject_put(kobj);
>> err = -EINVAL;
>> goto out;
>> }
>> diff --git a/lib/kobject.c b/lib/kobject.c
>> index e07ee1f..b82633f 100644
>> --- a/lib/kobject.c
>> +++ b/lib/kobject.c
>> @@ -760,6 +760,34 @@ struct kobject *kset_find_obj(struct kset *kset, const char *name)
>> return ret;
>> }
>>
>> +/**
>> + * kset_obj_exists - verify if an object exists in kset
>> + * @kset: kset we're looking in.
>> + * @name: object's name.
>> + *
>> + * Lock kset via @kset->subsys, and iterate over @kset->list,
>> + * looking for a matching kobject. Returns bool, found/not found.
>> + * This function does not take a reference and thus doesn't
>> + * guarantee that the object won't go away any time after.
>> + */
>> +
>> +bool kset_obj_exists(struct kset *kset, const char *name)
>> +{
>> + struct kobject *k;
>> +
>> + spin_lock(&kset->list_lock);
>> +
>> + list_for_each_entry(k, &kset->list, entry) {
>> + if (kobject_name(k) && !strcmp(kobject_name(k), name)) {
>> + spin_unlock(&kset->list_lock);
>> + return true;
>> + }
>> + }
>> +
>> + spin_unlock(&kset->list_lock);
>> + return false;
>> +}
>> +
>> static void kset_release(struct kobject *kobj)
>> {
>> struct kset *kset = container_of(kobj, struct kset, kobj);
>> --
>> 1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists