nbd lifetimes fixes

10 messages, 4 authors, 2021-10-08 · open the first message on its own page

nbd lifetimes fixes

From: Christoph Hellwig <hch@lst.de>
Date: 2021-08-25 16:34:32

Hi Josef and Jens,

this series tries to deal with the fallout of the recent lock scope
reduction as pointed out by Tetsuo and szybot and inspired by /
reused from the catchall patch by Tetsuo.  One big change is that
I finally decided to kill off the ->destroy_complete scheme entirely
because I'm pretty sure it is not needed with the various fixes and
we can just return an error for the tiny race condition where it
matters.  Xiubo, can you double check this with your nbd-runner
setup?  nbd-runner itself seems pretty generic and not directly
reproduce anything here.

Note that the syzbot reproduer still fails eventually, but in
devtmpfsd in a way that does not look related to the loop code
at all.

[PATCH 1/6] nbd: add missing locking to the nbd_dev_add error path

From: Christoph Hellwig <hch@lst.de>
Date: 2021-08-25 16:36:23

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>

idr_remove needs external synchronization.

Fixes: 6e4df4c64881 ("nbd: reduce the nbd_index_mutex scope")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[hch: split from a larger patch]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index c5e2b4cd697f..2c63372a31dd 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c

@@ -1774,7 +1774,9 @@ static struct nbd_device *nbd_dev_add(int index, unsigned int refs)
 	return nbd;
 
 out_free_idr:
+	mutex_lock(&nbd_index_mutex);
 	idr_remove(&nbd_index_idr, index);
+	mutex_unlock(&nbd_index_mutex);
 out_free_tags:
 	blk_mq_free_tag_set(&nbd->tag_set);
 out_free_nbd:

-- 
2.30.2

[PATCH 2/6] nbd: reset NBD to NULL when restarting in nbd_genl_connect

From: Christoph Hellwig <hch@lst.de>
Date: 2021-08-25 16:37:29

When nbd_genl_connect restarts to wait for a disconnecting device, nbd
needs to be reset to NULL.  Do that by facoring out a helper to find
an unused device.

Fixes: 6177b56c96ff ("nbd: refactor device search and allocation in nbd_genl_connect")
Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Reported-by: Hillf Danton <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 2c63372a31dd..69971a47c36f 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c

@@ -1785,6 +1785,20 @@ static struct nbd_device *nbd_dev_add(int index, unsigned int refs)
 	return ERR_PTR(err);
 }
 
+static struct nbd_device *nbd_find_unused(void)
+{
+	struct nbd_device *nbd;
+	int id;
+
+	lockdep_assert_held(&nbd_index_mutex);
+
+	idr_for_each_entry(&nbd_index_idr, nbd, id)
+		if (!refcount_read(&nbd->config_refs))
+			return nbd;
+
+	return NULL;
+}
+
 /* Netlink interface. */
 static const struct nla_policy nbd_attr_policy[NBD_ATTR_MAX + 1] = {
 	[NBD_ATTR_INDEX]		=	{ .type = NLA_U32 },

@@ -1832,7 +1846,7 @@ static int nbd_genl_size_set(struct genl_info *info, struct nbd_device *nbd)
 static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 {
 	DECLARE_COMPLETION_ONSTACK(destroy_complete);
-	struct nbd_device *nbd = NULL;
+	struct nbd_device *nbd;
 	struct nbd_config *config;
 	int index = -1;
 	int ret;

@@ -1853,20 +1867,10 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 	}
 again:
 	mutex_lock(&nbd_index_mutex);
-	if (index == -1) {
-		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 {
+	if (index == -1)
+		nbd = nbd_find_unused();
+	else
 		nbd = idr_find(&nbd_index_idr, index);
-	}
-
 	if (nbd) {
 		if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
 		    test_bit(NBD_DISCONNECT_REQUESTED, &nbd->flags)) {

-- 
2.30.2

[PATCH 3/6] nbd: prevent IDR lookups from finding partially initialized devices

From: Christoph Hellwig <hch@lst.de>
Date: 2021-08-25 16:38:57

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>

Previously nbd_index_mutex was held during whole add/remove/lookup
operations in order to guarantee that partially initialized devices are
not reachable via idr_find() or idr_for_each(). But now that partially
initialized devices become reachable as soon as idr_alloc() succeeds,
we need to skip partially initialized devices. Since it seems that
all functions use refcount_inc_not_zero(&nbd->refs) in order to skip
destroying devices, update nbd->refs from zero to non-zero as the last
step of device initialization in order to also skip partially initialized
devices.

Fixes: 6e4df4c64881 ("nbd: reduce the nbd_index_mutex scope")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[hch: split from a larger patch, added comments]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 69971a47c36f..dfaa95df8d6c 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c

@@ -1751,7 +1751,11 @@ static struct nbd_device *nbd_dev_add(int index, unsigned int refs)
 
 	mutex_init(&nbd->config_lock);
 	refcount_set(&nbd->config_refs, 0);
-	refcount_set(&nbd->refs, refs);
+	/*
+	 * Start out with a zero references to keep other threads from using
+	 * this device until it is fully initialized.
+	 */
+	refcount_set(&nbd->refs, 0);
 	INIT_LIST_HEAD(&nbd->list);
 	disk->major = NBD_MAJOR;

@@ -1770,6 +1774,11 @@ static struct nbd_device *nbd_dev_add(int index, unsigned int refs)
 	disk->private_data = nbd;
 	sprintf(disk->disk_name, "nbd%d", index);
 	add_disk(disk);
+
+	/*
+	 * Now publish the device.
+	 */
+	refcount_set(&nbd->refs, refs);
 	nbd_total_devices++;
 	return nbd;

-- 
2.30.2

[PATCH 4/6] nbd: set nbd->index before releasing nbd_index_mutex

From: Christoph Hellwig <hch@lst.de>
Date: 2021-08-25 16:40:53

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>

Set nbd->index before releasing nbd_index_mutex, as populate_nbd_status()
might access nbd->index as soon as nbd_index_mutex is released.

Fixes: 6e4df4c64881 ("nbd: reduce the nbd_index_mutex scope")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[hch: split from a larger patch]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index dfaa95df8d6c..042af761d3a4 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c

@@ -1724,10 +1724,10 @@ static struct nbd_device *nbd_dev_add(int index, unsigned int refs)
 		if (err >= 0)
 			index = err;
 	}
+	nbd->index = index;
 	mutex_unlock(&nbd_index_mutex);
 	if (err < 0)
 		goto out_free_tags;
-	nbd->index = index;
 
 	disk = blk_mq_alloc_disk(&nbd->tag_set, NULL);
 	if (IS_ERR(disk)) {

-- 
2.30.2

[PATCH 5/6] nbd: only return usable devices from nbd_find_unused

From: Christoph Hellwig <hch@lst.de>
Date: 2021-08-25 16:42:10

Device marked as NBD_DESTROY_ON_DISCONNECT can and should be skipped
given that they won't survive the disconnect.  So skip them and try
to grab a reference directly and just continue if the the devices
is being torn down or created and thus has a zero refcount.

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 042af761d3a4..5c03f3eb3129 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c

@@ -1794,16 +1794,20 @@ static struct nbd_device *nbd_dev_add(int index, unsigned int refs)
 	return ERR_PTR(err);
 }
 
-static struct nbd_device *nbd_find_unused(void)
+static struct nbd_device *nbd_find_get_unused(void)
 {
 	struct nbd_device *nbd;
 	int id;
 
 	lockdep_assert_held(&nbd_index_mutex);
 
-	idr_for_each_entry(&nbd_index_idr, nbd, id)
-		if (!refcount_read(&nbd->config_refs))
+	idr_for_each_entry(&nbd_index_idr, nbd, id) {
+		if (refcount_read(&nbd->config_refs) ||
+		    test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags))
+			continue;
+		if (refcount_inc_not_zero(&nbd->refs))
 			return nbd;
+	}
 
 	return NULL;
 }

@@ -1877,10 +1881,10 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 again:
 	mutex_lock(&nbd_index_mutex);
 	if (index == -1)
-		nbd = nbd_find_unused();
+		nbd = nbd_find_get_unused();
 	else
 		nbd = idr_find(&nbd_index_idr, index);
-	if (nbd) {
+	if (nbd && index != -1) {
 		if (test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
 		    test_bit(NBD_DISCONNECT_REQUESTED, &nbd->flags)) {
 			nbd->destroy_complete = &destroy_complete;

@@ -1893,8 +1897,6 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 
 		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;

-- 
2.30.2

[PATCH 6/6] nbd: remove nbd->destroy_complete

From: Christoph Hellwig <hch@lst.de>
Date: 2021-08-25 16:43:14

The nbd->destroy_complete pointer is not really needed.  For creating
a device without a specific index we now simplify skip devices marked
NBD_DESTROY_ON_DISCONNECT as there is not much point to reuse them.
For device creation with a specific index there is no real need to
treat the case of a requested but not finished disconnect different
than any other device that is being shutdown, i.e. we can just return
an error, as a slightly different race window would anyway.

Fixes: 6e4df4c64881 ("nbd: reduce the nbd_index_mutex scope")
Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot+2c98885bcd769f56b6d6@syzkaller.appspotmail.com
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 52 ++++++++++++---------------------------------
 1 file changed, 14 insertions(+), 38 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 5c03f3eb3129..5170a630778d 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c

@@ -120,7 +120,6 @@ struct nbd_device {
 	struct task_struct *task_recv;
 	struct task_struct *task_setup;
 
-	struct completion *destroy_complete;
 	unsigned long flags;
 
 	char *backend;

@@ -235,19 +234,6 @@ static const struct device_attribute backend_attr = {
 	.show = backend_show,
 };
 
-/*
- * 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(struct nbd_device *nbd)
 {
 	struct gendisk *disk = nbd->disk;

@@ -262,7 +248,6 @@ static void nbd_dev_remove(struct nbd_device *nbd)
 	 */
 	mutex_lock(&nbd_index_mutex);
 	idr_remove(&nbd_index_idr, nbd->index);
-	nbd_notify_destroy_completion(nbd);
 	mutex_unlock(&nbd_index_mutex);
 
 	kfree(nbd);

@@ -1706,7 +1691,6 @@ static struct nbd_device *nbd_dev_add(int index, unsigned int refs)
 		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;
 
 	err = blk_mq_alloc_tag_set(&nbd->tag_set);

@@ -1858,7 +1842,6 @@ static int nbd_genl_size_set(struct genl_info *info, struct nbd_device *nbd)
 
 static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 {
-	DECLARE_COMPLETION_ONSTACK(destroy_complete);
 	struct nbd_device *nbd;
 	struct nbd_config *config;
 	int index = -1;

@@ -1880,31 +1863,24 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 	}
 again:
 	mutex_lock(&nbd_index_mutex);
-	if (index == -1)
+	if (index == -1) {
 		nbd = nbd_find_get_unused();
-	else
+	} else {
 		nbd = idr_find(&nbd_index_idr, index);
-	if (nbd && index != -1) {
-		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);
-
-			/* wait until the nbd device is completely destroyed */
-			wait_for_completion(&destroy_complete);
-			goto again;
-		}
-
-		if (!refcount_inc_not_zero(&nbd->refs)) {
-			mutex_unlock(&nbd_index_mutex);
-			pr_err("nbd: device at index %d is going down\n",
-				index);
-			return -EINVAL;
+		if (nbd) {
+			if ((test_bit(NBD_DESTROY_ON_DISCONNECT, &nbd->flags) &&
+			     test_bit(NBD_DISCONNECT_REQUESTED, &nbd->flags)) ||
+			    !refcount_inc_not_zero(&nbd->refs)) {
+				mutex_unlock(&nbd_index_mutex);
+				pr_err("nbd: device at index %d is going down\n",
+					index);
+				return -EINVAL;
+			}
 		}
-		mutex_unlock(&nbd_index_mutex);
-	} else {
-		mutex_unlock(&nbd_index_mutex);
+	}
+	mutex_unlock(&nbd_index_mutex);
 
+	if (!nbd) {
 		nbd = nbd_dev_add(index, 2);
 		if (IS_ERR(nbd)) {
 			pr_err("nbd: failed to add new device\n");

-- 
2.30.2

Re: nbd lifetimes fixes

From: Josef Bacik <josef@toxicpanda.com>
Date: 2021-08-25 20:15:02

On 8/25/21 12:31 PM, Christoph Hellwig wrote:

Hi Josef and Jens,

this series tries to deal with the fallout of the recent lock scope
reduction as pointed out by Tetsuo and szybot and inspired by /
reused from the catchall patch by Tetsuo.  One big change is that
I finally decided to kill off the ->destroy_complete scheme entirely
because I'm pretty sure it is not needed with the various fixes and
we can just return an error for the tiny race condition where it
matters.  Xiubo, can you double check this with your nbd-runner
setup?  nbd-runner itself seems pretty generic and not directly
reproduce anything here.

Note that the syzbot reproduer still fails eventually, but in
devtmpfsd in a way that does not look related to the loop code
at all.

Looks good to me, you can add

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

Thanks,

Josef

Re: nbd lifetimes fixes

From: Jens Axboe <axboe@kernel.dk>
Date: 2021-08-25 20:21:36

On 8/25/21 10:31 AM, Christoph Hellwig wrote:

Hi Josef and Jens,

this series tries to deal with the fallout of the recent lock scope
reduction as pointed out by Tetsuo and szybot and inspired by /
reused from the catchall patch by Tetsuo.  One big change is that
I finally decided to kill off the ->destroy_complete scheme entirely
because I'm pretty sure it is not needed with the various fixes and
we can just return an error for the tiny race condition where it
matters.  Xiubo, can you double check this with your nbd-runner
setup?  nbd-runner itself seems pretty generic and not directly
reproduce anything here.

Note that the syzbot reproduer still fails eventually, but in
devtmpfsd in a way that does not look related to the loop code
at all.

Applied, thanks.

-- 
Jens Axboe

Re: nbd lifetimes fixes

From: Xiubo Li <hidden>
Date: 2021-10-08 09:17:34

On 8/26/21 12:31 AM, Christoph Hellwig wrote:

Hi Josef and Jens,

this series tries to deal with the fallout of the recent lock scope
reduction as pointed out by Tetsuo and szybot and inspired by /
reused from the catchall patch by Tetsuo.  One big change is that
I finally decided to kill off the ->destroy_complete scheme entirely
because I'm pretty sure it is not needed with the various fixes and
we can just return an error for the tiny race condition where it
matters.  Xiubo, can you double check this with your nbd-runner
setup?  nbd-runner itself seems pretty generic and not directly
reproduce anything here.

Note that the syzbot reproduer still fails eventually, but in
devtmpfsd in a way that does not look related to the loop code
at all.

Sorry, I think I have missed this thread.

Test this and works well for me by using the nbd-runner.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help