nbd locking fixups

nbd locking fixups

[Christoph Hellwig]

Hi Josef and Jens,

this series fixed the lock order reversal that is showing up with
nbd lately.  The first patch contains the asynchronous deletion
from Hou which is needed as a baseline.

[PATCH 1/6] nbd: do del_gendisk() asynchronously for NBD_DESTROY_ON_DISCONNECT

[Christoph Hellwig]

From: Hou Tao <houtao1@huawei.com>

Now open_mutex is used to synchronize partition operations (e.g,
blk_drop_partitions() and blkdev_reread_part()), however it makes
nbd driver broken, because nbd may call del_gendisk() in nbd_release()
or nbd_genl_disconnect() if NBD_CFLAG_DESTROY_ON_DISCONNECT is enabled,
and deadlock occurs, as shown below:

// AB-BA dead-lock
nbd_genl_disconnect            blkdev_open
  nbd_disconnect_and_put
                                 lock bd_mutex
  // last ref
  nbd_put
    lock nbd_index_mutex
      del_gendisk
                                   nbd_open
                                     try lock nbd_index_mutex
        try lock bd_mutex

 or

// AA dead-lock
nbd_release
  lock bd_mutex
    nbd_put
      try lock bd_mutex

Instead of fixing block layer (e.g, introduce another lock), fixing
the nbd driver to call del_gendisk() in a kworker when
NBD_DESTROY_ON_DISCONNECT is enabled. When NBD_DESTROY_ON_DISCONNECT
is disabled, nbd device will always be destroy through module removal,
and there is no risky of deadlock.

To ensure the reuse of nbd index succeeds, moving the calling of
idr_remove() after del_gendisk(), so if the reused index is not found
in nbd_index_idr, the old disk must have been deleted. And reusing
the existing destroy_complete mechanism to ensure nbd_genl_connect()
will wait for the completion of del_gendisk().

Also adding a new workqueue for nbd removal, so nbd_cleanup()
can ensure all removals complete before exits.

Reported-by: syzbot+0fe7752e52337864d29b@syzkaller.appspotmail.com
Fixes: c76f48eb5c08 ("block: take bd_mutex around delete_partitions in del_gendisk")
Signed-off-by: Hou Tao <houtao1@huawei.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 70 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 61 insertions(+), 9 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index c38317979f74..9a7c9a425ab0 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -49,6 +49,7 @@
 
 static DEFINE_IDR(nbd_index_idr);
 static DEFINE_MUTEX(nbd_index_mutex);
+static struct workqueue_struct *nbd_del_wq;
 static int nbd_total_devices = 0;
 
 struct nbd_sock {
@@ -113,6 +114,7 @@ struct nbd_device {
 	struct mutex config_lock;
 	struct gendisk *disk;
 	struct workqueue_struct *recv_workq;
+	struct work_struct remove_work;
 
 	struct list_head list;
 	struct task_struct *task_recv;
@@ -233,7 +235,7 @@ static const struct device_attribute backend_attr = {
 	.show = backend_show,
 };
 
-static void nbd_dev_remove(struct nbd_device *nbd)
+static void nbd_del_disk(struct nbd_device *nbd)
 {
 	struct gendisk *disk = nbd->disk;
 
@@ -242,24 +244,60 @@ static void nbd_dev_remove(struct nbd_device *nbd)
 		blk_cleanup_disk(disk);
 		blk_mq_free_tag_set(&nbd->tag_set);
 	}
+}
 
+/*
+ * Place this in the last just before the nbd is freed to
+ * make sure that the disk and the related kobject are also
+ * totally removed to avoid duplicate creation of the same
+ * one.
+ */
+static void nbd_notify_destroy_completion(struct nbd_device *nbd)
+{
+	if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
+	    nbd->destroy_complete)
+		complete(nbd->destroy_complete);
+}
+
+static void nbd_dev_remove_work(struct work_struct *work)
+{
+	struct nbd_device *nbd =
+		container_of(work, struct nbd_device, remove_work);
+
+	nbd_del_disk(nbd);
+
+	mutex_lock(&nbd_index_mutex);
 	/*
-	 * Place this in the last just before the nbd is freed to
-	 * make sure that the disk and the related kobject are also
-	 * totally removed to avoid duplicate creation of the same
-	 * one.
+	 * Remove from idr after del_gendisk() completes,
+	 * so if the same id is reused, the following
+	 * add_disk() will succeed.
 	 */
-	if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) && nbd->destroy_complete)
-		complete(nbd->destroy_complete);
+	idr_remove(&nbd_index_idr, nbd->index);
+
+	nbd_notify_destroy_completion(nbd);
+	mutex_unlock(&nbd_index_mutex);
 
 	kfree(nbd);
 }
 
+static void nbd_dev_remove(struct nbd_device *nbd)
+{
+	/* Call del_gendisk() asynchrounously to prevent deadlock */
+	if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags)) {
+		queue_work(nbd_del_wq, &nbd->remove_work);
+		return;
+	}
+
+	nbd_del_disk(nbd);
+	idr_remove(&nbd_index_idr, nbd->index);
+	nbd_notify_destroy_completion(nbd);
+	kfree(nbd);
+}
+
 static void nbd_put(struct nbd_device *nbd)
 {
 	if (refcount_dec_and_mutex_lock(&nbd->refs,
 					&nbd_index_mutex)) {
-		idr_remove(&nbd_index_idr, nbd->index);
 		nbd_dev_remove(nbd);
 		mutex_unlock(&nbd_index_mutex);
 	}
@@ -1679,6 +1717,7 @@ static int nbd_dev_add(int index)
 	nbd->tag_set.flags = BLK_MQ_F_SHOULD_MERGE |
 		BLK_MQ_F_BLOCKING;
 	nbd->tag_set.driver_data = nbd;
+	INIT_WORK(&nbd->remove_work, nbd_dev_remove_work);
 	nbd->destroy_complete = NULL;
 	nbd->backend = NULL;
 
@@ -2416,7 +2455,14 @@ static int __init nbd_init(void)
 	if (register_blkdev(NBD_MAJOR, "nbd"))
 		return -EIO;
 
+	nbd_del_wq = alloc_workqueue("nbd-del", WQ_UNBOUND, 0);
+	if (!nbd_del_wq) {
+		unregister_blkdev(NBD_MAJOR, "nbd");
+		return -ENOMEM;
+	}
+
 	if (genl_register_family(&nbd_genl_family)) {
+		destroy_workqueue(nbd_del_wq);
 		unregister_blkdev(NBD_MAJOR, "nbd");
 		return -EINVAL;
 	}
@@ -2434,7 +2480,10 @@ static int nbd_exit_cb(int id, void *ptr, void *data)
 	struct list_head *list = (struct list_head *)data;
 	struct nbd_device *nbd = ptr;
 
-	list_add_tail(&nbd->list, list);
+	/* Skip nbd that is being removed asynchronously */
+	if (refcount_read(&nbd->refs))
+		list_add_tail(&nbd->list, list);
+
 	return 0;
 }
 
@@ -2457,6 +2506,9 @@ static void __exit nbd_cleanup(void)
 		nbd_put(nbd);
 	}
 
+	/* Also wait for nbd_dev_remove_work() completes */
+	destroy_workqueue(nbd_del_wq);
+
 	idr_destroy(&nbd_index_idr);
 	genl_unregister_family(&nbd_genl_family);
 	unregister_blkdev(NBD_MAJOR, "nbd");
-- 
2.30.2

[PATCH 2/6] nbd: refactor device removal

[Christoph Hellwig]

Share common code for the synchronous and workqueue based device removal,
and remove the pointless use of refcount_dec_and_mutex_lock.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 9a7c9a425ab0..6caf26b84a5b 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -259,48 +259,37 @@ static void nbd_notify_destroy_completion(struct nbd_device *nbd)
 		complete(nbd->destroy_complete);
 }
 
-static void nbd_dev_remove_work(struct work_struct *work)
+static void nbd_dev_remove(struct nbd_device *nbd)
 {
-	struct nbd_device *nbd =
-		container_of(work, struct nbd_device, remove_work);
-
 	nbd_del_disk(nbd);
 
-	mutex_lock(&nbd_index_mutex);
 	/*
-	 * Remove from idr after del_gendisk() completes,
-	 * so if the same id is reused, the following
-	 * add_disk() will succeed.
+	 * Remove from idr after del_gendisk() completes, so if the same ID is
+	 * reused, the following add_disk() will succeed.
 	 */
+	mutex_lock(&nbd_index_mutex);
 	idr_remove(&nbd_index_idr, nbd->index);
-
 	nbd_notify_destroy_completion(nbd);
 	mutex_unlock(&nbd_index_mutex);
 
 	kfree(nbd);
 }
 
-static void nbd_dev_remove(struct nbd_device *nbd)
+static void nbd_dev_remove_work(struct work_struct *work)
 {
-	/* Call del_gendisk() asynchrounously to prevent deadlock */
-	if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags)) {
-		queue_work(nbd_del_wq, &nbd->remove_work);
-		return;
-	}
-
-	nbd_del_disk(nbd);
-	idr_remove(&nbd_index_idr, nbd->index);
-	nbd_notify_destroy_completion(nbd);
-	kfree(nbd);
+	nbd_dev_remove(container_of(work, struct nbd_device, remove_work));
 }
 
 static void nbd_put(struct nbd_device *nbd)
 {
-	if (refcount_dec_and_mutex_lock(&nbd->refs,
-					&nbd_index_mutex)) {
+	if (!refcount_dec_and_test(&nbd->refs))
+		return;
+
+	/* Call del_gendisk() asynchrounously to prevent deadlock */
+	if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags))
+		queue_work(nbd_del_wq, &nbd->remove_work);
+	else
 		nbd_dev_remove(nbd);
-		mutex_unlock(&nbd_index_mutex);
-	}
 }
 
 static int nbd_disconnected(struct nbd_config *config)
-- 
2.30.2

[PATCH 3/6] nbd: remove nbd_del_disk

[Christoph Hellwig]

Fold nbd_del_disk and remove the pointless NULL check on ->disk given
that it is always set for a successfully allocated nbd_device structure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 6caf26b84a5b..de8b23af2486 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -235,17 +235,6 @@ static const struct device_attribute backend_attr = {
 	.show = backend_show,
 };
 
-static void nbd_del_disk(struct nbd_device *nbd)
-{
-	struct gendisk *disk = nbd->disk;
-
-	if (disk) {
-		del_gendisk(disk);
-		blk_cleanup_disk(disk);
-		blk_mq_free_tag_set(&nbd->tag_set);
-	}
-}
-
 /*
  * Place this in the last just before the nbd is freed to
  * make sure that the disk and the related kobject are also
@@ -261,7 +250,11 @@ static void nbd_notify_destroy_completion(struct nbd_device *nbd)
 
 static void nbd_dev_remove(struct nbd_device *nbd)
 {
-	nbd_del_disk(nbd);
+	struct gendisk *disk = nbd->disk;
+
+	del_gendisk(disk);
+	blk_cleanup_disk(disk);
+	blk_mq_free_tag_set(&nbd->tag_set);
 
 	/*
 	 * Remove from idr after del_gendisk() completes, so if the same ID is
-- 
2.30.2

[PATCH 4/6] nbd: return the allocated nbd_device from nbd_dev_add

[Christoph Hellwig]

Return the device we just allocated instead of doing an extra search for
it in the caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index de8b23af2486..08161c73c9ed 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1681,7 +1681,7 @@ static const struct blk_mq_ops nbd_mq_ops = {
 	.timeout	= nbd_xmit_timeout,
 };
 
-static int nbd_dev_add(int index)
+static struct nbd_device *nbd_dev_add(int index)
 {
 	struct nbd_device *nbd;
 	struct gendisk *disk;
@@ -1753,7 +1753,7 @@ static int nbd_dev_add(int index)
 	sprintf(disk->disk_name, "nbd%d", index);
 	add_disk(disk);
 	nbd_total_devices++;
-	return index;
+	return nbd;
 
 out_free_idr:
 	idr_remove(&nbd_index_idr, index);
@@ -1762,7 +1762,7 @@ static int nbd_dev_add(int index)
 out_free_nbd:
 	kfree(nbd);
 out:
-	return err;
+	return ERR_PTR(err);
 }
 
 static int find_free_cb(int id, void *ptr, void *data)
@@ -1848,25 +1848,22 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 	if (index == -1) {
 		ret = idr_for_each(&nbd_index_idr, &find_free_cb, &nbd);
 		if (ret == 0) {
-			int new_index;
-			new_index = nbd_dev_add(-1);
-			if (new_index < 0) {
+			nbd = nbd_dev_add(-1);
+			if (IS_ERR(nbd)) {
 				mutex_unlock(&nbd_index_mutex);
 				printk(KERN_ERR "nbd: failed to add new device\n");
-				return new_index;
+				return PTR_ERR(nbd);
 			}
-			nbd = idr_find(&nbd_index_idr, new_index);
 		}
 	} else {
 		nbd = idr_find(&nbd_index_idr, index);
 		if (!nbd) {
-			ret = nbd_dev_add(index);
-			if (ret < 0) {
+			nbd = nbd_dev_add(index);
+			if (IS_ERR(nbd)) {
 				mutex_unlock(&nbd_index_mutex);
 				printk(KERN_ERR "nbd: failed to add new device\n");
-				return ret;
+				return PTR_ERR(nbd);
 			}
-			nbd = idr_find(&nbd_index_idr, index);
 		}
 	}
 	if (!nbd) {
-- 
2.30.2

[PATCH 5/6] nbd: refactor device search and allocation in nbd_genl_connect

[Christoph Hellwig]

Use idr_for_each_entry instead of the awkward callback to find an
existing device for the index == -1 case, and de-duplicate the device
allocation if no existing device was found.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 45 ++++++++++++++-------------------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 08161c73c9ed..0b46a608f879 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1765,18 +1765,6 @@ static struct nbd_device *nbd_dev_add(int index)
 	return ERR_PTR(err);
 }
 
-static int find_free_cb(int id, void *ptr, void *data)
-{
-	struct nbd_device *nbd = ptr;
-	struct nbd_device **found = data;
-
-	if (!refcount_read(&nbd->config_refs)) {
-		*found = nbd;
-		return 1;
-	}
-	return 0;
-}
-
 /* Netlink interface. */
 static const struct nla_policy nbd_attr_policy[NBD_ATTR_MAX + 1] = {
 	[NBD_ATTR_INDEX]		=	{ .type = NLA_U32 },
@@ -1846,31 +1834,26 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 again:
 	mutex_lock(&nbd_index_mutex);
 	if (index == -1) {
-		ret = idr_for_each(&nbd_index_idr, &find_free_cb, &nbd);
-		if (ret == 0) {
-			nbd = nbd_dev_add(-1);
-			if (IS_ERR(nbd)) {
-				mutex_unlock(&nbd_index_mutex);
-				printk(KERN_ERR "nbd: failed to add new device\n");
-				return PTR_ERR(nbd);
+		struct nbd_device *tmp;
+		int id;
+
+		idr_for_each_entry(&nbd_index_idr, tmp, id) {
+			if (!refcount_read(&tmp->config_refs)) {
+				nbd = tmp;
+				break;
 			}
 		}
 	} else {
 		nbd = idr_find(&nbd_index_idr, index);
-		if (!nbd) {
-			nbd = nbd_dev_add(index);
-			if (IS_ERR(nbd)) {
-				mutex_unlock(&nbd_index_mutex);
-				printk(KERN_ERR "nbd: failed to add new device\n");
-				return PTR_ERR(nbd);
-			}
-		}
 	}
+
 	if (!nbd) {
-		printk(KERN_ERR "nbd: couldn't find device at index %d\n",
-		       index);
-		mutex_unlock(&nbd_index_mutex);
-		return -EINVAL;
+		nbd = nbd_dev_add(index);
+		if (IS_ERR(nbd)) {
+			mutex_unlock(&nbd_index_mutex);
+			pr_err("nbd: failed to add new device\n");
+			return PTR_ERR(nbd);
+		}
 	}
 
 	if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
-- 
2.30.2

[PATCH 6/6] nbd: reduce the nbd_index_mutex scope

[Christoph Hellwig]

nbd_index_mutex is currently held over add_disk and inside ->open, which
leads to lock order reversals.  Refactor the device creation code path
so that nbd_dev_add is called without nbd_index_mutex lock held and
only takes it for the IDR insertation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 55 +++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0b46a608f879..4054cc33fc1e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1681,7 +1681,7 @@ static const struct blk_mq_ops nbd_mq_ops = {
 	.timeout	= nbd_xmit_timeout,
 };
 
-static struct nbd_device *nbd_dev_add(int index)
+static struct nbd_device *nbd_dev_add(int index, unsigned int refs)
 {
 	struct nbd_device *nbd;
 	struct gendisk *disk;
@@ -1707,6 +1707,7 @@ static struct nbd_device *nbd_dev_add(int index)
 	if (err)
 		goto out_free_nbd;
 
+	mutex_lock(&nbd_index_mutex);
 	if (index >= 0) {
 		err = idr_alloc(&nbd_index_idr, nbd, index, index + 1,
 				GFP_KERNEL);
@@ -1717,6 +1718,7 @@ static struct nbd_device *nbd_dev_add(int index)
 		if (err >= 0)
 			index = err;
 	}
+	mutex_unlock(&nbd_index_mutex);
 	if (err < 0)
 		goto out_free_tags;
 	nbd->index = index;
@@ -1743,7 +1745,7 @@ static struct nbd_device *nbd_dev_add(int index)
 
 	mutex_init(&nbd->config_lock);
 	refcount_set(&nbd->config_refs, 0);
-	refcount_set(&nbd->refs, 1);
+	refcount_set(&nbd->refs, refs);
 	INIT_LIST_HEAD(&nbd->list);
 	disk->major = NBD_MAJOR;
 	disk->first_minor = index << part_shift;
@@ -1847,34 +1849,35 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 		nbd = idr_find(&nbd_index_idr, index);
 	}
 
-	if (!nbd) {
-		nbd = nbd_dev_add(index);
-		if (IS_ERR(nbd)) {
+	if (nbd) {
+		if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
+		    test_bit(NBD_DISCONNECT_REQUESTED, &nbd->flags)) {
+			nbd->destroy_complete = &destroy_complete;
 			mutex_unlock(&nbd_index_mutex);
-			pr_err("nbd: failed to add new device\n");
-			return PTR_ERR(nbd);
+
+			/* wait until the nbd device is completely destroyed */
+			wait_for_completion(&destroy_complete);
+			goto again;
 		}
-	}
 
-	if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
-	    test_bit(NBD_DISCONNECT_REQUESTED, &nbd->flags)) {
-		nbd->destroy_complete = &destroy_complete;
+		if (!refcount_inc_not_zero(&nbd->refs)) {
+			mutex_unlock(&nbd_index_mutex);
+			if (index == -1)
+				goto again;
+			pr_err("nbd: device at index %d is going down\n",
+		       		index);
+			return -EINVAL;
+		}
 		mutex_unlock(&nbd_index_mutex);
-
-		/* Wait untill the the nbd stuff is totally destroyed */
-		wait_for_completion(&destroy_complete);
-		goto again;
-	}
-
-	if (!refcount_inc_not_zero(&nbd->refs)) {
+	} else {
 		mutex_unlock(&nbd_index_mutex);
-		if (index == -1)
-			goto again;
-		printk(KERN_ERR "nbd: device at index %d is going down\n",
-		       index);
-		return -EINVAL;
+
+		nbd = nbd_dev_add(index, 2);
+		if (IS_ERR(nbd)) {
+			pr_err("nbd: failed to add new device\n");
+			return PTR_ERR(nbd);
+		}
 	}
-	mutex_unlock(&nbd_index_mutex);
 
 	mutex_lock(&nbd->config_lock);
 	if (refcount_read(&nbd->config_refs)) {
@@ -2430,10 +2433,8 @@ static int __init nbd_init(void)
 	}
 	nbd_dbg_init();
 
-	mutex_lock(&nbd_index_mutex);
 	for (i = 0; i < nbds_max; i++)
-		nbd_dev_add(i);
-	mutex_unlock(&nbd_index_mutex);
+		nbd_dev_add(i, 1);
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH 6/6] nbd: reduce the nbd_index_mutex scope

[Eric Blake]

On Wed, Aug 11, 2021 at 02:44:28PM +0200, Christoph Hellwig wrote:
> nbd_index_mutex is currently held over add_disk and inside ->open, which
> leads to lock order reversals.  Refactor the device creation code path
> so that nbd_dev_add is called without nbd_index_mutex lock held and
> only takes it for the IDR insertation.

insertion

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/block/nbd.c | 55 +++++++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 27 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 6/6] nbd: reduce the nbd_index_mutex scope

[Josef Bacik]

On 8/11/21 8:44 AM, Christoph Hellwig wrote:
> nbd_index_mutex is currently held over add_disk and inside ->open, which
> leads to lock order reversals.  Refactor the device creation code path
> so that nbd_dev_add is called without nbd_index_mutex lock held and
> only takes it for the IDR insertation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/block/nbd.c | 55 +++++++++++++++++++++++----------------------
>   1 file changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 0b46a608f879..4054cc33fc1e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -1681,7 +1681,7 @@ static const struct blk_mq_ops nbd_mq_ops = {
>   	.timeout	= nbd_xmit_timeout,
>   };
>   
> -static struct nbd_device *nbd_dev_add(int index)
> +static struct nbd_device *nbd_dev_add(int index, unsigned int refs)
>   {
>   	struct nbd_device *nbd;
>   	struct gendisk *disk;
> @@ -1707,6 +1707,7 @@ static struct nbd_device *nbd_dev_add(int index)
>   	if (err)
>   		goto out_free_nbd;
>   
> +	mutex_lock(&nbd_index_mutex);
>   	if (index >= 0) {
>   		err = idr_alloc(&nbd_index_idr, nbd, index, index + 1,
>   				GFP_KERNEL);
> @@ -1717,6 +1718,7 @@ static struct nbd_device *nbd_dev_add(int index)
>   		if (err >= 0)
>   			index = err;
>   	}
> +	mutex_unlock(&nbd_index_mutex);
>   	if (err < 0)
>   		goto out_free_tags;
>   	nbd->index = index;
> @@ -1743,7 +1745,7 @@ static struct nbd_device *nbd_dev_add(int index)
>   
>   	mutex_init(&nbd->config_lock);
>   	refcount_set(&nbd->config_refs, 0);
> -	refcount_set(&nbd->refs, 1);
> +	refcount_set(&nbd->refs, refs);
>   	INIT_LIST_HEAD(&nbd->list);
>   	disk->major = NBD_MAJOR;
>   	disk->first_minor = index << part_shift;
> @@ -1847,34 +1849,35 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
>   		nbd = idr_find(&nbd_index_idr, index);
>   	}
>   
> -	if (!nbd) {
> -		nbd = nbd_dev_add(index);
> -		if (IS_ERR(nbd)) {
> +	if (nbd) {
> +		if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
> +		    test_bit(NBD_DISCONNECT_REQUESTED, &nbd->flags)) {
> +			nbd->destroy_complete = &destroy_complete;
>   			mutex_unlock(&nbd_index_mutex);
> -			pr_err("nbd: failed to add new device\n");
> -			return PTR_ERR(nbd);
> +
> +			/* wait until the nbd device is completely destroyed */
> +			wait_for_completion(&destroy_complete);
> +			goto again;
>   		}
> -	}
>   
> -	if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
> -	    test_bit(NBD_DISCONNECT_REQUESTED, &nbd->flags)) {
> -		nbd->destroy_complete = &destroy_complete;
> +		if (!refcount_inc_not_zero(&nbd->refs)) {
> +			mutex_unlock(&nbd_index_mutex);
> +			if (index == -1)
> +				goto again;
> +			pr_err("nbd: device at index %d is going down\n",
> +		       		index);

Errant whitespace here.  Thanks,

Josef

Re: nbd locking fixups

[Josef Bacik]

On 8/11/21 8:44 AM, Christoph Hellwig wrote:
> Hi Josef and Jens,
> 
> this series fixed the lock order reversal that is showing up with
> nbd lately.  The first patch contains the asynchronous deletion
> from Hou which is needed as a baseline.
> 

Other than the whitespace thing everything looks good, you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: nbd locking fixups

[Jens Axboe]

On 8/11/21 6:44 AM, Christoph Hellwig wrote:
> Hi Josef and Jens,
> 
> this series fixed the lock order reversal that is showing up with
> nbd lately.  The first patch contains the asynchronous deletion
> from Hou which is needed as a baseline.

Applied, thanks.

-- 
Jens Axboe