* [PATCH 1/6] nbd: add missing locking to the nbd_dev_add error path
2021-08-25 16:31 nbd lifetimes fixes Christoph Hellwig
@ 2021-08-25 16:31 ` Christoph Hellwig
2021-08-25 16:31 ` [PATCH 2/6] nbd: reset NBD to NULL when restarting in nbd_genl_connect Christoph Hellwig
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-08-25 16:31 UTC (permalink / raw)
To: Josef Bacik, Jens Axboe
Cc: linux-block, nbd, Xiubo Li, Tetsuo Handa, Tetsuo Handa
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/6] nbd: reset NBD to NULL when restarting in nbd_genl_connect
2021-08-25 16:31 nbd lifetimes fixes Christoph Hellwig
2021-08-25 16:31 ` [PATCH 1/6] nbd: add missing locking to the nbd_dev_add error path Christoph Hellwig
@ 2021-08-25 16:31 ` Christoph Hellwig
2021-08-25 16:31 ` [PATCH 3/6] nbd: prevent IDR lookups from finding partially initialized devices Christoph Hellwig
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-08-25 16:31 UTC (permalink / raw)
To: Josef Bacik, Jens Axboe
Cc: linux-block, nbd, Xiubo Li, Tetsuo Handa, Hillf Danton
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 <hdanton@sina.com>
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/6] nbd: prevent IDR lookups from finding partially initialized devices
2021-08-25 16:31 nbd lifetimes fixes Christoph Hellwig
2021-08-25 16:31 ` [PATCH 1/6] nbd: add missing locking to the nbd_dev_add error path Christoph Hellwig
2021-08-25 16:31 ` [PATCH 2/6] nbd: reset NBD to NULL when restarting in nbd_genl_connect Christoph Hellwig
@ 2021-08-25 16:31 ` Christoph Hellwig
2021-08-25 16:31 ` [PATCH 4/6] nbd: set nbd->index before releasing nbd_index_mutex Christoph Hellwig
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-08-25 16:31 UTC (permalink / raw)
To: Josef Bacik, Jens Axboe
Cc: linux-block, nbd, Xiubo Li, Tetsuo Handa, Tetsuo Handa
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/6] nbd: set nbd->index before releasing nbd_index_mutex
2021-08-25 16:31 nbd lifetimes fixes Christoph Hellwig
` (2 preceding siblings ...)
2021-08-25 16:31 ` [PATCH 3/6] nbd: prevent IDR lookups from finding partially initialized devices Christoph Hellwig
@ 2021-08-25 16:31 ` Christoph Hellwig
2021-08-25 16:31 ` [PATCH 5/6] nbd: only return usable devices from nbd_find_unused Christoph Hellwig
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-08-25 16:31 UTC (permalink / raw)
To: Josef Bacik, Jens Axboe
Cc: linux-block, nbd, Xiubo Li, Tetsuo Handa, Tetsuo Handa
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/6] nbd: only return usable devices from nbd_find_unused
2021-08-25 16:31 nbd lifetimes fixes Christoph Hellwig
` (3 preceding siblings ...)
2021-08-25 16:31 ` [PATCH 4/6] nbd: set nbd->index before releasing nbd_index_mutex Christoph Hellwig
@ 2021-08-25 16:31 ` Christoph Hellwig
2021-08-25 16:31 ` [PATCH 6/6] nbd: remove nbd->destroy_complete Christoph Hellwig
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-08-25 16:31 UTC (permalink / raw)
To: Josef Bacik, Jens Axboe; +Cc: linux-block, nbd, Xiubo Li, Tetsuo Handa
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 6/6] nbd: remove nbd->destroy_complete
2021-08-25 16:31 nbd lifetimes fixes Christoph Hellwig
` (4 preceding siblings ...)
2021-08-25 16:31 ` [PATCH 5/6] nbd: only return usable devices from nbd_find_unused Christoph Hellwig
@ 2021-08-25 16:31 ` Christoph Hellwig
2021-08-25 20:14 ` nbd lifetimes fixes Josef Bacik
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-08-25 16:31 UTC (permalink / raw)
To: Josef Bacik, Jens Axboe
Cc: linux-block, nbd, Xiubo Li, Tetsuo Handa, Tetsuo Handa,
syzbot+2c98885bcd769f56b6d6
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: nbd lifetimes fixes
2021-08-25 16:31 nbd lifetimes fixes Christoph Hellwig
` (5 preceding siblings ...)
2021-08-25 16:31 ` [PATCH 6/6] nbd: remove nbd->destroy_complete Christoph Hellwig
@ 2021-08-25 20:14 ` Josef Bacik
2021-08-25 20:21 ` Jens Axboe
2021-10-08 9:17 ` Xiubo Li
8 siblings, 0 replies; 10+ messages in thread
From: Josef Bacik @ 2021-08-25 20:14 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe; +Cc: linux-block, nbd, Xiubo Li, Tetsuo Handa
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: nbd lifetimes fixes
2021-08-25 16:31 nbd lifetimes fixes Christoph Hellwig
` (6 preceding siblings ...)
2021-08-25 20:14 ` nbd lifetimes fixes Josef Bacik
@ 2021-08-25 20:21 ` Jens Axboe
2021-10-08 9:17 ` Xiubo Li
8 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2021-08-25 20:21 UTC (permalink / raw)
To: Christoph Hellwig, Josef Bacik; +Cc: linux-block, nbd, Xiubo Li, Tetsuo Handa
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: nbd lifetimes fixes
2021-08-25 16:31 nbd lifetimes fixes Christoph Hellwig
` (7 preceding siblings ...)
2021-08-25 20:21 ` Jens Axboe
@ 2021-10-08 9:17 ` Xiubo Li
8 siblings, 0 replies; 10+ messages in thread
From: Xiubo Li @ 2021-10-08 9:17 UTC (permalink / raw)
To: Christoph Hellwig, Josef Bacik, Jens Axboe; +Cc: linux-block, nbd, Tetsuo Handa
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.
^ permalink raw reply [flat|nested] 10+ messages in thread