| Message ID | 1365506529-8396-1-git-send-email-vfalico@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Apr 09, 2013 at 01:22:09PM +0200, Veaceslav Falico wrote: > 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). 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(). As Rusty points out, this isn't a kobject issue that can be solved with a new api call, but rather, the user of the kobject code needs to be fixed, with something like his proposed patch instead. So, because of this, I can't take this patch, sorry. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Greg KH <gregkh@linuxfoundation.org> writes: > On Tue, Apr 09, 2013 at 01:22:09PM +0200, Veaceslav Falico wrote: >> 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). 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(). > > As Rusty points out, this isn't a kobject issue that can be solved with > a new api call, but rather, the user of the kobject code needs to be > fixed, with something like his proposed patch instead. > > So, because of this, I can't take this patch, sorry. Greg, I'm shocked! Surely you've been doing this long enough to know that we don't use that kind of language on lkml? To restore the list's reputation as a hostile pressure cooker powered by the smouldering remains of flame-roasted newcomers, allow me to correct your reply: "NAK. And you smell." Crisis averted, Rusty. PS. Thanks :) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 11, 2013 at 11:28:06AM +0930, Rusty Russell wrote: >Greg KH <gregkh@linuxfoundation.org> writes: > >> On Tue, Apr 09, 2013 at 01:22:09PM +0200, Veaceslav Falico wrote: >>> 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). 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(). >> >> As Rusty points out, this isn't a kobject issue that can be solved with >> a new api call, but rather, the user of the kobject code needs to be >> fixed, with something like his proposed patch instead. >> >> So, because of this, I can't take this patch, sorry. > >Greg, I'm shocked! Surely you've been doing this long enough to know >that we don't use that kind of language on lkml? > >To restore the list's reputation as a hostile pressure cooker powered by >the smouldering remains of flame-roasted newcomers, allow me to correct >your reply: > > "NAK. And you smell." Got it :). Thank you for your input and your patch, I'll come back with a proper fix when I'll have one. Thanks all. > >Crisis averted, >Rusty. >PS. Thanks :) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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). 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(). 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@redhat.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(-)