Re: kobject: provide kobject_put_wait to fix module unload race

[Mikulas Patocka]

On Mon, 6 Jan 2014, Mike Snitzer wrote:

> On Mon, Jan 06 2014 at  1:55pm -0500,
> Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
> 
> > 
> > 
> > On Sun, 5 Jan 2014, Greg Kroah-Hartman wrote:
> > 
> > > On Sun, Jan 05, 2014 at 05:43:56PM +0100, Bart Van Assche wrote:
> > > > On 01/04/14 19:06, Mikulas Patocka wrote:
> > > > > -	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);
> > > > > -
> > > > 
> > > > Has it been considered to issue a warning if no release function has
> > > > been defined and free_completion == NULL instead of removing the above
> > > > debug message entirely ? I think even with this patch applied it is
> > > > still wrong to invoke kobject_put() on an object without defining a
> > > > release function.
> > > 
> > > This patch isn't going to be applied, and I've reverted the original
> > > commit, so there shouldn't be any issues anymore with this code.
> > 
> > Why? This patch does the same thing as 
> > eee031649707db3c9920d9498f8d03819b74fc23, but it's smaller. So why did you 
> > accept eee031649707db3c9920d9498f8d03819b74fc23 and not this?
> > 
> > The code to wait for kobject destruction using completion already exists 
> > in cpufreq_sysfs_release, cpuidle_sysfs_release, 
> > cpuidle_state_sysfs_release, cpuidle_driver_sysfs_release, 
> > ext4_sb_release, ext4_feat_release, f2fs_sb_release (these are the only 
> > kobject users that are correct w.r.t. module unloading), so if you accept 
> > this patch, you can simplify them to use kobject_put_wait.
> 
> Hi Mikulas,
> 
> Please just submit a DM-only patch that follows the same racey pattern
> of firing a completion from the kobj_type .release method in dm_mod.
> I'll get it queued up for 3.14.
> 
> If/when we gets reports of a crash due to dm_mod unload racing with
> kobject_put we can revisit this.
> 
> Thanks,
> Mike

Here I'm sending dm-only patch.



dm: wait until kobject is destroyed

There may be other parts of the kernel taking reference to the dm kobject.
We must wait until they drop the references before deallocating the md
structure.

Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx

---
 drivers/md/dm-sysfs.c |   10 +++++++++-
 drivers/md/dm.c       |   11 +++++++++++
 drivers/md/dm.h       |    2 ++
 3 files changed, 22 insertions(+), 1 deletion(-)

Index: linux-3.13-rc7/drivers/md/dm-sysfs.c
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-sysfs.c	2014-01-07 02:06:08.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-sysfs.c	2014-01-07 02:07:09.000000000 +0100
@@ -79,6 +79,11 @@ static const struct sysfs_ops dm_sysfs_o
 	.show	= dm_attr_show,
 };
 
+static void dm_kobject_release(struct kobject *kobj)
+{
+	complete(dm_get_completion_from_kobject(kobj));
+}
+
 /*
  * dm kobject is embedded in mapped_device structure
  * no need to define release function here
@@ -86,6 +91,7 @@ static const struct sysfs_ops dm_sysfs_o
 static struct kobj_type dm_ktype = {
 	.sysfs_ops	= &dm_sysfs_ops,
 	.default_attrs	= dm_attrs,
+	.release	= dm_kobject_release,
 };
 
 /*
@@ -104,5 +110,7 @@ int dm_sysfs_init(struct mapped_device *
  */
 void dm_sysfs_exit(struct mapped_device *md)
 {
-	kobject_put(dm_kobject(md));
+	struct kobject *kobj = dm_kobject(md);
+	kobject_put(kobj);
+	wait_for_completion(dm_get_completion_from_kobject(kobj));
 }
Index: linux-3.13-rc7/drivers/md/dm.c
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm.c	2014-01-07 02:07:09.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm.c	2014-01-07 04:58:37.000000000 +0100
@@ -203,6 +203,9 @@ struct mapped_device {
 	/* sysfs handle */
 	struct kobject kobj;
 
+	/* wait until the kobject is released */
+	struct completion kobj_completion;
+
 	/* zero-length flush that will be cloned and submitted to targets */
 	struct bio flush_bio;
 
@@ -2049,6 +2052,7 @@ static struct mapped_device *alloc_dev(i
 	init_waitqueue_head(&md->wait);
 	INIT_WORK(&md->work, dm_wq_work);
 	init_waitqueue_head(&md->eventq);
+	init_completion(&md->kobj_completion);
 
 	md->disk->major = _major;
 	md->disk->first_minor = minor;
@@ -2931,6 +2935,13 @@ struct mapped_device *dm_get_from_kobjec
 	return md;
 }
 
+struct completion *dm_get_completion_from_kobject(struct kobject *kobj)
+{
+	struct mapped_device *md = container_of(kobj, struct mapped_device, kobj);
+
+	return &md->kobj_completion;
+}
+
 int dm_suspended_md(struct mapped_device *md)
 {
 	return test_bit(DMF_SUSPENDED, &md->flags);
Index: linux-3.13-rc7/drivers/md/dm.h
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm.h	2014-01-07 02:06:08.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm.h	2014-01-07 02:07:09.000000000 +0100
@@ -15,6 +15,7 @@
 #include <linux/list.h>
 #include <linux/blkdev.h>
 #include <linux/hdreg.h>
+#include <linux/completion.h>
 
 #include "dm-stats.h"
 
@@ -152,6 +153,7 @@ int dm_sysfs_init(struct mapped_device *
 void dm_sysfs_exit(struct mapped_device *md);
 struct kobject *dm_kobject(struct mapped_device *md);
 struct mapped_device *dm_get_from_kobject(struct kobject *kobj);
+struct completion *dm_get_completion_from_kobject(struct kobject *kobj);
 
 /*
  * Targets for linear and striped mappings
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/