From: Roman Kagan
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
[vsementsov: rebase, revert 0267101af6 changes]
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 56 -
1 file changed, 21 insertions(+), 35 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index a407a3814b..272af60b44 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(>mutex);
+thr_running = thr->state == CONNECT_THREAD_RUNNING;
+if (thr_running) {
+thr->state = CONNECT_THREAD_RUNNING_DETACHED;
+}
+qemu_mutex_unlock(>mutex);
+
+/* the runaway thread will clean it up itself */
+if (!thr_running) {
+nbd_free_connect_thread(thr);
+}
yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name));
@@ -297,7 +311,7 @@ static void coroutine_fn
nbd_client_co_drain_begin(BlockDriverState *bs)
qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
}
-nbd_co_establish_connection_cancel(bs, false);
+nbd_co_establish_connection_cancel(bs);
reconnect_delay_timer_del(s);
@@ -337,7 +351,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
if (s->connection_co_sleep_ns_state) {
qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
}
-nbd_co_establish_connection_cancel(bs, true);
+nbd_co_establish_connection_cancel(bs);
}
if (qemu_in_coroutine()) {
s->teardown_co = qemu_coroutine_self();
@@ -448,11 +462,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(>mutex);
switch (thr->state) {
@@ -496,12 +505,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(>mutex);
switch (thr->state) {
@@ -549,18 +552,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(>mutex);
@@ -571,21 +568,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(>mutex);
-if (do_free) {
-nbd_free_connect_thread(thr);
-s->connect_thread = NULL;
-}
-
if (wake) {
aio_co_wake(s->connection_co);
}
@@ -2306,6 +2292,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.
@@ -2322,8 +2310,6 @@ static int nbd_open(BlockDriverState *bs, QDict
