On 1/4/14, 1:06 PM, Mikulas Patocka wrote: > Hi > > I noticed that Jeff Mahoney added a new structure kobj_completion, defined > in include/linux/kobj_completion.h to the kernel 3.13-rc1 in the patch > eee031649707db3c9920d9498f8d03819b74fc23. In the current upstream kernel, > this interface is still unused. > > However, converting the drivers to use kobj_completion is not trivial > (note that all users of the original kobject interface are buggy - so all > of them need to be converted). > > I came up with a simpler patch to achieve the same purpose - this patch > makes fixing the drivers easy - the driver is fixed just by replacing > "kobject_put" with "kobject_put_wait" in the unload routine. > > I'd like to ask if you could revert > eee031649707db3c9920d9498f8d03819b74fc23 (no code uses it) and replace it > with this patch. I have no objections to reverting it. There were concerns from Al Viro that it'd be tough to get right by callers and I had assumed it got dropped after that. I had planned on using it in my btrfs sysfs exports patchset but came up with a better way. -Jeff > See http://www.redhat.com/archives/dm-devel/2013-October/msg00141.html for > the bug that this patch fixes. > > Mikulas > > > > From: Mikulas Patocka <mpatocka@xxxxxxxxxx> > > This patch introduces a new function kobject_put_wait. It decrements the > kobject reference count, waits until the count reaches zero. When this > function returns, it is guaranteed that the kobject was freed. > > A rationale for this function: > > The kobject is keeps a reference count. The driver unload routine > decrements the reference count, however, references to the kobject may > still be held by other kernel subsystems. The driver must not free the > memory that contains the kobject. Instead, the driver provides a "release" > method. The "release" method is called by the kernel when the last kobject > refernce is dropped. The "release" method should free the memory that > contains the kobject. > > However, this pattern is buggy with respect to modules. The release method > is placed in the driver's module. When the driver exits, the module > reference count is zero, thus the module may be freed. However, there may > still be references to the kobject. If the module is unloaded and then the > release method is called, a crash happens. > > Recently, CONFIG_DEBUG_KOBJECT_RELEASE was added. This option deliberately > provokes this race condition. > > This patch fixes the bug by providing new function kobject_put_wait. > kobject_put_wait works like kobject_put, but it also waits until all other > references were dropped and until the kobject was freed. When > kobject_put_wait returns, it is guaranteed that the kobject was released > with the release method. > > Thus, we can change kobject_put to kobject_put_wait in the unload routine > of various drivers to fix the above race condition. > > This patch fixes it for device mapper. Note that all kobject users in > modules should be fixed to use this function. > > Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx> > Cc: stable@xxxxxxxxxx > > --- > drivers/md/dm-sysfs.c | 2 +- > include/linux/kobject.h | 3 +++ > lib/kobject.c | 34 +++++++++++++++++++++++++++++----- > 3 files changed, 33 insertions(+), 6 deletions(-) > > Index: linux-3.13-rc6/include/linux/kobject.h > =================================================================== > --- linux-3.13-rc6.orig/include/linux/kobject.h 2014-01-02 23:13:24.000000000 +0100 > +++ linux-3.13-rc6/include/linux/kobject.h 2014-01-02 23:14:02.000000000 +0100 > @@ -27,6 +27,7 @@ > #include <linux/wait.h> > #include <linux/atomic.h> > #include <linux/workqueue.h> > +#include <linux/completion.h> > > #define UEVENT_HELPER_PATH_LEN 256 > #define UEVENT_NUM_ENVP 32 /* number of env pointers */ > @@ -66,6 +67,7 @@ struct kobject { > struct kobj_type *ktype; > struct sysfs_dirent *sd; > struct kref kref; > + struct completion *free_completion; > #ifdef CONFIG_DEBUG_KOBJECT_RELEASE > struct delayed_work release; > #endif > @@ -106,6 +108,7 @@ extern int __must_check kobject_move(str > > extern struct kobject *kobject_get(struct kobject *kobj); > extern void kobject_put(struct kobject *kobj); > +extern void kobject_put_wait(struct kobject *kobj); > > extern const void *kobject_namespace(struct kobject *kobj); > extern char *kobject_get_path(struct kobject *kobj, gfp_t flag); > Index: linux-3.13-rc6/lib/kobject.c > =================================================================== > --- linux-3.13-rc6.orig/lib/kobject.c 2014-01-02 23:13:23.000000000 +0100 > +++ linux-3.13-rc6/lib/kobject.c 2014-01-02 23:17:01.000000000 +0100 > @@ -172,6 +172,7 @@ static void kobject_init_internal(struct > if (!kobj) > return; > kref_init(&kobj->kref); > + kobj->free_completion = NULL; > INIT_LIST_HEAD(&kobj->entry); > kobj->state_in_sysfs = 0; > kobj->state_add_uevent_sent = 0; > @@ -577,15 +578,11 @@ static void kobject_cleanup(struct kobje > { > struct kobj_type *t = get_ktype(kobj); > const char *name = kobj->name; > + struct completion *free_completion = kobj->free_completion; > > pr_debug("kobject: '%s' (%p): %s, parent %p\n", > kobject_name(kobj), kobj, __func__, kobj->parent); > > - if (t && !t->release) > - pr_debug("kobject: '%s' (%p): does not have a release() " > - "function, it is broken and must be fixed.\n", > - kobject_name(kobj), kobj); > - > /* send "remove" if the caller did not do it but sent "add" */ > if (kobj->state_add_uevent_sent && !kobj->state_remove_uevent_sent) { > pr_debug("kobject: '%s' (%p): auto cleanup 'remove' event\n", > @@ -611,6 +608,10 @@ static void kobject_cleanup(struct kobje > pr_debug("kobject: '%s': free name\n", name); > kfree(name); > } > + > + /* if someone is waiting for the kobject to be freed, wake him up */ > + if (free_completion) > + complete(free_completion); > } > > #ifdef CONFIG_DEBUG_KOBJECT_RELEASE > @@ -651,6 +652,28 @@ void kobject_put(struct kobject *kobj) > } > } > > +/** > + * kobject_put - decrement refcount for object and wait until it reaches zero. > + * @kobj: object. > + * > + * Decrement the refcount, and wait until the refcount reaches zero and the > + * kobject is freed. > + * > + * This function should be called from the driver unload routine. It must not > + * be called concurrently on the same kobject. When this function returns, it > + * is guaranteed that the kobject was freed. > + */ > +void kobject_put_wait(struct kobject *kobj) > +{ > + if (kobj) { > + DECLARE_COMPLETION_ONSTACK(completion); > + BUG_ON(kobj->free_completion); > + kobj->free_completion = &completion; > + kobject_put(kobj); > + wait_for_completion(&completion); > + } > +} > + > static void dynamic_kobj_release(struct kobject *kobj) > { > pr_debug("kobject: (%p): %s\n", kobj, __func__); > @@ -1076,6 +1099,7 @@ void kobj_ns_drop(enum kobj_ns_type type > > EXPORT_SYMBOL(kobject_get); > EXPORT_SYMBOL(kobject_put); > +EXPORT_SYMBOL(kobject_put_wait); > EXPORT_SYMBOL(kobject_del); > > EXPORT_SYMBOL(kset_register); > Index: linux-3.13-rc6/drivers/md/dm-sysfs.c > =================================================================== > --- linux-3.13-rc6.orig/drivers/md/dm-sysfs.c 2014-01-02 23:13:24.000000000 +0100 > +++ linux-3.13-rc6/drivers/md/dm-sysfs.c 2014-01-02 23:14:02.000000000 +0100 > @@ -104,5 +104,5 @@ int dm_sysfs_init(struct mapped_device * > */ > void dm_sysfs_exit(struct mapped_device *md) > { > - kobject_put(dm_kobject(md)); > + kobject_put_wait(dm_kobject(md)); > } > -- Jeff Mahoney SUSE Labs
Attachment:
signature.asc
Description: OpenPGP digital signature
-- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
