[PATCH] dm: wait until kobject is destroyed

[PATCH] dm: wait until kobject is destroyed

[Mikulas Patocka]

Hi

Here I'm sending the updated kobject patch.

Changes:
	The file was renamed to dm-builtin.c
	A comment with explanation of the race condition added.

Mikulas



From: Mikulas Patocka <mpatocka@redhat.com>

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@redhat.com>
Cc: stable@vger.kernel.org

---
 drivers/md/Kconfig      |    4 ++++
 drivers/md/Makefile     |    1 +
 drivers/md/dm-builtin.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/dm-sysfs.c   |    5 ++++-
 drivers/md/dm.c         |    9 +++++----
 drivers/md/dm.h         |   15 +++++++++++++++
 6 files changed, 77 insertions(+), 5 deletions(-)

Index: linux-3.13-rc7/drivers/md/dm-sysfs.c
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-sysfs.c	2014-01-09 20:50:18.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-sysfs.c	2014-01-09 20:50:38.000000000 +0100
@@ -86,6 +86,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 +105,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-09 20:50:18.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm.c	2014-01-09 20:50:38.000000000 +0100
@@ -200,8 +200,8 @@ struct mapped_device {
 	/* forced geometry settings */
 	struct hd_geometry geometry;
 
-	/* sysfs handle */
-	struct kobject kobj;
+	/* kobject and completion */
+	struct dm_kobject_holder h;
 
 	/* zero-length flush that will be cloned and submitted to targets */
 	struct bio flush_bio;
@@ -2049,6 +2049,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->h.completion);
 
 	md->disk->major = _major;
 	md->disk->first_minor = minor;
@@ -2910,7 +2911,7 @@ struct gendisk *dm_disk(struct mapped_de
 
 struct kobject *dm_kobject(struct mapped_device *md)
 {
-	return &md->kobj;
+	return &md->h.kobj;
 }
 
 /*
@@ -2921,7 +2922,7 @@ struct mapped_device *dm_get_from_kobjec
 {
 	struct mapped_device *md;
 
-	md = container_of(kobj, struct mapped_device, kobj);
+	md = container_of(kobj, struct mapped_device, h.kobj);
 
 	if (test_bit(DMF_FREEING, &md->flags) ||
 	    dm_deleting_md(md))
Index: linux-3.13-rc7/drivers/md/dm.h
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm.h	2014-01-09 20:50:18.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm.h	2014-01-09 20:50:38.000000000 +0100
@@ -15,6 +15,8 @@
 #include <linux/list.h>
 #include <linux/blkdev.h>
 #include <linux/hdreg.h>
+#include <linux/completion.h>
+#include <linux/kobject.h>
 
 #include "dm-stats.h"
 
@@ -148,12 +150,25 @@ void dm_interface_exit(void);
 /*
  * sysfs interface
  */
+struct dm_kobject_holder {
+	struct kobject kobj;
+	struct completion completion;
+};
+static inline struct completion *dm_get_completion_from_kobject(struct kobject *kobj)
+{
+	return &container_of(kobj, struct dm_kobject_holder, kobj)->completion;
+}
 int dm_sysfs_init(struct mapped_device *md);
 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);
 
 /*
+ * The kobject helper
+ */
+void dm_kobject_release(struct kobject *kobj);
+
+/*
  * Targets for linear and striped mappings
  */
 int dm_linear_init(void);
Index: linux-3.13-rc7/drivers/md/Kconfig
===================================================================
--- linux-3.13-rc7.orig/drivers/md/Kconfig	2014-01-09 20:50:18.000000000 +0100
+++ linux-3.13-rc7/drivers/md/Kconfig	2014-01-09 22:49:28.000000000 +0100
@@ -176,8 +176,12 @@ config MD_FAULTY
 
 source "drivers/md/bcache/Kconfig"
 
+config BLK_DEV_DM_BUILTIN
+	boolean
+
 config BLK_DEV_DM
 	tristate "Device mapper support"
+	select BLK_DEV_DM_BUILTIN
 	---help---
 	  Device-mapper is a low level volume manager.  It works by allowing
 	  people to specify mappings for ranges of logical sectors.  Various
Index: linux-3.13-rc7/drivers/md/Makefile
===================================================================
--- linux-3.13-rc7.orig/drivers/md/Makefile	2014-01-09 20:50:18.000000000 +0100
+++ linux-3.13-rc7/drivers/md/Makefile	2014-01-09 22:49:37.000000000 +0100
@@ -32,6 +32,7 @@ obj-$(CONFIG_MD_FAULTY)		+= faulty.o
 obj-$(CONFIG_BCACHE)		+= bcache/
 obj-$(CONFIG_BLK_DEV_MD)	+= md-mod.o
 obj-$(CONFIG_BLK_DEV_DM)	+= dm-mod.o
+obj-$(CONFIG_BLK_DEV_DM_BUILTIN) += dm-builtin.o
 obj-$(CONFIG_DM_BUFIO)		+= dm-bufio.o
 obj-$(CONFIG_DM_BIO_PRISON)	+= dm-bio-prison.o
 obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
Index: linux-3.13-rc7/drivers/md/dm-builtin.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-3.13-rc7/drivers/md/dm-builtin.c	2014-01-09 22:56:17.000000000 +0100
@@ -0,0 +1,48 @@
+#include "dm.h"
+
+/*
+ * The kobject release method must not be placed in the module itself,
+ * otherwise we are subject to module unload races.
+ *
+ * The release method is called when the last reference to the kobject is
+ * dropped. It may be called by any other kernel code that drops the last
+ * reference.
+ *
+ * The release method suffers from module unload race. We may prevent the
+ * module from being unloaded at the start of the release method (using
+ * increased module reference count or synchronizing against the release
+ * method), however there is no way to prevent the module from being
+ * unloaded at the end of the release method.
+ *
+ * If this code were placed in the dm module, the following race may
+ * happen:
+ *  1. Some other process takes a reference to dm kobject
+ *  2. The user issues ioctl function to unload the dm device
+ *  3. dm_sysfs_exit calls kobject_put, however the object is not released
+ *     because of the other reference taken at step 1
+ *  4. dm_sysfs_exit waits on the completion
+ *  5. The other process that took the reference in step 1 drops it,
+ *     dm_kobject_release is called from this process
+ *  6. dm_kobject_release calls complete()
+ *  7. a reschedule happens before dm_kobject_release returns
+ *  8. dm_sysfs_exit continues, the dm device is unloaded, module reference
+ *     count is decremented
+ *  9. The user unloads the dm module
+ * 10. The other process that was rescheduled in step 7 continues to run,
+ *     it is now executing code in unloaded module, so it crashes
+ *
+ * Note that if the process that takes the foreign reference to dm kobject
+ * has a low priority and the system is sufficiently loaded with
+ * higher-priority processes that prevent the low-priority process from
+ * being scheduled long enough, this bug may really happen.
+ *
+ * In order to fix this module unload race, we place the release method
+ * into a helper code that is compiled directly into the kernel.
+ */
+
+void dm_kobject_release(struct kobject *kobj)
+{
+	complete(dm_get_completion_from_kobject(kobj));
+}
+
+EXPORT_SYMBOL(dm_kobject_release);

Re: dm: wait until kobject is destroyed

[Mike Snitzer]

On Thu, Jan 09 2014 at  8:53pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Hi
> 
> Here I'm sending the updated kobject patch.
> 
> Changes:
> 	The file was renamed to dm-builtin.c
> 	A comment with explanation of the race condition added.
> 
> Mikulas
> 
> 
> 
> From: Mikulas Patocka <mpatocka@redhat.com>
> 
> 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@redhat.com>
> Cc: stable@vger.kernel.org

This header could use some work considering the previous patch is
already staged in linux-dm.git's 'for-next' -- so this patch will build
on that.  Would be nice to see some commentary about introducing a
dm-builtin to avoid concerns about dm_kobject_release crash after dm-mod
is unloaded.  Also, touch on the role/need of struct dm_kobject_holder?

> Index: linux-3.13-rc7/drivers/md/dm-sysfs.c
> ===================================================================
> --- linux-3.13-rc7.orig/drivers/md/dm-sysfs.c	2014-01-09 20:50:18.000000000 +0100
> +++ linux-3.13-rc7/drivers/md/dm-sysfs.c	2014-01-09 20:50:38.000000000 +0100
> @@ -86,6 +86,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 +105,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-09 20:50:18.000000000 +0100
> +++ linux-3.13-rc7/drivers/md/dm.c	2014-01-09 20:50:38.000000000 +0100
> @@ -200,8 +200,8 @@ struct mapped_device {
>  	/* forced geometry settings */
>  	struct hd_geometry geometry;
>  
> -	/* sysfs handle */
> -	struct kobject kobj;
> +	/* kobject and completion */
> +	struct dm_kobject_holder h;
>  
>  	/* zero-length flush that will be cloned and submitted to targets */
>  	struct bio flush_bio;

Could you rename 'h' to 'kobj_holder'?

Also, this patch needs to be rebased ontop of linux-dm.git's 'for-next'
branch.

Mike

[PATCH v2] Re: dm: wait until kobject is destroyed

[Mikulas Patocka]

On Fri, 10 Jan 2014, Mike Snitzer wrote:

> On Thu, Jan 09 2014 at  8:53pm -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > Hi
> > 
> > Here I'm sending the updated kobject patch.
> > 
> > Changes:
> > 	The file was renamed to dm-builtin.c
> > 	A comment with explanation of the race condition added.
> > 
> > Mikulas
> > 
> > 
> > 
> > From: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > 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@redhat.com>
> > Cc: stable@vger.kernel.org
> 
> This header could use some work considering the previous patch is
> already staged in linux-dm.git's 'for-next' -- so this patch will build
> on that.  Would be nice to see some commentary about introducing a
> dm-builtin to avoid concerns about dm_kobject_release crash after dm-mod
> is unloaded.  Also, touch on the role/need of struct dm_kobject_holder?
> 
> Could you rename 'h' to 'kobj_holder'?
> 
> Also, this patch needs to be rebased ontop of linux-dm.git's 'for-next'
> branch.
> 
> Mike

Here I'm sending the updated patch. (it reverts the patch that is already 
in git and applies the new patch). The patch is untested, because I don't 
use that git branch, but the same code was tested on Linus' 3.13-rc7.

Mikulas



From: Mikulas Patocka <mpatocka@redhat.com>

dm-sysfs: fix a module unload race

The code that calls the completion must be placed in non-module file,
otherwise there is a module unload race (if the process is preempted and
module unloaded after the completion is triggered, but before the function
returns).

To fix this race, this patch moves the completion code to dm-builtin.c
that is always compiled directly to the kernel.

The patch introduces a new structure struct dm_kobject_holder, its purpose
is to keep the completion and the kobject at one place, so that it can be
accessed from non-module code without the need to export the layout of
struct mapped_device to that code.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/Kconfig      |    4 ++++
 drivers/md/Makefile     |    1 +
 drivers/md/dm-builtin.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/dm-sysfs.c   |    5 -----
 drivers/md/dm.c         |   20 +++++---------------
 drivers/md/dm.h         |   15 ++++++++++++++-
 6 files changed, 72 insertions(+), 21 deletions(-)

Index: linux-dm/drivers/md/dm-sysfs.c
===================================================================
--- linux-dm.orig/drivers/md/dm-sysfs.c	2014-01-14 01:24:49.000000000 +0100
+++ linux-dm/drivers/md/dm-sysfs.c	2014-01-14 01:25:13.000000000 +0100
@@ -79,11 +79,6 @@ 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
Index: linux-dm/drivers/md/dm.c
===================================================================
--- linux-dm.orig/drivers/md/dm.c	2014-01-14 01:24:49.000000000 +0100
+++ linux-dm/drivers/md/dm.c	2014-01-14 01:25:13.000000000 +0100
@@ -200,11 +200,8 @@ struct mapped_device {
 	/* forced geometry settings */
 	struct hd_geometry geometry;
 
-	/* sysfs handle */
-	struct kobject kobj;
-
-	/* wait until the kobject is released */
-	struct completion kobj_completion;
+	/* kobject and completion */
+	struct dm_kobject_holder kobj_holder;
 
 	/* zero-length flush that will be cloned and submitted to targets */
 	struct bio flush_bio;
@@ -2044,7 +2041,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);
+	init_completion(&md->kobj_holder.completion);
 
 	md->disk->major = _major;
 	md->disk->first_minor = minor;
@@ -2906,14 +2903,14 @@ struct gendisk *dm_disk(struct mapped_de
 
 struct kobject *dm_kobject(struct mapped_device *md)
 {
-	return &md->kobj;
+	return &md->kobj_holder.kobj;
 }
 
 struct mapped_device *dm_get_from_kobject(struct kobject *kobj)
 {
 	struct mapped_device *md;
 
-	md = container_of(kobj, struct mapped_device, kobj);
+	md = container_of(kobj, struct mapped_device, kobj_holder.kobj);
 
 	if (test_bit(DMF_FREEING, &md->flags) ||
 	    dm_deleting_md(md))
@@ -2923,13 +2920,6 @@ 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-dm/drivers/md/dm.h
===================================================================
--- linux-dm.orig/drivers/md/dm.h	2014-01-14 01:24:49.000000000 +0100
+++ linux-dm/drivers/md/dm.h	2014-01-14 01:25:13.000000000 +0100
@@ -16,6 +16,7 @@
 #include <linux/blkdev.h>
 #include <linux/hdreg.h>
 #include <linux/completion.h>
+#include <linux/kobject.h>
 
 #include "dm-stats.h"
 
@@ -149,11 +150,23 @@ void dm_interface_exit(void);
 /*
  * sysfs interface
  */
+struct dm_kobject_holder {
+	struct kobject kobj;
+	struct completion completion;
+};
+static inline struct completion *dm_get_completion_from_kobject(struct kobject *kobj)
+{
+	return &container_of(kobj, struct dm_kobject_holder, kobj)->completion;
+}
 int dm_sysfs_init(struct mapped_device *md);
 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);
+
+/*
+ * The kobject helper
+ */
+void dm_kobject_release(struct kobject *kobj);
 
 /*
  * Targets for linear and striped mappings
Index: linux-dm/drivers/md/Kconfig
===================================================================
--- linux-dm.orig/drivers/md/Kconfig	2014-01-14 01:24:48.000000000 +0100
+++ linux-dm/drivers/md/Kconfig	2014-01-14 01:25:13.000000000 +0100
@@ -176,8 +176,12 @@ config MD_FAULTY
 
 source "drivers/md/bcache/Kconfig"
 
+config BLK_DEV_DM_BUILTIN
+	boolean
+
 config BLK_DEV_DM
 	tristate "Device mapper support"
+	select BLK_DEV_DM_BUILTIN
 	---help---
 	  Device-mapper is a low level volume manager.  It works by allowing
 	  people to specify mappings for ranges of logical sectors.  Various
Index: linux-dm/drivers/md/Makefile
===================================================================
--- linux-dm.orig/drivers/md/Makefile	2014-01-14 01:24:48.000000000 +0100
+++ linux-dm/drivers/md/Makefile	2014-01-14 01:25:13.000000000 +0100
@@ -32,6 +32,7 @@ obj-$(CONFIG_MD_FAULTY)		+= faulty.o
 obj-$(CONFIG_BCACHE)		+= bcache/
 obj-$(CONFIG_BLK_DEV_MD)	+= md-mod.o
 obj-$(CONFIG_BLK_DEV_DM)	+= dm-mod.o
+obj-$(CONFIG_BLK_DEV_DM_BUILTIN) += dm-builtin.o
 obj-$(CONFIG_DM_BUFIO)		+= dm-bufio.o
 obj-$(CONFIG_DM_BIO_PRISON)	+= dm-bio-prison.o
 obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
Index: linux-dm/drivers/md/dm-builtin.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-dm/drivers/md/dm-builtin.c	2014-01-14 01:25:13.000000000 +0100
@@ -0,0 +1,48 @@
+#include "dm.h"
+
+/*
+ * The kobject release method must not be placed in the module itself,
+ * otherwise we are subject to module unload races.
+ *
+ * The release method is called when the last reference to the kobject is
+ * dropped. It may be called by any other kernel code that drops the last
+ * reference.
+ *
+ * The release method suffers from module unload race. We may prevent the
+ * module from being unloaded at the start of the release method (using
+ * increased module reference count or synchronizing against the release
+ * method), however there is no way to prevent the module from being
+ * unloaded at the end of the release method.
+ *
+ * If this code were placed in the dm module, the following race may
+ * happen:
+ *  1. Some other process takes a reference to dm kobject
+ *  2. The user issues ioctl function to unload the dm device
+ *  3. dm_sysfs_exit calls kobject_put, however the object is not released
+ *     because of the other reference taken at step 1
+ *  4. dm_sysfs_exit waits on the completion
+ *  5. The other process that took the reference in step 1 drops it,
+ *     dm_kobject_release is called from this process
+ *  6. dm_kobject_release calls complete()
+ *  7. a reschedule happens before dm_kobject_release returns
+ *  8. dm_sysfs_exit continues, the dm device is unloaded, module reference
+ *     count is decremented
+ *  9. The user unloads the dm module
+ * 10. The other process that was rescheduled in step 7 continues to run,
+ *     it is now executing code in unloaded module, so it crashes
+ *
+ * Note that if the process that takes the foreign reference to dm kobject
+ * has a low priority and the system is sufficiently loaded with
+ * higher-priority processes that prevent the low-priority process from
+ * being scheduled long enough, this bug may really happen.
+ *
+ * In order to fix this module unload race, we place the release method
+ * into a helper code that is compiled directly into the kernel.
+ */
+
+void dm_kobject_release(struct kobject *kobj)
+{
+	complete(dm_get_completion_from_kobject(kobj));
+}
+
+EXPORT_SYMBOL(dm_kobject_release);

Re: [PATCH v2] Re: dm: wait until kobject is destroyed

[Mike Snitzer]

On Mon, Jan 13 2014 at  7:37pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:
 
> Here I'm sending the updated patch. (it reverts the patch that is already 
> in git and applies the new patch). The patch is untested, because I don't 
> use that git branch, but the same code was tested on Linus' 3.13-rc7.
> 
> Mikulas
> 
> 
> 
> From: Mikulas Patocka <mpatocka@redhat.com>
> 
> dm-sysfs: fix a module unload race
> 
> The code that calls the completion must be placed in non-module file,
> otherwise there is a module unload race (if the process is preempted and
> module unloaded after the completion is triggered, but before the function
> returns).
> 
> To fix this race, this patch moves the completion code to dm-builtin.c
> that is always compiled directly to the kernel.
> 
> The patch introduces a new structure struct dm_kobject_holder, its purpose
> is to keep the completion and the kobject at one place, so that it can be
> accessed from non-module code without the need to export the layout of
> struct mapped_device to that code.

Thanks for this v2 patch and updated header, I'll get it queued up.