From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org, vsementsov@virtuozzo.com,
eblake@redhat.com, mreitz@redhat.com, kwolf@redhat.com,
pbonzini@redhat.com, Roman Kagan <rvkagan@yandex-team.ru>
Subject: [PATCH v4 07/32] block/nbd: ensure ->connection_thread is always valid
Date: Thu, 10 Jun 2021 13:07:37 +0300 [thread overview]
Message-ID: <20210610100802.5888-8-vsementsov@virtuozzo.com> (raw)
In-Reply-To: <20210610100802.5888-1-vsementsov@virtuozzo.com>
From: Roman Kagan <rvkagan@yandex-team.ru>
Simplify lifetime management of BDRVNBDState->connect_thread by
delaying the possible cleanup of it until the BDRVNBDState itself goes
away.
This also reverts
0267101af6 "block/nbd: fix possible use after free of s->connect_thread"
as now s->connect_thread can't be cleared until the very end.
Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
[vsementsov: rebase, revert 0267101af6 changes]
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[eblake: tweak comment]
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/nbd.c | 56 ++++++++++++++++++++---------------------------------
1 file changed, 21 insertions(+), 35 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 1c99654ef7..08ae47d83c 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -144,17 +144,31 @@ typedef struct BDRVNBDState {
NBDConnectThread *connect_thread;
} BDRVNBDState;
+static void nbd_free_connect_thread(NBDConnectThread *thr);
static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr,
Error **errp);
static int nbd_co_establish_connection(BlockDriverState *bs, Error **errp);
-static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
- bool detach);
+static void nbd_co_establish_connection_cancel(BlockDriverState *bs);
static int nbd_client_handshake(BlockDriverState *bs, Error **errp);
static void nbd_yank(void *opaque);
static void nbd_clear_bdrvstate(BlockDriverState *bs)
{
BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
+ NBDConnectThread *thr = s->connect_thread;
+ bool thr_running;
+
+ qemu_mutex_lock(&thr->mutex);
+ thr_running = thr->state == CONNECT_THREAD_RUNNING;
+ if (thr_running) {
+ thr->state = CONNECT_THREAD_RUNNING_DETACHED;
+ }
+ qemu_mutex_unlock(&thr->mutex);
+
+ /* the runaway thread will clean up itself */
+ if (!thr_running) {
+ nbd_free_connect_thread(thr);
+ }
yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name));
@@ -295,7 +309,7 @@ static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs)
s->drained = true;
qemu_co_sleep_wake(&s->reconnect_sleep);
- nbd_co_establish_connection_cancel(bs, false);
+ nbd_co_establish_connection_cancel(bs);
reconnect_delay_timer_del(s);
@@ -333,7 +347,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
s->state = NBD_CLIENT_QUIT;
if (s->connection_co) {
qemu_co_sleep_wake(&s->reconnect_sleep);
- nbd_co_establish_connection_cancel(bs, true);
+ nbd_co_establish_connection_cancel(bs);
}
if (qemu_in_coroutine()) {
s->teardown_co = qemu_coroutine_self();
@@ -446,11 +460,6 @@ nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
BDRVNBDState *s = bs->opaque;
NBDConnectThread *thr = s->connect_thread;
- if (!thr) {
- /* detached */
- return -1;
- }
-
qemu_mutex_lock(&thr->mutex);
switch (thr->state) {
@@ -494,12 +503,6 @@ nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
s->wait_connect = true;
qemu_coroutine_yield();
- if (!s->connect_thread) {
- /* detached */
- return -1;
- }
- assert(thr == s->connect_thread);
-
qemu_mutex_lock(&thr->mutex);
switch (thr->state) {
@@ -547,18 +550,12 @@ nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
* nbd_co_establish_connection_cancel
* Cancel nbd_co_establish_connection asynchronously: it will finish soon, to
* allow drained section to begin.
- *
- * If detach is true, also cleanup the state (or if thread is running, move it
- * to CONNECT_THREAD_RUNNING_DETACHED state). s->connect_thread becomes NULL if
- * detach is true.
*/
-static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
- bool detach)
+static void nbd_co_establish_connection_cancel(BlockDriverState *bs)
{
BDRVNBDState *s = bs->opaque;
NBDConnectThread *thr = s->connect_thread;
bool wake = false;
- bool do_free = false;
qemu_mutex_lock(&thr->mutex);
@@ -569,21 +566,10 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
s->wait_connect = false;
wake = true;
}
- if (detach) {
- thr->state = CONNECT_THREAD_RUNNING_DETACHED;
- s->connect_thread = NULL;
- }
- } else if (detach) {
- do_free = true;
}
qemu_mutex_unlock(&thr->mutex);
- if (do_free) {
- nbd_free_connect_thread(thr);
- s->connect_thread = NULL;
- }
-
if (wake) {
aio_co_wake(s->connection_co);
}
@@ -2310,6 +2296,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
+ nbd_init_connect_thread(s);
+
/*
* establish TCP connection, return error if it fails
* TODO: Configurable retry-until-timeout behaviour.
@@ -2326,8 +2314,6 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
/* successfully connected */
s->state = NBD_CLIENT_CONNECTED;
- nbd_init_connect_thread(s);
-
s->connection_co = qemu_coroutine_create(nbd_connection_entry, s);
bdrv_inc_in_flight(bs);
aio_co_schedule(bdrv_get_aio_context(bs), s->connection_co);
--
2.29.2
next prev parent reply other threads:[~2021-06-10 10:11 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-10 10:07 [PATCH v4 00/32] block/nbd: rework client connection Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 01/32] co-queue: drop extra coroutine_fn marks Vladimir Sementsov-Ogievskiy
2021-06-10 17:22 ` Eric Blake
2021-06-10 17:37 ` Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 02/32] block/nbd: fix channel object leak Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 03/32] block/nbd: fix how state is cleared on nbd_open() failure paths Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 04/32] block/nbd: connect_thread_func(): do qio_channel_set_delay(false) Vladimir Sementsov-Ogievskiy
2021-06-10 18:37 ` Eric Blake
2021-06-10 10:07 ` [PATCH v4 05/32] qemu-sockets: introduce socket_address_parse_named_fd() Vladimir Sementsov-Ogievskiy
2021-06-11 13:22 ` Eric Blake
2021-06-11 14:10 ` Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 06/32] block/nbd: call socket_address_parse_named_fd() in advance Vladimir Sementsov-Ogievskiy
2021-06-11 13:54 ` Eric Blake
2021-06-10 10:07 ` Vladimir Sementsov-Ogievskiy [this message]
2021-06-10 10:07 ` [PATCH v4 08/32] block/nbd: nbd_client_handshake(): fix leak of s->ioc Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 09/32] block/nbd: BDRVNBDState: drop unused connect_err and connect_status Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 10/32] block/nbd: simplify waking of nbd_co_establish_connection() Vladimir Sementsov-Ogievskiy
2021-06-11 14:06 ` Eric Blake
2021-06-10 10:07 ` [PATCH v4 11/32] block/nbd: drop thr->state Vladimir Sementsov-Ogievskiy
2021-06-11 14:25 ` Eric Blake
2021-06-10 10:07 ` [PATCH v4 12/32] block/nbd: bs-independent interface for nbd_co_establish_connection() Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 13/32] block/nbd: make nbd_co_establish_connection_cancel() bs-independent Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 14/32] block/nbd: rename NBDConnectThread to NBDClientConnection Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 15/32] block/nbd: introduce nbd_client_connection_new() Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 16/32] block/nbd: introduce nbd_client_connection_release() Vladimir Sementsov-Ogievskiy
2021-06-11 14:28 ` Eric Blake
2021-06-10 10:07 ` [PATCH v4 17/32] nbd: move connection code from block/nbd to nbd/client-connection Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 18/32] nbd/client-connection: use QEMU_LOCK_GUARD Vladimir Sementsov-Ogievskiy
2021-06-11 14:31 ` Eric Blake
2021-06-10 10:07 ` [PATCH v4 19/32] nbd/client-connection: add possibility of negotiation Vladimir Sementsov-Ogievskiy
2021-06-11 15:07 ` Eric Blake
2021-06-10 10:07 ` [PATCH v4 20/32] nbd/client-connection: implement connection retry Vladimir Sementsov-Ogievskiy
2021-06-11 15:12 ` Eric Blake
2021-11-22 16:30 ` Eric Blake
2021-11-22 17:17 ` Vladimir Sementsov-Ogievskiy
2021-11-22 21:51 ` Eric Blake
2021-06-10 10:07 ` [PATCH v4 21/32] nbd/client-connection: shutdown connection on release Vladimir Sementsov-Ogievskiy
2021-06-11 15:27 ` Eric Blake
2021-06-10 10:07 ` [PATCH v4 22/32] block/nbd: split nbd_handle_updated_info out of nbd_client_handshake() Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 23/32] block/nbd: use negotiation of NBDClientConnection Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 24/32] block/nbd: don't touch s->sioc in nbd_teardown_connection() Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 25/32] block/nbd: drop BDRVNBDState::sioc Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 26/32] nbd/client-connection: return only one io channel Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 27/32] block-coroutine-wrapper: allow non bdrv_ prefix Vladimir Sementsov-Ogievskiy
2021-06-10 10:07 ` [PATCH v4 28/32] block/nbd: split nbd_co_do_establish_connection out of nbd_reconnect_attempt Vladimir Sementsov-Ogievskiy
2021-06-11 15:29 ` Eric Blake
2021-06-10 10:07 ` [PATCH v4 29/32] nbd/client-connection: add option for non-blocking connection attempt Vladimir Sementsov-Ogievskiy
2021-06-10 10:08 ` [PATCH v4 30/32] block/nbd: reuse nbd_co_do_establish_connection() in nbd_open() Vladimir Sementsov-Ogievskiy
2021-06-10 10:08 ` [PATCH v4 31/32] block/nbd: add nbd_client_connected() helper Vladimir Sementsov-Ogievskiy
2021-06-10 10:08 ` [PATCH v4 32/32] block/nbd: safer transition to receiving request Vladimir Sementsov-Ogievskiy
2021-06-11 15:55 ` [PATCH v4 00/32] block/nbd: rework client connection Eric Blake
2021-06-11 17:23 ` Vladimir Sementsov-Ogievskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210610100802.5888-8-vsementsov@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=eblake@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=rvkagan@yandex-team.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.