From patchwork Fri Jan 10 01:53:18 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mikulas Patocka X-Patchwork-Id: 3463981 Return-Path: X-Original-To: patchwork-dm-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id AD795C02DC for ; Fri, 10 Jan 2014 01:57:03 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9D09C2010E for ; Fri, 10 Jan 2014 01:57:02 +0000 (UTC) Received: from mx4-phx2.redhat.com (mx4-phx2.redhat.com [209.132.183.25]) by mail.kernel.org (Postfix) with ESMTP id 574EA2010B for ; Fri, 10 Jan 2014 01:57:01 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx4-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s0A1rKdj023063; Thu, 9 Jan 2014 20:53:21 -0500 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s0A1rJ43029846 for ; Thu, 9 Jan 2014 20:53:19 -0500 Received: from file01.intranet.prod.int.rdu2.redhat.com (file01.intranet.prod.int.rdu2.redhat.com [10.11.5.7]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s0A1rIxQ030172 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 9 Jan 2014 20:53:19 -0500 Received: from file01.intranet.prod.int.rdu2.redhat.com (localhost [127.0.0.1]) by file01.intranet.prod.int.rdu2.redhat.com (8.14.4/8.14.4) with ESMTP id s0A1rIIa031500; Thu, 9 Jan 2014 20:53:18 -0500 Received: from localhost (mpatocka@localhost) by file01.intranet.prod.int.rdu2.redhat.com (8.14.4/8.14.4/Submit) with ESMTP id s0A1rISZ031496; Thu, 9 Jan 2014 20:53:18 -0500 X-Authentication-Warning: file01.intranet.prod.int.rdu2.redhat.com: mpatocka owned process doing -bs Date: Thu, 9 Jan 2014 20:53:18 -0500 (EST) From: Mikulas Patocka X-X-Sender: mpatocka@file01.intranet.prod.int.rdu2.redhat.com To: "Alasdair G. Kergon" , Mike Snitzer , dm-devel@redhat.com Message-ID: User-Agent: Alpine 2.02 (LRH 1266 2009-07-14) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.25 X-loop: dm-devel@redhat.com Subject: [dm-devel] [PATCH] dm: wait until kobject is destroyed X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk Reply-To: device-mapper development List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 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 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(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel 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 #include #include +#include +#include #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);