From: Michael Tokarev <mjt@tls.msk.ru>
To: qemu-devel@nongnu.org
Cc: qemu-stable@nongnu.org, Zhu Yangyang <zhuyangyang14@huawei.com>,
Eric Blake <eblake@redhat.com>,
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>,
Michael Tokarev <mjt@tls.msk.ru>
Subject: [Stable-8.2.4 06/16] nbd/server: do not poll within a coroutine context
Date: Tue, 7 May 2024 11:42:05 +0300 [thread overview]
Message-ID: <20240507084226.1026455-6-mjt@tls.msk.ru> (raw)
In-Reply-To: <qemu-stable-8.2.4-20240506205855@cover.tls.msk.ru>
From: Zhu Yangyang <zhuyangyang14@huawei.com>
Coroutines are not supposed to block. Instead, they should yield.
The client performs TLS upgrade outside of an AIOContext, during
synchronous handshake; this still requires g_main_loop. But the
server responds to TLS upgrade inside a coroutine, so a nested
g_main_loop is wrong. Since the two callbacks no longer share more
than the setting of data.complete and data.error, it's just as easy to
use static helpers instead of trying to share a common code path. It
is also possible to add assertions that no other code is interfering
with the eventual path to qio reaching the callback, whether or not it
required a yield or main loop.
Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation")
Signed-off-by: Zhu Yangyang <zhuyangyang14@huawei.com>
[eblake: move callbacks to their use point, add assertions]
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-ID: <20240408160214.1200629-5-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
(cherry picked from commit ae6d91a7e9b77abb029ed3fa9fad461422286942)
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
diff --git a/nbd/client.c b/nbd/client.c
index 29ffc609a4..c89c750467 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -596,13 +596,31 @@ static int nbd_request_simple_option(QIOChannel *ioc, int opt, bool strict,
return 1;
}
+/* Callback to learn when QIO TLS upgrade is complete */
+struct NBDTLSClientHandshakeData {
+ bool complete;
+ Error *error;
+ GMainLoop *loop;
+};
+
+static void nbd_client_tls_handshake(QIOTask *task, void *opaque)
+{
+ struct NBDTLSClientHandshakeData *data = opaque;
+
+ qio_task_propagate_error(task, &data->error);
+ data->complete = true;
+ if (data->loop) {
+ g_main_loop_quit(data->loop);
+ }
+}
+
static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
const char *hostname, Error **errp)
{
int ret;
QIOChannelTLS *tioc;
- struct NBDTLSHandshakeData data = { 0 };
+ struct NBDTLSClientHandshakeData data = { 0 };
ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, true, errp);
if (ret <= 0) {
@@ -619,18 +637,20 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
return NULL;
}
qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
- data.loop = g_main_loop_new(g_main_context_default(), FALSE);
trace_nbd_receive_starttls_tls_handshake();
qio_channel_tls_handshake(tioc,
- nbd_tls_handshake,
+ nbd_client_tls_handshake,
&data,
NULL,
NULL);
if (!data.complete) {
+ data.loop = g_main_loop_new(g_main_context_default(), FALSE);
g_main_loop_run(data.loop);
+ assert(data.complete);
+ g_main_loop_unref(data.loop);
}
- g_main_loop_unref(data.loop);
+
if (data.error) {
error_propagate(errp, data.error);
object_unref(OBJECT(tioc));
diff --git a/nbd/common.c b/nbd/common.c
index 3247c1d618..589a748cfe 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -47,17 +47,6 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
}
-void nbd_tls_handshake(QIOTask *task,
- void *opaque)
-{
- struct NBDTLSHandshakeData *data = opaque;
-
- qio_task_propagate_error(task, &data->error);
- data->complete = true;
- g_main_loop_quit(data->loop);
-}
-
-
const char *nbd_opt_lookup(uint32_t opt)
{
switch (opt) {
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index dfa02f77ee..91895106a9 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -72,16 +72,6 @@ static inline int nbd_write(QIOChannel *ioc, const void *buffer, size_t size,
return qio_channel_write_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
}
-struct NBDTLSHandshakeData {
- GMainLoop *loop;
- bool complete;
- Error *error;
-};
-
-
-void nbd_tls_handshake(QIOTask *task,
- void *opaque);
-
int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);
#endif
diff --git a/nbd/server.c b/nbd/server.c
index 091b57119e..9fbac7d409 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -748,6 +748,23 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
return rc;
}
+/* Callback to learn when QIO TLS upgrade is complete */
+struct NBDTLSServerHandshakeData {
+ bool complete;
+ Error *error;
+ Coroutine *co;
+};
+
+static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
+{
+ struct NBDTLSServerHandshakeData *data = opaque;
+
+ qio_task_propagate_error(task, &data->error);
+ data->complete = true;
+ if (!qemu_coroutine_entered(data->co)) {
+ aio_co_wake(data->co);
+ }
+}
/* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
* new channel for all further (now-encrypted) communication. */
@@ -756,7 +773,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
{
QIOChannel *ioc;
QIOChannelTLS *tioc;
- struct NBDTLSHandshakeData data = { 0 };
+ struct NBDTLSServerHandshakeData data = { 0 };
assert(client->opt == NBD_OPT_STARTTLS);
@@ -777,17 +794,18 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-server-tls");
trace_nbd_negotiate_handle_starttls_handshake();
- data.loop = g_main_loop_new(g_main_context_default(), FALSE);
+ data.co = qemu_coroutine_self();
qio_channel_tls_handshake(tioc,
- nbd_tls_handshake,
+ nbd_server_tls_handshake,
&data,
NULL,
NULL);
if (!data.complete) {
- g_main_loop_run(data.loop);
+ qemu_coroutine_yield();
+ assert(data.complete);
}
- g_main_loop_unref(data.loop);
+
if (data.error) {
object_unref(OBJECT(tioc));
error_propagate(errp, data.error);
--
2.39.2
next prev parent reply other threads:[~2024-05-07 8:43 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 8:41 [Stable-8.2.4 00/16] Patch Round-up for stable 8.2.4 (planned for 2024-05-12) Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 01/16] target/riscv/kvm: change KVM_REG_RISCV_FP_F to u32 Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 02/16] target/riscv/kvm: change KVM_REG_RISCV_FP_D to u64 Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 03/16] target/riscv/kvm: change timer regs size " Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 04/16] migration/colo: Fix bdrv_graph_rdlock_main_loop: Assertion `!qemu_in_coroutine()' failed Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 05/16] linux-user: do_setsockopt: fix SOL_ALG.ALG_SET_KEY Michael Tokarev
2024-05-07 8:42 ` Michael Tokarev [this message]
2024-05-07 8:42 ` [Stable-8.2.4 07/16] nbd/server: Mark negotiation functions as coroutine_fn Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 08/16] backends/cryptodev-builtin: Fix local_error leaks Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 09/16] target/loongarch/cpu.c: typo fix: expection Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 10/16] tests/avocado: update sunxi kernel from armbian to 6.6.16 Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 11/16] .gitlab-ci.d/cirrus.yml: Shorten the runtime of the macOS and FreeBSD jobs Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 12/16] hw/ufs: Fix buffer overflow bug Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 13/16] hw/dmax/xlnx_dpdma: fix handling of address_extension descriptor fields Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 14/16] hw/arm/npcm7xx: Store derivative OTP fuse key in little endian Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 15/16] target/sh4: Fix ADDV opcode Michael Tokarev
2024-05-07 8:42 ` [Stable-8.2.4 16/16] target/sh4: Fix SUBV opcode Michael Tokarev
2024-05-07 8:45 ` [Stable-8.2.4 00/16] Patch Round-up for stable 8.2.4 (planned for 2024-05-10) Michael Tokarev
2024-05-10 15:05 ` Michael Tokarev
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=20240507084226.1026455-6-mjt@tls.msk.ru \
--to=mjt@tls.msk.ru \
--cc=eblake@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-stable@nongnu.org \
--cc=vsementsov@yandex-team.ru \
--cc=zhuyangyang14@huawei.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).