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

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

[Fabiano Rosas]

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 (4):
  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/tlssession.c         | 40 ++++++++++++++++++
 include/crypto/tlssession.h | 22 ++++++++++
 include/io/channel-tls.h    | 12 ++++++
 io/channel-tls.c            | 84 +++++++++++++++++++++++++++++++++++++
 io/trace-events             |  5 +++
 migration/multifd.c         | 34 ++++++++++++++-
 migration/savevm.c          |  6 ++-
 migration/tls.c             |  5 +++
 migration/tls.h             |  2 +-
 9 files changed, 207 insertions(+), 3 deletions(-)

-- 
2.35.3

[RFC PATCH 1/4] 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         | 40 +++++++++++++++++++++++++++++++++++++
 include/crypto/tlssession.h | 22 ++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 77286e23f4..d52714a3f3 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,12 @@ 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

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

[Daniel P. Berrangé]

On Thu, Feb 06, 2025 at 02:58:21PM -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")
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  crypto/tlssession.c         | 40 +++++++++++++++++++++++++++++++++++++
>  include/crypto/tlssession.h | 22 ++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> index 77286e23f4..d52714a3f3 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,12 @@ 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);

For handshake I have separate methods

   qcrypto_tls_session_handshake
   qcrypto_tls_session_get_handshake_status

Essentially because I was missing gnutls which has the separate
method to get the record direction.

I would prefer if the handshake and bye code used the same design
pattern.

In retrospect the 2 separate methods for handshake appears redundant
as all uses of them invoke both from the same piece of code. So I'm
fine if you re-factor the existing handshake method to return the
status immediate and get rid of qcrypto_tls_session_get_handshake_status
to match you 'bye' design here.

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 :|

[RFC PATCH 2/4] 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 3/4] 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 4/4] 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

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

[Maciej S. Szmigiero]

On 6.02.2025 18:58, Fabiano Rosas wrote:
> 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.
> 

Thanks for quickly posting this RFC.

I've applied these patches on top of the current QEMU git master,
ported my patch set on top of it, dropped my premature_ok patches
and can confirm that the tests still pass.

Thanks,
Maciej