The structure is shared between NBD BDS and connection thread. And it
is possible the connect thread will finish after closing and releasing
for the bs. To handle this we have a concept of
CONNECT_THREAD_RUNNING_DETACHED state and when thread is running and
BDS is going to be closed we don't free the structure, but instead move
it to CONNECT_THREAD_RUNNING_DETACHED state, so that thread will free
it.
Still more native way to solve the problem is using reference counter
for shared structure. Let's use it. It makes code smaller and more
readable.
New approach also makes checks in nbd_co_establish_connection()
redundant: now we are sure that s->connect_thread is valid during the
whole life of NBD BDS.
This also fixes possible use-after-free of s->connect_thread if
nbd_co_establish_connection_cancel() clears it during
nbd_co_establish_connection(), and nbd_co_establish_connection() uses
local copy of s->connect_thread after yield point.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
block/nbd.c | 62 +++++++++++++++++------------------------------------
1 file changed, 20 insertions(+), 42 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index c26dc5a54f..64b2977dc8 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -73,12 +73,6 @@ typedef enum NBDConnectThreadState {
/* Thread is running, no results for now */
CONNECT_THREAD_RUNNING,
- /*
- * Thread is running, but requestor exited. Thread should close
- * the new socket and free the connect state on exit.
- */
- CONNECT_THREAD_RUNNING_DETACHED,
-
/* Thread finished, results are stored in a state */
CONNECT_THREAD_FAIL,
CONNECT_THREAD_SUCCESS
@@ -101,6 +95,8 @@ typedef struct NBDConnectThread {
QIOChannelSocket *sioc;
Error *err;
+ int refcnt; /* atomic access */
+
/* state and bh_ctx are protected by mutex */
QemuMutex mutex;
NBDConnectThreadState state; /* current state of the thread */
@@ -144,16 +140,18 @@ typedef struct BDRVNBDState {
NBDConnectThread *connect_thread;
} BDRVNBDState;
+static void connect_thread_state_unref(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(BDRVNBDState *s)
{
+ connect_thread_state_unref(s->connect_thread);
+ s->connect_thread = NULL;
object_unref(OBJECT(s->tlscreds));
qapi_free_SocketAddress(s->saddr);
s->saddr = NULL;
@@ -293,7 +291,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);
@@ -333,7 +331,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();
@@ -376,26 +374,28 @@ static void nbd_init_connect_thread(BDRVNBDState *s)
.state = CONNECT_THREAD_NONE,
.bh_func = connect_bh,
.bh_opaque = s,
+ .refcnt = 1,
};
qemu_mutex_init(&s->connect_thread->mutex);
}
-static void nbd_free_connect_thread(NBDConnectThread *thr)
+static void connect_thread_state_unref(NBDConnectThread *thr)
{
- if (thr->sioc) {
- qio_channel_close(QIO_CHANNEL(thr->sioc), NULL);
+ if (qatomic_dec_fetch(&thr->refcnt) == 0) {
+ if (thr->sioc) {
+ qio_channel_close(QIO_CHANNEL(thr->sioc), NULL);
+ }
+ error_free(thr->err);
+ qapi_free_SocketAddress(thr->saddr);
+ g_free(thr);
}
- error_free(thr->err);
- qapi_free_SocketAddress(thr->saddr);
- g_free(thr);
}
static void *connect_thread_func(void *opaque)
{
NBDConnectThread *thr = opaque;
int ret;
- bool do_free = false;
thr->sioc = qio_channel_socket_new();
@@ -419,18 +419,13 @@ static void *connect_thread_func(void *opaque)
thr->bh_ctx = NULL;
}
break;
- case CONNECT_THREAD_RUNNING_DETACHED:
- do_free = true;
- break;
default:
abort();
}
qemu_mutex_unlock(&thr->mutex);
- if (do_free) {
- nbd_free_connect_thread(thr);
- }
+ connect_thread_state_unref(thr);
return NULL;
}
@@ -451,6 +446,7 @@ nbd_co_establish_connection(BlockDriverState *bs, Error
**errp)
error_free(thr->err);
thr->err = NULL;
thr->state = CONNECT_THREAD_RUNNING;
+ qatomic_inc(&thr->refcnt); /* for thread */
qemu_thread_create(&thread, "nbd-connect",
connect_thread_func, thr, QEMU_THREAD_DETACHED);
break;
@@ -503,7 +499,6 @@ nbd_co_establish_connection(BlockDriverState *bs, Error
**errp)
ret = (s->sioc ? 0 : -1);
break;
case CONNECT_THREAD_RUNNING:
- case CONNECT_THREAD_RUNNING_DETACHED:
/*
* Obviously, drained section wants to start. Report the attempt as
* failed. Still connect thread is executing in background, and its
@@ -533,18 +528,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);
@@ -555,21 +544,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);
}
--
2.29.2
