[PATCH v2] NVMe: eliminate potential deadlock by nvme_get_ns_from_disk invoking nvme_free_ns

[PATCH v2] NVMe: eliminate potential deadlock by nvme_get_ns_from_disk invoking nvme_free_ns

[Wang Sheng-Hui]

Release dev_list_lock before enter nvme_free_ns from
nvme_get_ns_from_disk to 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(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 643f457..ab12892 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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)
-- 
2.7.4

[PATCH v2] NVMe: eliminate potential deadlock by nvme_get_ns_from_disk invoking nvme_free_ns

[Christoph Hellwig]

On Wed, May 04, 2016@09:08:34AM +0800, Wang Sheng-Hui wrote:
> Release dev_list_lock before enter nvme_free_ns from
> nvme_get_ns_from_disk to 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(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 643f457..ab12892 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -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))
> +			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] NVMe: eliminate potential deadlock by nvme_get_ns_from_disk invoking nvme_free_ns

[Busch, Keith]

I can't be bothered to fix up my dev machines this week, so I get to reply with outlook. :(


> -----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 by nvme_get_ns_from_disk invoking
> nvme_free_ns

<snip>

> 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;

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;
--