lkml.org 
[lkml]   [2014]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] kobject: provide kobject_put_wait to fix module unload race
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@redhat.com>
>
> 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@redhat.com>
> Cc: stable@kernel.org
>
> ---
> 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

[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2014-01-04 20:21    [W:0.118 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site