[RFC PATCH v2 0/8] crypto,io,migration: Add support to gnutls_bye()

[RFC PATCH v2 0/8] crypto,io,migration: Add support to gnutls_bye()

[Fabiano Rosas]

v2:

Added the premature_ok logic;
Added compat property for QEMU <9.1;
Refactored the existing handshake code;

CI run:
https://gitlab.com/farosas/qemu/-/pipelines/1660800456

v1:
https://lore.kernel.org/r/20250206175824.22664-1-farosas@suse.de

Hi,

We've been discussing a way to stop multifd recv threads from getting
an error at the end of migration when the source threads close the
iochannel without ending the TLS session.

The original issue was introduced by commit 1d457daf86
("migration/multifd: Further remove the SYNC on complete") which
altered the synchronization of the source and destination in a manner
that causes the destination to already be waiting at recv() when the
source closes the connection.

One approach would be to issue gnutls_bye() at the source after all
the data has been sent. The destination would then gracefully exit
when it gets EOF.

Aside from stopping the recv thread from seeing an error, this also
creates a contract that all connections should be closed only after
the TLS session is ended. This helps to avoid masking a legitimate
issue where the connection is closed prematurely.

Fabiano Rosas (8):
  crypto: Allow gracefully ending the TLS session
  io: tls: Add qio_channel_tls_bye
  migration/multifd: Terminate the TLS connection
  migration: Check migration error after loadvm
  crypto: Remove qcrypto_tls_session_get_handshake_status
  io: Plumb read flags into qio_channel_read_all_eof
  io: Add a read flag for relaxed EOF
  migration/multifd: Add a compat property for TLS termination

 crypto/tlssession.c                 | 105 +++++++++++++++++-----------
 hw/remote/mpqemu-link.c             |   2 +-
 include/crypto/tlssession.h         |  46 ++++++------
 include/io/channel-tls.h            |  12 ++++
 include/io/channel.h                |   7 ++
 io/channel-tls.c                    |  92 +++++++++++++++++++++++-
 io/channel.c                        |  13 ++--
 io/trace-events                     |   5 ++
 migration/migration.h               |  33 +++++++++
 migration/multifd.c                 |  42 ++++++++++-
 migration/multifd.h                 |   2 +
 migration/options.c                 |   2 +
 migration/savevm.c                  |   6 +-
 migration/tls.c                     |   5 ++
 migration/tls.h                     |   2 +-
 tests/unit/test-crypto-tlssession.c |  12 ++--
 tools/i386/qemu-vmsr-helper.c       |   3 +-
 util/vhost-user-server.c            |   2 +-
 18 files changed, 308 insertions(+), 83 deletions(-)

-- 
2.35.3

[RFC PATCH v2 1/8] crypto: Allow gracefully ending the TLS session

[Fabiano Rosas]

QEMU's TLS session code provides no way to call gnutls_bye() to
terminate a TLS session. Callers of qcrypto_tls_session_read() can
choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the
gracefulTermination argument.

The QIOChannelTLS ignores the premature termination error whenever
shutdown() has already been issued. This is not enough anymore for the
migration code due to changes [1] in the synchronization between
migration source and destination.

Add support for calling gnutls_bye() in the tlssession layer so users
of QIOChannelTLS can clearly identify the end of a TLS session.

1- 1d457daf86 ("migration/multifd: Further remove the SYNC on complete")

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 crypto/tlssession.c         | 41 +++++++++++++++++++++++++++++++++++++
 include/crypto/tlssession.h | 22 ++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 77286e23f4..d769d7a304 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -585,6 +585,40 @@ qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *session)
     }
 }
 
+int
+qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp)
+{
+    int ret;
+
+    if (!session->handshakeComplete) {
+        return 0;
+    }
+
+    ret = gnutls_bye(session->handle, GNUTLS_SHUT_WR);
+
+    if (!ret) {
+        return QCRYPTO_TLS_BYE_COMPLETE;
+    }
+
+    if (ret == GNUTLS_E_INTERRUPTED || ret == GNUTLS_E_AGAIN) {
+        int direction = gnutls_record_get_direction(session->handle);
+        return direction ? QCRYPTO_TLS_BYE_SENDING : QCRYPTO_TLS_BYE_RECVING;
+    }
+
+    if (session->rerr || session->werr) {
+        error_setg(errp, "TLS termination failed: %s: %s", gnutls_strerror(ret),
+                   error_get_pretty(session->rerr ?
+                                    session->rerr : session->werr));
+    } else {
+        error_setg(errp, "TLS termination failed: %s", gnutls_strerror(ret));
+    }
+
+    error_free(session->rerr);
+    error_free(session->werr);
+    session->rerr = session->werr = NULL;
+
+    return -1;
+}
 
 int
 qcrypto_tls_session_get_key_size(QCryptoTLSSession *session,
@@ -699,6 +733,13 @@ qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess)
 }
 
 
+int
+qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp)
+{
+    return QCRYPTO_TLS_BYE_COMPLETE;
+}
+
+
 int
 qcrypto_tls_session_get_key_size(QCryptoTLSSession *sess,
                                  Error **errp)
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index f694a5c3c5..c0f64ce989 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -323,6 +323,28 @@ typedef enum {
 QCryptoTLSSessionHandshakeStatus
 qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess);
 
+typedef enum {
+    QCRYPTO_TLS_BYE_COMPLETE,
+    QCRYPTO_TLS_BYE_SENDING,
+    QCRYPTO_TLS_BYE_RECVING,
+} QCryptoTLSSessionByeStatus;
+
+/**
+ * qcrypto_tls_session_bye:
+ * @session: the TLS session object
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Start, or continue, a TLS termination sequence. If the underlying
+ * data channel is non-blocking, then this method may return control
+ * before the termination is complete. The return value will indicate
+ * whether the termination has completed, or is waiting to send or
+ * receive data. In the latter cases, the caller should setup an event
+ * loop watch and call this method again once the underlying data
+ * channel is ready to read or write again.
+ */
+int
+qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp);
+
 /**
  * qcrypto_tls_session_get_key_size:
  * @sess: the TLS session object
-- 
2.35.3

[RFC PATCH v2 2/8] io: tls: Add qio_channel_tls_bye

[Fabiano Rosas]

Add a task dispatcher for gnutls_bye similar to the
qio_channel_tls_handshake_task(). The gnutls_bye() call might be
interrupted and so it needs to be rescheduled.

The migration code will make use of this to help the migration
destination identify a premature EOF. Once the session termination is
in place, any EOF that happens before the source issued gnutls_bye()
will be considered an error.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 include/io/channel-tls.h | 12 ++++++
 io/channel-tls.c         | 84 ++++++++++++++++++++++++++++++++++++++++
 io/trace-events          |  5 +++
 3 files changed, 101 insertions(+)

diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
index 26c67f17e2..7e9023570d 100644
--- a/include/io/channel-tls.h
+++ b/include/io/channel-tls.h
@@ -49,8 +49,20 @@ struct QIOChannelTLS {
     QCryptoTLSSession *session;
     QIOChannelShutdown shutdown;
     guint hs_ioc_tag;
+    guint bye_ioc_tag;
 };
 
+/**
+ * qio_channel_tls_bye:
+ * @ioc: the TLS channel object
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Perform the TLS session termination. This method will return
+ * immediately and the termination will continue in the background,
+ * provided the main loop is running.
+ */
+void qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp);
+
 /**
  * qio_channel_tls_new_server:
  * @master: the underlying channel object
diff --git a/io/channel-tls.c b/io/channel-tls.c
index aab630e5ae..517ce190a4 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -247,6 +247,85 @@ void qio_channel_tls_handshake(QIOChannelTLS *ioc,
     qio_channel_tls_handshake_task(ioc, task, context);
 }
 
+static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition condition,
+                                       gpointer user_data);
+
+static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
+                                     GMainContext *context)
+{
+    GIOCondition condition;
+    QIOChannelTLSData *data;
+    int status;
+    Error *err = NULL;
+
+    status = qcrypto_tls_session_bye(ioc->session, &err);
+
+    if (status < 0) {
+        trace_qio_channel_tls_bye_fail(ioc);
+        qio_task_set_error(task, err);
+        qio_task_complete(task);
+        return;
+    }
+
+    if (status == QCRYPTO_TLS_BYE_COMPLETE) {
+        qio_task_complete(task);
+        return;
+    }
+
+    data = g_new0(typeof(*data), 1);
+    data->task = task;
+    data->context = context;
+
+    if (context) {
+        g_main_context_ref(context);
+    }
+
+    if (status == QCRYPTO_TLS_BYE_SENDING) {
+        condition = G_IO_OUT;
+    } else {
+        condition = G_IO_IN;
+    }
+
+    trace_qio_channel_tls_bye_pending(ioc, status);
+    ioc->bye_ioc_tag = qio_channel_add_watch_full(ioc->master, condition,
+                                                  qio_channel_tls_bye_io,
+                                                  data, NULL, context);
+}
+
+
+static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition condition,
+                                       gpointer user_data)
+{
+    QIOChannelTLSData *data = user_data;
+    QIOTask *task = data->task;
+    GMainContext *context = data->context;
+    QIOChannelTLS *tioc = QIO_CHANNEL_TLS(qio_task_get_source(task));
+
+    tioc->bye_ioc_tag = 0;
+    g_free(data);
+    qio_channel_tls_bye_task(tioc, task, context);
+
+    if (context) {
+        g_main_context_unref(context);
+    }
+
+    return FALSE;
+}
+
+static void propagate_error(QIOTask *task, gpointer opaque)
+{
+    qio_task_propagate_error(task, opaque);
+}
+
+void qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp)
+{
+    QIOTask *task;
+
+    task = qio_task_new(OBJECT(ioc), propagate_error, errp, NULL);
+
+    trace_qio_channel_tls_bye_start(ioc);
+    qio_channel_tls_bye_task(ioc, task, NULL);
+}
 
 static void qio_channel_tls_init(Object *obj G_GNUC_UNUSED)
 {
@@ -379,6 +458,11 @@ static int qio_channel_tls_close(QIOChannel *ioc,
         g_clear_handle_id(&tioc->hs_ioc_tag, g_source_remove);
     }
 
+    if (tioc->bye_ioc_tag) {
+        trace_qio_channel_tls_bye_cancel(ioc);
+        g_clear_handle_id(&tioc->bye_ioc_tag, g_source_remove);
+    }
+
     return qio_channel_close(tioc->master, errp);
 }
 
diff --git a/io/trace-events b/io/trace-events
index d4c0f84a9a..dc3a63ba1f 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -44,6 +44,11 @@ qio_channel_tls_handshake_pending(void *ioc, int status) "TLS handshake pending
 qio_channel_tls_handshake_fail(void *ioc) "TLS handshake fail ioc=%p"
 qio_channel_tls_handshake_complete(void *ioc) "TLS handshake complete ioc=%p"
 qio_channel_tls_handshake_cancel(void *ioc) "TLS handshake cancel ioc=%p"
+qio_channel_tls_bye_start(void *ioc) "TLS termination start ioc=%p"
+qio_channel_tls_bye_pending(void *ioc, int status) "TLS termination pending ioc=%p status=%d"
+qio_channel_tls_bye_fail(void *ioc) "TLS termination fail ioc=%p"
+qio_channel_tls_bye_complete(void *ioc) "TLS termination complete ioc=%p"
+qio_channel_tls_bye_cancel(void *ioc) "TLS termination cancel ioc=%p"
 qio_channel_tls_credentials_allow(void *ioc) "TLS credentials allow ioc=%p"
 qio_channel_tls_credentials_deny(void *ioc) "TLS credentials deny ioc=%p"
 
-- 
2.35.3

[RFC PATCH v2 3/8] migration/multifd: Terminate the TLS connection

[Fabiano Rosas]

The multifd recv side has been getting a TLS error of
GNUTLS_E_PREMATURE_TERMINATION at the end of migration when the send
side closes the sockets without ending the TLS session. This has been
masked by the code not checking the migration error after loadvm.

Start ending the TLS session at multifd_send_shutdown() so the recv
side always sees a clean termination (EOF) and we can start to
differentiate that from an actual premature termination that might
possibly happen in the middle of the migration.

There's nothing to be done if a previous migration error has already
broken the connection, so add a comment explaining it and ignore any
errors coming from gnutls_bye().

This doesn't break compat with older recv-side QEMUs because EOF has
always caused the recv thread to exit cleanly.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 34 +++++++++++++++++++++++++++++++++-
 migration/tls.c     |  5 +++++
 migration/tls.h     |  2 +-
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index ab73d6d984..b57cad3bb1 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -490,6 +490,32 @@ void multifd_send_shutdown(void)
         return;
     }
 
+    for (i = 0; i < migrate_multifd_channels(); i++) {
+        MultiFDSendParams *p = &multifd_send_state->params[i];
+
+        /* thread_created implies the TLS handshake has succeeded */
+        if (p->tls_thread_created && p->thread_created) {
+            Error *local_err = NULL;
+            /*
+             * The destination expects the TLS session to always be
+             * properly terminated. This helps to detect a premature
+             * termination in the middle of the stream.  Note that
+             * older QEMUs always break the connection on the source
+             * and the destination always sees
+             * GNUTLS_E_PREMATURE_TERMINATION.
+             */
+            migration_tls_channel_end(p->c, &local_err);
+
+            if (local_err) {
+                /*
+                 * The above can fail with broken pipe due to a
+                 * previous migration error, ignore the error.
+                 */
+                assert(migration_has_failed(migrate_get_current()));
+            }
+        }
+    }
+
     multifd_send_terminate_threads();
 
     for (i = 0; i < migrate_multifd_channels(); i++) {
@@ -1141,7 +1167,13 @@ static void *multifd_recv_thread(void *opaque)
 
             ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
                                            p->packet_len, &local_err);
-            if (ret == 0 || ret == -1) {   /* 0: EOF  -1: Error */
+            if (!ret) {
+                /* EOF */
+                assert(!local_err);
+                break;
+            }
+
+            if (ret == -1) {
                 break;
             }
 
diff --git a/migration/tls.c b/migration/tls.c
index fa03d9136c..5cbf952383 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -156,6 +156,11 @@ void migration_tls_channel_connect(MigrationState *s,
                               NULL);
 }
 
+void migration_tls_channel_end(QIOChannel *ioc, Error **errp)
+{
+    qio_channel_tls_bye(QIO_CHANNEL_TLS(ioc), errp);
+}
+
 bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc)
 {
     if (!migrate_tls()) {
diff --git a/migration/tls.h b/migration/tls.h
index 5797d153cb..58b25e1228 100644
--- a/migration/tls.h
+++ b/migration/tls.h
@@ -36,7 +36,7 @@ void migration_tls_channel_connect(MigrationState *s,
                                    QIOChannel *ioc,
                                    const char *hostname,
                                    Error **errp);
-
+void migration_tls_channel_end(QIOChannel *ioc, Error **errp);
 /* Whether the QIO channel requires further TLS handshake? */
 bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc);
 
-- 
2.35.3

[RFC PATCH v2 4/8] migration: Check migration error after loadvm

[Fabiano Rosas]

We're currently only checking the QEMUFile error after
qemu_loadvm_state(). Check the migration error as well to avoid
missing errors that might be set by the multifd recv thread.

This doesn't break compat between 9.2 and 10.0 because 9.2 still has
the multifd recv threads stuck at sync when the source channel shuts
down. I.e. it doesn't have commit 1d457daf86 ("migration/multifd:
Further remove the SYNC on complete"). QEMU versions with that commit
will have compat broken with versions containing this commit. This is
not an issue because both will be present in 10.0, but development
trees might see a migration error.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/savevm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index bc375db282..4046faf009 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2940,7 +2940,11 @@ int qemu_loadvm_state(QEMUFile *f)
 
     /* When reaching here, it must be precopy */
     if (ret == 0) {
-        ret = qemu_file_get_error(f);
+        if (migrate_has_error(migrate_get_current())) {
+            ret = -EINVAL;
+        } else {
+            ret = qemu_file_get_error(f);
+        }
     }
 
     /*
-- 
2.35.3

[RFC PATCH v2 5/8] crypto: Remove qcrypto_tls_session_get_handshake_status

[Fabiano Rosas]

The correct way of calling qcrypto_tls_session_handshake() requires
calling qcrypto_tls_session_get_handshake_status() right after it so
there's no reason to have a separate method.

Refactor qcrypto_tls_session_handshake() to inform the status in its
own return value and alter the callers accordingly.

No functional change.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 crypto/tlssession.c                 | 64 +++++++++++------------------
 include/crypto/tlssession.h         | 32 ++++-----------
 io/channel-tls.c                    |  7 ++--
 tests/unit/test-crypto-tlssession.c | 12 ++----
 4 files changed, 39 insertions(+), 76 deletions(-)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index d769d7a304..567698f5d9 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -546,45 +546,35 @@ qcrypto_tls_session_handshake(QCryptoTLSSession *session,
                               Error **errp)
 {
     int ret = gnutls_handshake(session->handle);
-    if (ret == 0) {
+    if (!ret) {
         session->handshakeComplete = true;
-    } else {
-        if (ret == GNUTLS_E_INTERRUPTED ||
-            ret == GNUTLS_E_AGAIN) {
-            ret = 1;
-        } else {
-            if (session->rerr || session->werr) {
-                error_setg(errp, "TLS handshake failed: %s: %s",
-                           gnutls_strerror(ret),
-                           error_get_pretty(session->rerr ?
-                                            session->rerr : session->werr));
-            } else {
-                error_setg(errp, "TLS handshake failed: %s",
-                           gnutls_strerror(ret));
-            }
-            ret = -1;
-        }
-    }
-    error_free(session->rerr);
-    error_free(session->werr);
-    session->rerr = session->werr = NULL;
-
-    return ret;
-}
-
-
-QCryptoTLSSessionHandshakeStatus
-qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *session)
-{
-    if (session->handshakeComplete) {
         return QCRYPTO_TLS_HANDSHAKE_COMPLETE;
-    } else if (gnutls_record_get_direction(session->handle) == 0) {
-        return QCRYPTO_TLS_HANDSHAKE_RECVING;
+    }
+
+    if (ret == GNUTLS_E_INTERRUPTED || ret == GNUTLS_E_AGAIN) {
+        int direction = gnutls_record_get_direction(session->handle);
+        return direction ? QCRYPTO_TLS_HANDSHAKE_SENDING :
+            QCRYPTO_TLS_HANDSHAKE_RECVING;
+    }
+
+    if (session->rerr || session->werr) {
+        error_setg(errp, "TLS handshake failed: %s: %s",
+                   gnutls_strerror(ret),
+                   error_get_pretty(session->rerr ?
+                                    session->rerr : session->werr));
     } else {
-        return QCRYPTO_TLS_HANDSHAKE_SENDING;
+        error_setg(errp, "TLS handshake failed: %s",
+                   gnutls_strerror(ret));
     }
+
+    error_free(session->rerr);
+    error_free(session->werr);
+    session->rerr = session->werr = NULL;
+
+    return -1;
 }
 
+
 int
 qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp)
 {
@@ -720,14 +710,6 @@ qcrypto_tls_session_check_pending(QCryptoTLSSession *session)
 int
 qcrypto_tls_session_handshake(QCryptoTLSSession *sess,
                               Error **errp)
-{
-    error_setg(errp, "TLS requires GNUTLS support");
-    return -1;
-}
-
-
-QCryptoTLSSessionHandshakeStatus
-qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess)
 {
     return QCRYPTO_TLS_HANDSHAKE_COMPLETE;
 }
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index c0f64ce989..d77ae0d423 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -75,12 +75,14 @@
  *                                      GINT_TO_POINTER(fd));
  *
  *    while (1) {
- *       if (qcrypto_tls_session_handshake(sess, errp) < 0) {
+ *       int ret = qcrypto_tls_session_handshake(sess, errp);
+ *
+ *       if (ret < 0) {
  *           qcrypto_tls_session_free(sess);
  *           return -1;
  *       }
  *
- *       switch(qcrypto_tls_session_get_handshake_status(sess)) {
+ *       switch(ret) {
  *       case QCRYPTO_TLS_HANDSHAKE_COMPLETE:
  *           if (qcrypto_tls_session_check_credentials(sess, errp) < )) {
  *               qcrypto_tls_session_free(sess);
@@ -170,7 +172,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoTLSSession, qcrypto_tls_session_free)
  *
  * Validate the peer's credentials after a successful
  * TLS handshake. It is an error to call this before
- * qcrypto_tls_session_get_handshake_status() returns
+ * qcrypto_tls_session_handshake() returns
  * QCRYPTO_TLS_HANDSHAKE_COMPLETE
  *
  * Returns 0 if the credentials validated, -1 on error
@@ -226,7 +228,7 @@ void qcrypto_tls_session_set_callbacks(QCryptoTLSSession *sess,
  * registered with qcrypto_tls_session_set_callbacks()
  *
  * It is an error to call this before
- * qcrypto_tls_session_get_handshake_status() returns
+ * qcrypto_tls_session_handshake() returns
  * QCRYPTO_TLS_HANDSHAKE_COMPLETE
  *
  * Returns: the number of bytes sent,
@@ -256,7 +258,7 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
  * opposed to an error.
  *
  * It is an error to call this before
- * qcrypto_tls_session_get_handshake_status() returns
+ * qcrypto_tls_session_handshake() returns
  * QCRYPTO_TLS_HANDSHAKE_COMPLETE
  *
  * Returns: the number of bytes received,
@@ -289,8 +291,7 @@ size_t qcrypto_tls_session_check_pending(QCryptoTLSSession *sess);
  * the underlying data channel is non-blocking, then
  * this method may return control before the handshake
  * is complete. On non-blocking channels the
- * qcrypto_tls_session_get_handshake_status() method
- * should be used to determine whether the handshake
+ * return value determines whether the handshake
  * has completed, or is waiting to send or receive
  * data. In the latter cases, the caller should setup
  * an event loop watch and call this method again
@@ -306,23 +307,6 @@ typedef enum {
     QCRYPTO_TLS_HANDSHAKE_RECVING,
 } QCryptoTLSSessionHandshakeStatus;
 
-/**
- * qcrypto_tls_session_get_handshake_status:
- * @sess: the TLS session object
- *
- * Check the status of the TLS handshake. This
- * is used with non-blocking data channels to
- * determine whether the handshake is waiting
- * to send or receive further data to/from the
- * remote peer.
- *
- * Once this returns QCRYPTO_TLS_HANDSHAKE_COMPLETE
- * it is permitted to send/receive payload data on
- * the channel
- */
-QCryptoTLSSessionHandshakeStatus
-qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess);
-
 typedef enum {
     QCRYPTO_TLS_BYE_COMPLETE,
     QCRYPTO_TLS_BYE_SENDING,
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 517ce190a4..ecde6b57bf 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -162,16 +162,17 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
                                            GMainContext *context)
 {
     Error *err = NULL;
-    QCryptoTLSSessionHandshakeStatus status;
+    int status;
 
-    if (qcrypto_tls_session_handshake(ioc->session, &err) < 0) {
+    status = qcrypto_tls_session_handshake(ioc->session, &err);
+
+    if (status < 0) {
         trace_qio_channel_tls_handshake_fail(ioc);
         qio_task_set_error(task, err);
         qio_task_complete(task);
         return;
     }
 
-    status = qcrypto_tls_session_get_handshake_status(ioc->session);
     if (status == QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
         trace_qio_channel_tls_handshake_complete(ioc);
         if (qcrypto_tls_session_check_credentials(ioc->session,
diff --git a/tests/unit/test-crypto-tlssession.c b/tests/unit/test-crypto-tlssession.c
index 3395f73560..554054e934 100644
--- a/tests/unit/test-crypto-tlssession.c
+++ b/tests/unit/test-crypto-tlssession.c
@@ -158,8 +158,7 @@ static void test_crypto_tls_session_psk(void)
             rv = qcrypto_tls_session_handshake(serverSess,
                                                &error_abort);
             g_assert(rv >= 0);
-            if (qcrypto_tls_session_get_handshake_status(serverSess) ==
-                QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
+            if (rv == QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
                 serverShake = true;
             }
         }
@@ -167,8 +166,7 @@ static void test_crypto_tls_session_psk(void)
             rv = qcrypto_tls_session_handshake(clientSess,
                                                &error_abort);
             g_assert(rv >= 0);
-            if (qcrypto_tls_session_get_handshake_status(clientSess) ==
-                QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
+            if (rv == QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
                 clientShake = true;
             }
         }
@@ -352,8 +350,7 @@ static void test_crypto_tls_session_x509(const void *opaque)
             rv = qcrypto_tls_session_handshake(serverSess,
                                                &error_abort);
             g_assert(rv >= 0);
-            if (qcrypto_tls_session_get_handshake_status(serverSess) ==
-                QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
+            if (rv == QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
                 serverShake = true;
             }
         }
@@ -361,8 +358,7 @@ static void test_crypto_tls_session_x509(const void *opaque)
             rv = qcrypto_tls_session_handshake(clientSess,
                                                &error_abort);
             g_assert(rv >= 0);
-            if (qcrypto_tls_session_get_handshake_status(clientSess) ==
-                QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
+            if (rv == QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
                 clientShake = true;
             }
         }
-- 
2.35.3

[RFC PATCH v2 6/8] io: Plumb read flags into qio_channel_read_all_eof

[Fabiano Rosas]

We want to pass flags into qio_channel_tls_readv() but some functions
along the way don't take a flags argument. Plumb the flags through.

No functional change.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 hw/remote/mpqemu-link.c       |  2 +-
 include/io/channel.h          |  6 ++++++
 io/channel.c                  | 13 +++++++++----
 migration/multifd.c           |  2 +-
 tools/i386/qemu-vmsr-helper.c |  3 ++-
 util/vhost-user-server.c      |  2 +-
 6 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c
index e25f97680d..49885a1db6 100644
--- a/hw/remote/mpqemu-link.c
+++ b/hw/remote/mpqemu-link.c
@@ -110,7 +110,7 @@ static ssize_t mpqemu_read(QIOChannel *ioc, void *buf, size_t len, int **fds,
         bql_unlock();
     }
 
-    ret = qio_channel_readv_full_all_eof(ioc, &iov, 1, fds, nfds, errp);
+    ret = qio_channel_readv_full_all_eof(ioc, &iov, 1, fds, nfds, 0, errp);
 
     if (drop_bql && !iothread && !qemu_in_coroutine()) {
         bql_lock();
diff --git a/include/io/channel.h b/include/io/channel.h
index bdf0bca92a..6110f0ffe9 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -298,6 +298,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
  * @ioc: the channel object
  * @iov: the array of memory regions to read data into
  * @niov: the length of the @iov array
+ * @flags: read flags (QIO_CHANNEL_READ_FLAG_*)
  * @errp: pointer to a NULL-initialized error object
  *
  * Read data from the IO channel, storing it in the
@@ -321,6 +322,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
 int coroutine_mixed_fn qio_channel_readv_all_eof(QIOChannel *ioc,
                                                  const struct iovec *iov,
                                                  size_t niov,
+                                                 int flags,
                                                  Error **errp);
 
 /**
@@ -442,6 +444,7 @@ ssize_t qio_channel_write(QIOChannel *ioc,
  * @ioc: the channel object
  * @buf: the memory region to read data into
  * @buflen: the number of bytes to @buf
+ * @flags: read flags (QIO_CHANNEL_READ_FLAG_*)
  * @errp: pointer to a NULL-initialized error object
  *
  * Reads @buflen bytes into @buf, possibly blocking or (if the
@@ -457,6 +460,7 @@ ssize_t qio_channel_write(QIOChannel *ioc,
 int coroutine_mixed_fn qio_channel_read_all_eof(QIOChannel *ioc,
                                                 char *buf,
                                                 size_t buflen,
+                                                int flags,
                                                 Error **errp);
 
 /**
@@ -885,6 +889,7 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc,
  * @niov: the length of the @iov array
  * @fds: an array of file handles to read
  * @nfds: number of file handles in @fds
+ * @flags: read flags (QIO_CHANNEL_READ_FLAG_*)
  * @errp: pointer to a NULL-initialized error object
  *
  *
@@ -903,6 +908,7 @@ int coroutine_mixed_fn qio_channel_readv_full_all_eof(QIOChannel *ioc,
                                                       const struct iovec *iov,
                                                       size_t niov,
                                                       int **fds, size_t *nfds,
+                                                      int flags,
                                                       Error **errp);
 
 /**
diff --git a/io/channel.c b/io/channel.c
index e3f17c24a0..61e09202f1 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -113,9 +113,11 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
 int coroutine_mixed_fn qio_channel_readv_all_eof(QIOChannel *ioc,
                                                  const struct iovec *iov,
                                                  size_t niov,
+                                                 int flags,
                                                  Error **errp)
 {
-    return qio_channel_readv_full_all_eof(ioc, iov, niov, NULL, NULL, errp);
+    return qio_channel_readv_full_all_eof(ioc, iov, niov, NULL, NULL, flags,
+                                          errp);
 }
 
 int coroutine_mixed_fn qio_channel_readv_all(QIOChannel *ioc,
@@ -130,6 +132,7 @@ int coroutine_mixed_fn qio_channel_readv_full_all_eof(QIOChannel *ioc,
                                                       const struct iovec *iov,
                                                       size_t niov,
                                                       int **fds, size_t *nfds,
+                                                      int flags,
                                                       Error **errp)
 {
     int ret = -1;
@@ -155,7 +158,7 @@ int coroutine_mixed_fn qio_channel_readv_full_all_eof(QIOChannel *ioc,
     while ((nlocal_iov > 0) || local_fds) {
         ssize_t len;
         len = qio_channel_readv_full(ioc, local_iov, nlocal_iov, local_fds,
-                                     local_nfds, 0, errp);
+                                     local_nfds, flags, errp);
         if (len == QIO_CHANNEL_ERR_BLOCK) {
             if (qemu_in_coroutine()) {
                 qio_channel_yield(ioc, G_IO_IN);
@@ -222,7 +225,8 @@ int coroutine_mixed_fn qio_channel_readv_full_all(QIOChannel *ioc,
                                                   int **fds, size_t *nfds,
                                                   Error **errp)
 {
-    int ret = qio_channel_readv_full_all_eof(ioc, iov, niov, fds, nfds, errp);
+    int ret = qio_channel_readv_full_all_eof(ioc, iov, niov, fds, nfds, 0,
+                                             errp);
 
     if (ret == 0) {
         error_setg(errp, "Unexpected end-of-file before all data were read");
@@ -329,10 +333,11 @@ ssize_t qio_channel_write(QIOChannel *ioc,
 int coroutine_mixed_fn qio_channel_read_all_eof(QIOChannel *ioc,
                                                 char *buf,
                                                 size_t buflen,
+                                                int flags,
                                                 Error **errp)
 {
     struct iovec iov = { .iov_base = buf, .iov_len = buflen };
-    return qio_channel_readv_all_eof(ioc, &iov, 1, errp);
+    return qio_channel_readv_all_eof(ioc, &iov, 1, flags, errp);
 }
 
 
diff --git a/migration/multifd.c b/migration/multifd.c
index b57cad3bb1..b4f82b0893 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1166,7 +1166,7 @@ static void *multifd_recv_thread(void *opaque)
             }
 
             ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
-                                           p->packet_len, &local_err);
+                                           p->packet_len, 0, &local_err);
             if (!ret) {
                 /* EOF */
                 assert(!local_err);
diff --git a/tools/i386/qemu-vmsr-helper.c b/tools/i386/qemu-vmsr-helper.c
index a35dcb88a3..2a9f1825b7 100644
--- a/tools/i386/qemu-vmsr-helper.c
+++ b/tools/i386/qemu-vmsr-helper.c
@@ -237,7 +237,8 @@ static void coroutine_fn vh_co_entry(void *opaque)
          * Only RAPL MSR in rapl-msr-index.h is allowed
          */
         r = qio_channel_read_all_eof(QIO_CHANNEL(client->ioc),
-                                     (char *) &request, sizeof(request), &local_err);
+                                     (char *) &request, sizeof(request), 0,
+                                     &local_err);
         if (r <= 0) {
             break;
         }
diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index b19229074a..7006328b2e 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -190,7 +190,7 @@ vu_message_read(VuDev *vu_dev, int conn_fd, VhostUserMsg *vmsg)
         .iov_len = vmsg->size,
     };
     if (vmsg->size) {
-        rc = qio_channel_readv_all_eof(ioc, &iov_payload, 1, &local_err);
+        rc = qio_channel_readv_all_eof(ioc, &iov_payload, 1, 0, &local_err);
         if (rc != 1) {
             if (local_err) {
                 error_report_err(local_err);
-- 
2.35.3

[RFC PATCH v2 7/8] io: Add a read flag for relaxed EOF

[Fabiano Rosas]

Add a read flag that can inform a channel that it's ok to receive an
EOF at any moment. Channels that have some form of strict EOF
tracking, such as TLS session termination, may choose to ignore EOF
errors with the use of this flag.

This is being added for compatibility with older migration streams
that do not include a TLS termination step.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 include/io/channel.h | 1 +
 io/channel-tls.c     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/io/channel.h b/include/io/channel.h
index 6110f0ffe9..55d70fb853 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -35,6 +35,7 @@ OBJECT_DECLARE_TYPE(QIOChannel, QIOChannelClass,
 #define QIO_CHANNEL_WRITE_FLAG_ZERO_COPY 0x1
 
 #define QIO_CHANNEL_READ_FLAG_MSG_PEEK 0x1
+#define QIO_CHANNEL_READ_FLAG_RELAXED_EOF 0x2
 
 typedef enum QIOChannelFeature QIOChannelFeature;
 
diff --git a/io/channel-tls.c b/io/channel-tls.c
index ecde6b57bf..caf8301a9e 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -359,6 +359,7 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
             tioc->session,
             iov[i].iov_base,
             iov[i].iov_len,
+            flags & QIO_CHANNEL_READ_FLAG_RELAXED_EOF ||
             qatomic_load_acquire(&tioc->shutdown) & QIO_CHANNEL_SHUTDOWN_READ,
             errp);
         if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
-- 
2.35.3

[RFC PATCH v2 8/8] migration/multifd: Add a compat property for TLS termination

[Fabiano Rosas]

We're currently changing the way the source multifd migration handles
the shutdown of the multifd channels when TLS is in use to perform a
clean termination by calling gnutls_bye().

Older src QEMUs will always close the channel without terminating the
TLS session. New dst QEMUs treat an unclean termination as an
error. Due to synchronization conditions, src QEMUs 9.1 and 9.2 are an
exception and can put the destination in a condition where it ignores
the unclean termination. For src QEMUs older than 9.1, we'll need a
compat property on the destination to inform that the src does not
terminate the TLS session.

Add multifd_clean_tls_termination (default true) that can be switched
on the destination whenever a src QEMU <9.1 is in use.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.h | 33 +++++++++++++++++++++++++++++++++
 migration/multifd.c   |  8 +++++++-
 migration/multifd.h   |  2 ++
 migration/options.c   |  2 ++
 4 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/migration/migration.h b/migration/migration.h
index 4c1fafc2b5..77def0b437 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -443,6 +443,39 @@ struct MigrationState {
      * Default value is false. (since 8.1)
      */
     bool multifd_flush_after_each_section;
+
+    /*
+     * This variable only makes sense when set on the machine that is
+     * the destination of a multifd migration with TLS enabled. It
+     * affects the behavior of the last send->recv iteration with
+     * regards to termination of the TLS session.
+     *
+     * When set:
+     *
+     * - the destination QEMU instance can expect to never get a
+     *   GNUTLS_E_PREMATURE_TERMINATION error. Manifested as the error
+     *   message: "The TLS connection was non-properly terminated".
+     *
+     * When clear:
+     *
+     * - the destination QEMU instance can expect to see a
+     *   GNUTLS_E_PREMATURE_TERMINATION error in any multifd channel
+     *   whenever the last recv() call of that channel happens after
+     *   the source QEMU instance has already issued shutdown() on the
+     *   channel.
+     *
+     *   Commit 637280aeb2 (since 9.1) introduced a side effect that
+     *   causes the destination instance to not be affected by the
+     *   premature termination, while commit 1d457daf86 (since 10.0)
+     *   causes the premature termination condition to be once again
+     *   reachable.
+     *
+     * NOTE: Regardless of the state of this option, a premature
+     * termination of the TLS connection might happen due to error at
+     * any moment prior to the last send->recv iteration.
+     */
+    bool multifd_clean_tls_termination;
+
     /*
      * This decides the size of guest memory chunk that will be used
      * to track dirty bitmap clearing.  The size of memory chunk will
diff --git a/migration/multifd.c b/migration/multifd.c
index b4f82b0893..4342399818 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1147,6 +1147,7 @@ void multifd_recv_sync_main(void)
 
 static void *multifd_recv_thread(void *opaque)
 {
+    MigrationState *s = migrate_get_current();
     MultiFDRecvParams *p = opaque;
     Error *local_err = NULL;
     bool use_packets = multifd_use_packets();
@@ -1155,6 +1156,10 @@ static void *multifd_recv_thread(void *opaque)
     trace_multifd_recv_thread_start(p->id);
     rcu_register_thread();
 
+    if (!s->multifd_clean_tls_termination) {
+        p->read_flags = QIO_CHANNEL_READ_FLAG_RELAXED_EOF;
+    }
+
     while (true) {
         uint32_t flags = 0;
         bool has_data = false;
@@ -1166,7 +1171,8 @@ static void *multifd_recv_thread(void *opaque)
             }
 
             ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
-                                           p->packet_len, 0, &local_err);
+                                           p->packet_len, p->read_flags,
+                                           &local_err);
             if (!ret) {
                 /* EOF */
                 assert(!local_err);
diff --git a/migration/multifd.h b/migration/multifd.h
index bd785b9873..cf408ff721 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -244,6 +244,8 @@ typedef struct {
     uint32_t zero_num;
     /* used for de-compression methods */
     void *compress_data;
+    /* Flags for the QIOChannel */
+    int read_flags;
 } MultiFDRecvParams;
 
 typedef struct {
diff --git a/migration/options.c b/migration/options.c
index 1ad950e397..feda354935 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -99,6 +99,8 @@ const Property migration_properties[] = {
                       clear_bitmap_shift, CLEAR_BITMAP_SHIFT_DEFAULT),
     DEFINE_PROP_BOOL("x-preempt-pre-7-2", MigrationState,
                      preempt_pre_7_2, false),
+    DEFINE_PROP_BOOL("multifd-clean-tls-termination", MigrationState,
+                     multifd_clean_tls_termination, true),
 
     /* Migration parameters */
     DEFINE_PROP_UINT8("x-throttle-trigger-threshold", MigrationState,
-- 
2.35.3

Re: [RFC PATCH v2 1/8] crypto: Allow gracefully ending the TLS session

[Daniel P. Berrangé]

On Fri, Feb 07, 2025 at 11:27:51AM -0300, Fabiano Rosas wrote:
> QEMU's TLS session code provides no way to call gnutls_bye() to
> terminate a TLS session. Callers of qcrypto_tls_session_read() can
> choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the
> gracefulTermination argument.
> 
> The QIOChannelTLS ignores the premature termination error whenever
> shutdown() has already been issued. This is not enough anymore for the
> migration code due to changes [1] in the synchronization between
> migration source and destination.
> 
> Add support for calling gnutls_bye() in the tlssession layer so users
> of QIOChannelTLS can clearly identify the end of a TLS session.
> 
> 1- 1d457daf86 ("migration/multifd: Further remove the SYNC on complete")

Would be slightly clearer as "[1] 1d457daf86 ...."

> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  crypto/tlssession.c         | 41 +++++++++++++++++++++++++++++++++++++
>  include/crypto/tlssession.h | 22 ++++++++++++++++++++
>  2 files changed, 63 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Acked-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v2 2/8] io: tls: Add qio_channel_tls_bye

[Daniel P. Berrangé]

On Fri, Feb 07, 2025 at 11:27:52AM -0300, Fabiano Rosas wrote:
> Add a task dispatcher for gnutls_bye similar to the
> qio_channel_tls_handshake_task(). The gnutls_bye() call might be
> interrupted and so it needs to be rescheduled.
> 
> The migration code will make use of this to help the migration
> destination identify a premature EOF. Once the session termination is
> in place, any EOF that happens before the source issued gnutls_bye()
> will be considered an error.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  include/io/channel-tls.h | 12 ++++++
>  io/channel-tls.c         | 84 ++++++++++++++++++++++++++++++++++++++++
>  io/trace-events          |  5 +++
>  3 files changed, 101 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Acked-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v2 5/8] crypto: Remove qcrypto_tls_session_get_handshake_status

[Daniel P. Berrangé]

On Fri, Feb 07, 2025 at 11:27:55AM -0300, Fabiano Rosas wrote:
> The correct way of calling qcrypto_tls_session_handshake() requires
> calling qcrypto_tls_session_get_handshake_status() right after it so
> there's no reason to have a separate method.
> 
> Refactor qcrypto_tls_session_handshake() to inform the status in its
> own return value and alter the callers accordingly.
> 
> No functional change.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  crypto/tlssession.c                 | 64 +++++++++++------------------
>  include/crypto/tlssession.h         | 32 ++++-----------
>  io/channel-tls.c                    |  7 ++--
>  tests/unit/test-crypto-tlssession.c | 12 ++----
>  4 files changed, 39 insertions(+), 76 deletions(-)
> 
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> @@ -720,14 +710,6 @@ qcrypto_tls_session_check_pending(QCryptoTLSSession *session)
>  int
>  qcrypto_tls_session_handshake(QCryptoTLSSession *sess,
>                                Error **errp)
> -{
> -    error_setg(errp, "TLS requires GNUTLS support");
> -    return -1;
> -}
> -

This codepath is the !GNUTLS branch, so we need to continue
reporting an error here, not return QCRYPTO_TLS_HANDSHAKE_COMPLETE.

> -
> -QCryptoTLSSessionHandshakeStatus
> -qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess)
>  {
>      return QCRYPTO_TLS_HANDSHAKE_COMPLETE;
>  }


With that small change made

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Acked-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v2 6/8] io: Plumb read flags into qio_channel_read_all_eof

[Daniel P. Berrangé]

On Fri, Feb 07, 2025 at 11:27:56AM -0300, Fabiano Rosas wrote:
> We want to pass flags into qio_channel_tls_readv() but some functions
> along the way don't take a flags argument. Plumb the flags through.
> 
> No functional change.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---

> diff --git a/include/io/channel.h b/include/io/channel.h
> index bdf0bca92a..6110f0ffe9 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -298,6 +298,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
>   * @ioc: the channel object
>   * @iov: the array of memory regions to read data into
>   * @niov: the length of the @iov array
> + * @flags: read flags (QIO_CHANNEL_READ_FLAG_*)
>   * @errp: pointer to a NULL-initialized error object
>   *
>   * Read data from the IO channel, storing it in the
> @@ -321,6 +322,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
>  int coroutine_mixed_fn qio_channel_readv_all_eof(QIOChannel *ioc,
>                                                   const struct iovec *iov,
>                                                   size_t niov,
> +                                                 int flags,
>                                                   Error **errp);
>  
>  /**
> @@ -442,6 +444,7 @@ ssize_t qio_channel_write(QIOChannel *ioc,
>   * @ioc: the channel object
>   * @buf: the memory region to read data into
>   * @buflen: the number of bytes to @buf
> + * @flags: read flags (QIO_CHANNEL_READ_FLAG_*)
>   * @errp: pointer to a NULL-initialized error object
>   *
>   * Reads @buflen bytes into @buf, possibly blocking or (if the
> @@ -457,6 +460,7 @@ ssize_t qio_channel_write(QIOChannel *ioc,
>  int coroutine_mixed_fn qio_channel_read_all_eof(QIOChannel *ioc,
>                                                  char *buf,
>                                                  size_t buflen,
> +                                                int flags,
>                                                  Error **errp);
>

The 'int flags' parameter is intended to only be added to the
"_full" method variants since it is niche usage, so these two
shouldn't be changed.

>  /**
> @@ -885,6 +889,7 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc,
>   * @niov: the length of the @iov array
>   * @fds: an array of file handles to read
>   * @nfds: number of file handles in @fds
> + * @flags: read flags (QIO_CHANNEL_READ_FLAG_*)
>   * @errp: pointer to a NULL-initialized error object
>   *
>   *
> @@ -903,6 +908,7 @@ int coroutine_mixed_fn qio_channel_readv_full_all_eof(QIOChannel *ioc,
>                                                        const struct iovec *iov,
>                                                        size_t niov,
>                                                        int **fds, size_t *nfds,
> +                                                      int flags,
>                                                        Error **errp);

This is ok, and migration code should switch to calling this
method instead of qio_channel_readv_all_eof to make use of flags.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v2 7/8] io: Add a read flag for relaxed EOF

[Daniel P. Berrangé]

On Fri, Feb 07, 2025 at 11:27:57AM -0300, Fabiano Rosas wrote:
> Add a read flag that can inform a channel that it's ok to receive an
> EOF at any moment. Channels that have some form of strict EOF
> tracking, such as TLS session termination, may choose to ignore EOF
> errors with the use of this flag.
> 
> This is being added for compatibility with older migration streams
> that do not include a TLS termination step.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  include/io/channel.h | 1 +
>  io/channel-tls.c     | 1 +
>  2 files changed, 2 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v2 1/8] crypto: Allow gracefully ending the TLS session

[Peter Xu]

On Fri, Feb 07, 2025 at 11:27:51AM -0300, Fabiano Rosas wrote:
> QEMU's TLS session code provides no way to call gnutls_bye() to
> terminate a TLS session. Callers of qcrypto_tls_session_read() can
> choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the
> gracefulTermination argument.
> 
> The QIOChannelTLS ignores the premature termination error whenever
> shutdown() has already been issued. This is not enough anymore for the
> migration code due to changes [1] in the synchronization between
> migration source and destination.

This sentence seems to say commit [1] changed something on the tls
condition, but IMHO fundamentally the issue is multifd recv thread model
that relies on blocking readv() rather than request-based (like what src
multifd does).

Now src uses either shutdown() or close() to kick dest multifd recv threads
out from readv().  That has nothing to do with what we do during complete()
with those sync messages.. referencing it is ok, but we'll need to
reference also the other commit to be clear pre-9.0 can also be prone to
this.  To me, it's more important to mention the root cause on the multifd
recv thread model, which requires explicit tls terminations.

> 
> Add support for calling gnutls_bye() in the tlssession layer so users
> of QIOChannelTLS can clearly identify the end of a TLS session.
> 
> 1- 1d457daf86 ("migration/multifd: Further remove the SYNC on complete")
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

-- 
Peter Xu

Re: [RFC PATCH v2 1/8] crypto: Allow gracefully ending the TLS session

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Feb 07, 2025 at 11:27:51AM -0300, Fabiano Rosas wrote:
>> QEMU's TLS session code provides no way to call gnutls_bye() to
>> terminate a TLS session. Callers of qcrypto_tls_session_read() can
>> choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the
>> gracefulTermination argument.
>> 
>> The QIOChannelTLS ignores the premature termination error whenever
>> shutdown() has already been issued. This is not enough anymore for the
>> migration code due to changes [1] in the synchronization between
>> migration source and destination.
>
> This sentence seems to say commit [1] changed something on the tls
> condition, but IMHO fundamentally the issue is multifd recv thread model
> that relies on blocking readv() rather than request-based (like what src
> multifd does).
>
> Now src uses either shutdown() or close() to kick dest multifd recv threads
> out from readv().  That has nothing to do with what we do during complete()
> with those sync messages.. referencing it is ok, but we'll need to
> reference also the other commit to be clear pre-9.0 can also be prone to
> this.  To me, it's more important to mention the root cause on the multifd
> recv thread model, which requires explicit tls terminations.
>

I didn't want to go into too much detail in a commit for crypto/. The
motivation for *this* patch is just: migration needs it. What about:

 The QIOChannelTLS ignores the premature termination error whenever
 shutdown() has already been issued. This was found to be not enough for
 the migration code because shutdown() might not have been issued before
 the connection is terminated.


>> 
>> Add support for calling gnutls_bye() in the tlssession layer so users
>> of QIOChannelTLS can clearly identify the end of a TLS session.
>> 
>> 1- 1d457daf86 ("migration/multifd: Further remove the SYNC on complete")
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Re: [RFC PATCH v2 3/8] migration/multifd: Terminate the TLS connection

[Peter Xu]

On Fri, Feb 07, 2025 at 11:27:53AM -0300, Fabiano Rosas wrote:
> The multifd recv side has been getting a TLS error of
> GNUTLS_E_PREMATURE_TERMINATION at the end of migration when the send
> side closes the sockets without ending the TLS session. This has been
> masked by the code not checking the migration error after loadvm.
> 
> Start ending the TLS session at multifd_send_shutdown() so the recv
> side always sees a clean termination (EOF) and we can start to
> differentiate that from an actual premature termination that might
> possibly happen in the middle of the migration.
> 
> There's nothing to be done if a previous migration error has already
> broken the connection, so add a comment explaining it and ignore any
> errors coming from gnutls_bye().
> 
> This doesn't break compat with older recv-side QEMUs because EOF has
> always caused the recv thread to exit cleanly.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

One trivial comment..

> ---
>  migration/multifd.c | 34 +++++++++++++++++++++++++++++++++-
>  migration/tls.c     |  5 +++++
>  migration/tls.h     |  2 +-
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index ab73d6d984..b57cad3bb1 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -490,6 +490,32 @@ void multifd_send_shutdown(void)
>          return;
>      }
>  
> +    for (i = 0; i < migrate_multifd_channels(); i++) {
> +        MultiFDSendParams *p = &multifd_send_state->params[i];
> +
> +        /* thread_created implies the TLS handshake has succeeded */
> +        if (p->tls_thread_created && p->thread_created) {
> +            Error *local_err = NULL;
> +            /*
> +             * The destination expects the TLS session to always be
> +             * properly terminated. This helps to detect a premature
> +             * termination in the middle of the stream.  Note that
> +             * older QEMUs always break the connection on the source
> +             * and the destination always sees
> +             * GNUTLS_E_PREMATURE_TERMINATION.
> +             */
> +            migration_tls_channel_end(p->c, &local_err);
> +
> +            if (local_err) {
> +                /*
> +                 * The above can fail with broken pipe due to a
> +                 * previous migration error, ignore the error.
> +                 */
> +                assert(migration_has_failed(migrate_get_current()));

Considering this is still src, do we want to be softer on this by
error_report?

Logically !migration_has_failed() means it succeeded, so we can throw src
qemu way now, that shouldn't be a huge deal. More of thinking out loud kind
of comment..  Your call.

> +            }
> +        }
> +    }
> +
>      multifd_send_terminate_threads();
>  
>      for (i = 0; i < migrate_multifd_channels(); i++) {
> @@ -1141,7 +1167,13 @@ static void *multifd_recv_thread(void *opaque)
>  
>              ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
>                                             p->packet_len, &local_err);
> -            if (ret == 0 || ret == -1) {   /* 0: EOF  -1: Error */
> +            if (!ret) {
> +                /* EOF */
> +                assert(!local_err);
> +                break;
> +            }
> +
> +            if (ret == -1) {
>                  break;
>              }
>  
> diff --git a/migration/tls.c b/migration/tls.c
> index fa03d9136c..5cbf952383 100644
> --- a/migration/tls.c
> +++ b/migration/tls.c
> @@ -156,6 +156,11 @@ void migration_tls_channel_connect(MigrationState *s,
>                                NULL);
>  }
>  
> +void migration_tls_channel_end(QIOChannel *ioc, Error **errp)
> +{
> +    qio_channel_tls_bye(QIO_CHANNEL_TLS(ioc), errp);
> +}
> +
>  bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc)
>  {
>      if (!migrate_tls()) {
> diff --git a/migration/tls.h b/migration/tls.h
> index 5797d153cb..58b25e1228 100644
> --- a/migration/tls.h
> +++ b/migration/tls.h
> @@ -36,7 +36,7 @@ void migration_tls_channel_connect(MigrationState *s,
>                                     QIOChannel *ioc,
>                                     const char *hostname,
>                                     Error **errp);
> -
> +void migration_tls_channel_end(QIOChannel *ioc, Error **errp);
>  /* Whether the QIO channel requires further TLS handshake? */
>  bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc);
>  
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [RFC PATCH v2 4/8] migration: Check migration error after loadvm

[Peter Xu]

On Fri, Feb 07, 2025 at 11:27:54AM -0300, Fabiano Rosas wrote:
> We're currently only checking the QEMUFile error after
> qemu_loadvm_state(). Check the migration error as well to avoid
> missing errors that might be set by the multifd recv thread.
> 
> This doesn't break compat between 9.2 and 10.0 because 9.2 still has
> the multifd recv threads stuck at sync when the source channel shuts
> down. I.e. it doesn't have commit 1d457daf86 ("migration/multifd:
> Further remove the SYNC on complete"). QEMU versions with that commit
> will have compat broken with versions containing this commit. This is
> not an issue because both will be present in 10.0, but development
> trees might see a migration error.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/savevm.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index bc375db282..4046faf009 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2940,7 +2940,11 @@ int qemu_loadvm_state(QEMUFile *f)
>  
>      /* When reaching here, it must be precopy */
>      if (ret == 0) {
> -        ret = qemu_file_get_error(f);
> +        if (migrate_has_error(migrate_get_current())) {
> +            ret = -EINVAL;
> +        } else {
> +            ret = qemu_file_get_error(f);
> +        }
>      }

IIUC this one needs to be after the patch that allows pre-mature
terminations from old qemus?

-- 
Peter Xu

Re: [RFC PATCH v2 8/8] migration/multifd: Add a compat property for TLS termination

[Peter Xu]

On Fri, Feb 07, 2025 at 11:27:58AM -0300, Fabiano Rosas wrote:
> We're currently changing the way the source multifd migration handles
> the shutdown of the multifd channels when TLS is in use to perform a
> clean termination by calling gnutls_bye().
> 
> Older src QEMUs will always close the channel without terminating the
> TLS session. New dst QEMUs treat an unclean termination as an
> error. Due to synchronization conditions, src QEMUs 9.1 and 9.2 are an
> exception and can put the destination in a condition where it ignores
> the unclean termination. For src QEMUs older than 9.1, we'll need a
> compat property on the destination to inform that the src does not
> terminate the TLS session.
> 
> Add multifd_clean_tls_termination (default true) that can be switched
> on the destination whenever a src QEMU <9.1 is in use.

Patch looks good.  Though did you forget to add the compat entry?

I suggest we add it for all pre-9.2, in case whoever backports the recent
changes so it re-exposes again in any distro stables.

IMHO it doesn't hurt us much to be always cautious on 9.1 and 9.2 too by
loosing the termination a bit.

> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.h | 33 +++++++++++++++++++++++++++++++++
>  migration/multifd.c   |  8 +++++++-
>  migration/multifd.h   |  2 ++
>  migration/options.c   |  2 ++
>  4 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index 4c1fafc2b5..77def0b437 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -443,6 +443,39 @@ struct MigrationState {
>       * Default value is false. (since 8.1)
>       */
>      bool multifd_flush_after_each_section;
> +
> +    /*
> +     * This variable only makes sense when set on the machine that is
> +     * the destination of a multifd migration with TLS enabled. It
> +     * affects the behavior of the last send->recv iteration with
> +     * regards to termination of the TLS session.
> +     *
> +     * When set:
> +     *
> +     * - the destination QEMU instance can expect to never get a
> +     *   GNUTLS_E_PREMATURE_TERMINATION error. Manifested as the error
> +     *   message: "The TLS connection was non-properly terminated".
> +     *
> +     * When clear:
> +     *
> +     * - the destination QEMU instance can expect to see a
> +     *   GNUTLS_E_PREMATURE_TERMINATION error in any multifd channel
> +     *   whenever the last recv() call of that channel happens after
> +     *   the source QEMU instance has already issued shutdown() on the
> +     *   channel.
> +     *
> +     *   Commit 637280aeb2 (since 9.1) introduced a side effect that
> +     *   causes the destination instance to not be affected by the
> +     *   premature termination, while commit 1d457daf86 (since 10.0)
> +     *   causes the premature termination condition to be once again
> +     *   reachable.
> +     *
> +     * NOTE: Regardless of the state of this option, a premature
> +     * termination of the TLS connection might happen due to error at
> +     * any moment prior to the last send->recv iteration.
> +     */
> +    bool multifd_clean_tls_termination;
> +
>      /*
>       * This decides the size of guest memory chunk that will be used
>       * to track dirty bitmap clearing.  The size of memory chunk will
> diff --git a/migration/multifd.c b/migration/multifd.c
> index b4f82b0893..4342399818 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -1147,6 +1147,7 @@ void multifd_recv_sync_main(void)
>  
>  static void *multifd_recv_thread(void *opaque)
>  {
> +    MigrationState *s = migrate_get_current();
>      MultiFDRecvParams *p = opaque;
>      Error *local_err = NULL;
>      bool use_packets = multifd_use_packets();
> @@ -1155,6 +1156,10 @@ static void *multifd_recv_thread(void *opaque)
>      trace_multifd_recv_thread_start(p->id);
>      rcu_register_thread();
>  
> +    if (!s->multifd_clean_tls_termination) {
> +        p->read_flags = QIO_CHANNEL_READ_FLAG_RELAXED_EOF;
> +    }
> +
>      while (true) {
>          uint32_t flags = 0;
>          bool has_data = false;
> @@ -1166,7 +1171,8 @@ static void *multifd_recv_thread(void *opaque)
>              }
>  
>              ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
> -                                           p->packet_len, 0, &local_err);
> +                                           p->packet_len, p->read_flags,
> +                                           &local_err);
>              if (!ret) {
>                  /* EOF */
>                  assert(!local_err);
> diff --git a/migration/multifd.h b/migration/multifd.h
> index bd785b9873..cf408ff721 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -244,6 +244,8 @@ typedef struct {
>      uint32_t zero_num;
>      /* used for de-compression methods */
>      void *compress_data;
> +    /* Flags for the QIOChannel */
> +    int read_flags;
>  } MultiFDRecvParams;
>  
>  typedef struct {
> diff --git a/migration/options.c b/migration/options.c
> index 1ad950e397..feda354935 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -99,6 +99,8 @@ const Property migration_properties[] = {
>                        clear_bitmap_shift, CLEAR_BITMAP_SHIFT_DEFAULT),
>      DEFINE_PROP_BOOL("x-preempt-pre-7-2", MigrationState,
>                       preempt_pre_7_2, false),
> +    DEFINE_PROP_BOOL("multifd-clean-tls-termination", MigrationState,
> +                     multifd_clean_tls_termination, true),
>  
>      /* Migration parameters */
>      DEFINE_PROP_UINT8("x-throttle-trigger-threshold", MigrationState,
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [RFC PATCH v2 1/8] crypto: Allow gracefully ending the TLS session

[Peter Xu]

On Fri, Feb 07, 2025 at 02:55:57PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Fri, Feb 07, 2025 at 11:27:51AM -0300, Fabiano Rosas wrote:
> >> QEMU's TLS session code provides no way to call gnutls_bye() to
> >> terminate a TLS session. Callers of qcrypto_tls_session_read() can
> >> choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the
> >> gracefulTermination argument.
> >> 
> >> The QIOChannelTLS ignores the premature termination error whenever
> >> shutdown() has already been issued. This is not enough anymore for the
> >> migration code due to changes [1] in the synchronization between
> >> migration source and destination.
> >
> > This sentence seems to say commit [1] changed something on the tls
> > condition, but IMHO fundamentally the issue is multifd recv thread model
> > that relies on blocking readv() rather than request-based (like what src
> > multifd does).
> >
> > Now src uses either shutdown() or close() to kick dest multifd recv threads
> > out from readv().  That has nothing to do with what we do during complete()
> > with those sync messages.. referencing it is ok, but we'll need to
> > reference also the other commit to be clear pre-9.0 can also be prone to
> > this.  To me, it's more important to mention the root cause on the multifd
> > recv thread model, which requires explicit tls terminations.
> >
> 
> I didn't want to go into too much detail in a commit for crypto/. The

You already did so by referencing a multifd commit that changes how
complete() works!

> motivation for *this* patch is just: migration needs it. What about:
> 
>  The QIOChannelTLS ignores the premature termination error whenever
>  shutdown() has already been issued. This was found to be not enough for
>  the migration code because shutdown() might not have been issued before
>  the connection is terminated.

Looks good to me, thanks.

-- 
Peter Xu

Re: [RFC PATCH v2 3/8] migration/multifd: Terminate the TLS connection

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Feb 07, 2025 at 11:27:53AM -0300, Fabiano Rosas wrote:
>> The multifd recv side has been getting a TLS error of
>> GNUTLS_E_PREMATURE_TERMINATION at the end of migration when the send
>> side closes the sockets without ending the TLS session. This has been
>> masked by the code not checking the migration error after loadvm.
>> 
>> Start ending the TLS session at multifd_send_shutdown() so the recv
>> side always sees a clean termination (EOF) and we can start to
>> differentiate that from an actual premature termination that might
>> possibly happen in the middle of the migration.
>> 
>> There's nothing to be done if a previous migration error has already
>> broken the connection, so add a comment explaining it and ignore any
>> errors coming from gnutls_bye().
>> 
>> This doesn't break compat with older recv-side QEMUs because EOF has
>> always caused the recv thread to exit cleanly.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> One trivial comment..
>
>> ---
>>  migration/multifd.c | 34 +++++++++++++++++++++++++++++++++-
>>  migration/tls.c     |  5 +++++
>>  migration/tls.h     |  2 +-
>>  3 files changed, 39 insertions(+), 2 deletions(-)
>> 
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index ab73d6d984..b57cad3bb1 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -490,6 +490,32 @@ void multifd_send_shutdown(void)
>>          return;
>>      }
>>  
>> +    for (i = 0; i < migrate_multifd_channels(); i++) {
>> +        MultiFDSendParams *p = &multifd_send_state->params[i];
>> +
>> +        /* thread_created implies the TLS handshake has succeeded */
>> +        if (p->tls_thread_created && p->thread_created) {
>> +            Error *local_err = NULL;
>> +            /*
>> +             * The destination expects the TLS session to always be
>> +             * properly terminated. This helps to detect a premature
>> +             * termination in the middle of the stream.  Note that
>> +             * older QEMUs always break the connection on the source
>> +             * and the destination always sees
>> +             * GNUTLS_E_PREMATURE_TERMINATION.
>> +             */
>> +            migration_tls_channel_end(p->c, &local_err);
>> +
>> +            if (local_err) {
>> +                /*
>> +                 * The above can fail with broken pipe due to a
>> +                 * previous migration error, ignore the error.
>> +                 */
>> +                assert(migration_has_failed(migrate_get_current()));
>
> Considering this is still src, do we want to be softer on this by
> error_report?
>
> Logically !migration_has_failed() means it succeeded, so we can throw src
> qemu way now, that shouldn't be a huge deal. More of thinking out loud kind
> of comment..  Your call.
>

Maybe even a warning? If at this point migration succeeded, it's probably
best to let cleanup carry on.

>> +            }
>> +        }
>> +    }
>> +
>>      multifd_send_terminate_threads();
>>  
>>      for (i = 0; i < migrate_multifd_channels(); i++) {
>> @@ -1141,7 +1167,13 @@ static void *multifd_recv_thread(void *opaque)
>>  
>>              ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
>>                                             p->packet_len, &local_err);
>> -            if (ret == 0 || ret == -1) {   /* 0: EOF  -1: Error */
>> +            if (!ret) {
>> +                /* EOF */
>> +                assert(!local_err);
>> +                break;
>> +            }
>> +
>> +            if (ret == -1) {
>>                  break;
>>              }
>>  
>> diff --git a/migration/tls.c b/migration/tls.c
>> index fa03d9136c..5cbf952383 100644
>> --- a/migration/tls.c
>> +++ b/migration/tls.c
>> @@ -156,6 +156,11 @@ void migration_tls_channel_connect(MigrationState *s,
>>                                NULL);
>>  }
>>  
>> +void migration_tls_channel_end(QIOChannel *ioc, Error **errp)
>> +{
>> +    qio_channel_tls_bye(QIO_CHANNEL_TLS(ioc), errp);
>> +}
>> +
>>  bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc)
>>  {
>>      if (!migrate_tls()) {
>> diff --git a/migration/tls.h b/migration/tls.h
>> index 5797d153cb..58b25e1228 100644
>> --- a/migration/tls.h
>> +++ b/migration/tls.h
>> @@ -36,7 +36,7 @@ void migration_tls_channel_connect(MigrationState *s,
>>                                     QIOChannel *ioc,
>>                                     const char *hostname,
>>                                     Error **errp);
>> -
>> +void migration_tls_channel_end(QIOChannel *ioc, Error **errp);
>>  /* Whether the QIO channel requires further TLS handshake? */
>>  bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc);
>>  
>> -- 
>> 2.35.3
>> 

Re: [RFC PATCH v2 8/8] migration/multifd: Add a compat property for TLS termination

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Feb 07, 2025 at 11:27:58AM -0300, Fabiano Rosas wrote:
>> We're currently changing the way the source multifd migration handles
>> the shutdown of the multifd channels when TLS is in use to perform a
>> clean termination by calling gnutls_bye().
>> 
>> Older src QEMUs will always close the channel without terminating the
>> TLS session. New dst QEMUs treat an unclean termination as an
>> error. Due to synchronization conditions, src QEMUs 9.1 and 9.2 are an
>> exception and can put the destination in a condition where it ignores
>> the unclean termination. For src QEMUs older than 9.1, we'll need a
>> compat property on the destination to inform that the src does not
>> terminate the TLS session.
>> 
>> Add multifd_clean_tls_termination (default true) that can be switched
>> on the destination whenever a src QEMU <9.1 is in use.
>
> Patch looks good.  Though did you forget to add the compat entry?
>

Indeed.

> I suggest we add it for all pre-9.2, in case whoever backports the recent
> changes so it re-exposes again in any distro stables.
>
> IMHO it doesn't hurt us much to be always cautious on 9.1 and 9.2 too by
> loosing the termination a bit.
>

Ok, I'll put it in hw_compat_9_2.

>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration.h | 33 +++++++++++++++++++++++++++++++++
>>  migration/multifd.c   |  8 +++++++-
>>  migration/multifd.h   |  2 ++
>>  migration/options.c   |  2 ++
>>  4 files changed, 44 insertions(+), 1 deletion(-)
>> 
>> diff --git a/migration/migration.h b/migration/migration.h
>> index 4c1fafc2b5..77def0b437 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -443,6 +443,39 @@ struct MigrationState {
>>       * Default value is false. (since 8.1)
>>       */
>>      bool multifd_flush_after_each_section;
>> +
>> +    /*
>> +     * This variable only makes sense when set on the machine that is
>> +     * the destination of a multifd migration with TLS enabled. It
>> +     * affects the behavior of the last send->recv iteration with
>> +     * regards to termination of the TLS session.
>> +     *
>> +     * When set:
>> +     *
>> +     * - the destination QEMU instance can expect to never get a
>> +     *   GNUTLS_E_PREMATURE_TERMINATION error. Manifested as the error
>> +     *   message: "The TLS connection was non-properly terminated".
>> +     *
>> +     * When clear:
>> +     *
>> +     * - the destination QEMU instance can expect to see a
>> +     *   GNUTLS_E_PREMATURE_TERMINATION error in any multifd channel
>> +     *   whenever the last recv() call of that channel happens after
>> +     *   the source QEMU instance has already issued shutdown() on the
>> +     *   channel.
>> +     *
>> +     *   Commit 637280aeb2 (since 9.1) introduced a side effect that
>> +     *   causes the destination instance to not be affected by the
>> +     *   premature termination, while commit 1d457daf86 (since 10.0)
>> +     *   causes the premature termination condition to be once again
>> +     *   reachable.
>> +     *
>> +     * NOTE: Regardless of the state of this option, a premature
>> +     * termination of the TLS connection might happen due to error at
>> +     * any moment prior to the last send->recv iteration.
>> +     */
>> +    bool multifd_clean_tls_termination;
>> +
>>      /*
>>       * This decides the size of guest memory chunk that will be used
>>       * to track dirty bitmap clearing.  The size of memory chunk will
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index b4f82b0893..4342399818 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -1147,6 +1147,7 @@ void multifd_recv_sync_main(void)
>>  
>>  static void *multifd_recv_thread(void *opaque)
>>  {
>> +    MigrationState *s = migrate_get_current();
>>      MultiFDRecvParams *p = opaque;
>>      Error *local_err = NULL;
>>      bool use_packets = multifd_use_packets();
>> @@ -1155,6 +1156,10 @@ static void *multifd_recv_thread(void *opaque)
>>      trace_multifd_recv_thread_start(p->id);
>>      rcu_register_thread();
>>  
>> +    if (!s->multifd_clean_tls_termination) {
>> +        p->read_flags = QIO_CHANNEL_READ_FLAG_RELAXED_EOF;
>> +    }
>> +
>>      while (true) {
>>          uint32_t flags = 0;
>>          bool has_data = false;
>> @@ -1166,7 +1171,8 @@ static void *multifd_recv_thread(void *opaque)
>>              }
>>  
>>              ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
>> -                                           p->packet_len, 0, &local_err);
>> +                                           p->packet_len, p->read_flags,
>> +                                           &local_err);
>>              if (!ret) {
>>                  /* EOF */
>>                  assert(!local_err);
>> diff --git a/migration/multifd.h b/migration/multifd.h
>> index bd785b9873..cf408ff721 100644
>> --- a/migration/multifd.h
>> +++ b/migration/multifd.h
>> @@ -244,6 +244,8 @@ typedef struct {
>>      uint32_t zero_num;
>>      /* used for de-compression methods */
>>      void *compress_data;
>> +    /* Flags for the QIOChannel */
>> +    int read_flags;
>>  } MultiFDRecvParams;
>>  
>>  typedef struct {
>> diff --git a/migration/options.c b/migration/options.c
>> index 1ad950e397..feda354935 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -99,6 +99,8 @@ const Property migration_properties[] = {
>>                        clear_bitmap_shift, CLEAR_BITMAP_SHIFT_DEFAULT),
>>      DEFINE_PROP_BOOL("x-preempt-pre-7-2", MigrationState,
>>                       preempt_pre_7_2, false),
>> +    DEFINE_PROP_BOOL("multifd-clean-tls-termination", MigrationState,
>> +                     multifd_clean_tls_termination, true),
>>  
>>      /* Migration parameters */
>>      DEFINE_PROP_UINT8("x-throttle-trigger-threshold", MigrationState,
>> -- 
>> 2.35.3
>> 

Re: [RFC PATCH v2 0/8] crypto,io,migration: Add support to gnutls_bye()

[Maciej S. Szmigiero]

On 7.02.2025 15:27, Fabiano Rosas wrote:
> v2:
> 
> Added the premature_ok logic;
> Added compat property for QEMU <9.1;
> Refactored the existing handshake code;
> 
> CI run:
> https://gitlab.com/farosas/qemu/-/pipelines/1660800456
> 
> v1:
> https://lore.kernel.org/r/20250206175824.22664-1-farosas@suse.de
> 
> Hi,
> 
> We've been discussing a way to stop multifd recv threads from getting
> an error at the end of migration when the source threads close the
> iochannel without ending the TLS session.
> 
> The original issue was introduced by commit 1d457daf86
> ("migration/multifd: Further remove the SYNC on complete") which
> altered the synchronization of the source and destination in a manner
> that causes the destination to already be waiting at recv() when the
> source closes the connection.
> 
> One approach would be to issue gnutls_bye() at the source after all
> the data has been sent. The destination would then gracefully exit
> when it gets EOF.
> 
> Aside from stopping the recv thread from seeing an error, this also
> creates a contract that all connections should be closed only after
> the TLS session is ended. This helps to avoid masking a legitimate
> issue where the connection is closed prematurely.
> 

I've rebased my patch set on top of this version and can confirm
it works too (with respect to VFIO migration and QEMU tests).

The updated series is available at its usual place.

Thanks,
Maciej

Re: [RFC PATCH v2 3/8] migration/multifd: Terminate the TLS connection

[Peter Xu]

On Fri, Feb 07, 2025 at 03:15:48PM -0300, Fabiano Rosas wrote:
> >> +    for (i = 0; i < migrate_multifd_channels(); i++) {
> >> +        MultiFDSendParams *p = &multifd_send_state->params[i];
> >> +
> >> +        /* thread_created implies the TLS handshake has succeeded */
> >> +        if (p->tls_thread_created && p->thread_created) {
> >> +            Error *local_err = NULL;
> >> +            /*
> >> +             * The destination expects the TLS session to always be
> >> +             * properly terminated. This helps to detect a premature
> >> +             * termination in the middle of the stream.  Note that
> >> +             * older QEMUs always break the connection on the source
> >> +             * and the destination always sees
> >> +             * GNUTLS_E_PREMATURE_TERMINATION.
> >> +             */
> >> +            migration_tls_channel_end(p->c, &local_err);
> >> +
> >> +            if (local_err) {
> >> +                /*
> >> +                 * The above can fail with broken pipe due to a
> >> +                 * previous migration error, ignore the error.
> >> +                 */
> >> +                assert(migration_has_failed(migrate_get_current()));
> >
> > Considering this is still src, do we want to be softer on this by
> > error_report?
> >
> > Logically !migration_has_failed() means it succeeded, so we can throw src
> > qemu way now, that shouldn't be a huge deal. More of thinking out loud kind
> > of comment..  Your call.
> >
> 
> Maybe even a warning? If at this point migration succeeded, it's probably
> best to let cleanup carry on.

Yep, warning sounds good too.

-- 
Peter Xu