NVMe: eliminate potential deadlock by nvme_get_ns_from_disk invoking nvme_free_ns
PATCH v2 1/1 NVMe: eliminate potential deadlock by nvme_get_ns_from_disk invoking nvme_free_ns
Release dev_list_lock before enter nvme_free_ns from nvme_get_ns_from_disk to avoid potential deadlock.
--- drivers/nvme/host/core.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
drivers/nvme/host/core.c +6 -9
@@ -87,20 +87,17 @@ static struct nvme_ns *nvme_get_ns_from_disk(struct gendisk *disk)spin_lock (&dev_list_lock); ns = disk->private_data;if (ns) { +if (!try_module_get (ns->ctrl->ops->module)) { +spin_unlock (&dev_list_lock); +kref_put (&ns->kref, nvme_free_ns); +return NULL ; + }if (!kref_get_unless_zero (&ns->kref)) - goto fail; -if (!try_module_get (ns->ctrl->ops->module)) - goto fail_put_ns; + ns =NULL ; }spin_unlock (&dev_list_lock);return ns; - -fail_put_ns: -kref_put (&ns->kref, nvme_free_ns); -fail: -spin_unlock (&dev_list_lock); -return NULL ; }void nvme_requeue_req (struct request *req)
Signature · 1 line
2.7.4
Christoph Hellwig
maintainerPATCH v2 1/1 NVMe: eliminate potential deadlock by nvme_get_ns_from_disk invoking nvme_free_ns
Quoted 18 lines · Wang Sheng-Hui
Release
dev_list_lockbefore enternvme_free_nsfromnvme_get_ns_from_diskto avoid potential deadlock.Signed-off-by Wang Sheng-Hui <shhuiw at foxmail.com>--- drivers/nvme/host/core.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)drivers/nvme/host/core.c +2
-0@@ -87,20 +87,17 @@ static struct nvme_ns *nvme_get_ns_from_disk(struct gendisk *disk)spin_lock (&dev_list_lock); ns = disk->private_data;if (ns) { +if (!try_module_get (ns->ctrl->ops->module)) { +spin_unlock (&dev_list_lock);
drivers/nvme/host/core.c static struct nvme_ns *nvme_get_ns_from_disk(struct gendisk *disk) · line 101
Quoted 10 lines
drivers/nvme/host/core.c +4
-0+kref_put (&ns->kref, nvme_free_ns); +return NULL ; + }if (!kref_get_unless_zero (&ns->kref)) + ns =NULL ; }spin_unlock (&dev_list_lock);return ns; }
This is incorrect - if kref_get_unless_zero fails we now fail to to to drop the reference again. What about this variant instead?
ns = disk->private_data;
if (ns) {
if (!kref_get_unless_zero(&ns->kref))
ns = NULL;
else if (!try_module_get(ns->ctrl->ops->module))
goto out_put_ns;
}
spin_unlock(&dev_list_lock);
return ns;out_put_ns:
spin_unlock(&dev_list_lock); kref_put(&ns->kref, nvme_free_ns); return NULL;
PATCH v2 1/1 NVMe: eliminate potential deadlock by nvme_get_ns_from_disk invoking nvme_free_ns
I can't be bothered to fix up my dev machines this week, so I get to reply with outlook. :(
Quoted 7 lines
-----Original Message-----
From: Christoph Hellwig [mailto:hch at lst.de]
Sent: Wednesday, May 04, 2016 3:50 AM
To: Wang Sheng-Hui
Cc: hch at lst.de; sagig at mellanox.com; Busch, Keith; axboe at fb.com; linux-nvme at lists.infradead.org
Subject: Re: [PATCH v2] NVMe: eliminate potential deadlock bynvme_get_ns_from_diskinvokingnvme_free_ns
<snip>
Quoted 12 lines
What about this variant instead?
ns = disk->private_data; if (ns) { if (!kref_get_unless_zero(&ns->kref)) ns = NULL; else if (!try_module_get(ns->ctrl->ops->module)) goto out_put_ns; } spin_unlock(&dev_list_lock); return ns;
Quoted 4 lines
out_put_ns:spin_unlock(&dev_list_lock); kref_put(&ns->kref, nvme_free_ns); return NULL;
I was trying to suggest a smaller change by swapping the module and ns reference order, then unwinding failure is simpler without goto's:
---
spin_lock(&dev_list_lock);
ns = disk->private_data;
if (ns) {
if (!try_module_get(ns->ctrl->ops->module))
ns = NULL;
else if (!kref_get_unless_zero(&ns->kref)) {
module_put(ns->ctrl->ops->module);
ns = NULL;
}
}
spin_unlock(&dev_list_lock);
return ns;--