[PATCH v2 0/3] migration/tls: Graceful shutdowns for main and postcopy channels

[PATCH v2 0/3] migration/tls: Graceful shutdowns for main and postcopy channels

[Peter Xu]

This is v2 of the series.  Note that v2 is almost a rewrite, so please
ignore v1, and there's no changelog too.

Fabiano fixed graceful shutdowns for multifd channels previously:

https://lore.kernel.org/qemu-devel/20250206175824.22664-1-farosas@suse.de/

However we can still see an warning when running preempt unit test on TLS,
even though migration functionality will not be affected:

QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/postcopy/preempt/tls/psk
...
qemu-kvm: Cannot read from TLS channel: The TLS connection was non-properly terminated.
...

It turns out this is because the crypto code only passes the ->shutdown
field into the read function, however that value can change concurrently in
another thread by a concurrent qio_channel_shutdown().

Patch 1 should fix this issue.

Patch 2-3 are something I found when looking at this problem, there's no
known issues I am aware of with them, however I still think they're
logically flawed, so I post them together here.

Please review, thanks.

Peter Xu (3):
  io/crypto: Move tls premature termination handling into QIO layer
  io/tls: Make qio_channel_tls_bye() always synchronous
  migration: Make migration_has_failed() work even for CANCELLING

 include/crypto/tlssession.h |   7 +--
 include/io/channel-tls.h    |   5 +-
 crypto/tlssession.c         |   7 +--
 io/channel-tls.c            | 107 ++++++++++--------------------------
 migration/migration.c       |   3 +-
 io/trace-events             |   3 +-
 6 files changed, 39 insertions(+), 93 deletions(-)

-- 
2.50.1

[PATCH v2 1/3] io/crypto: Move tls premature termination handling into QIO layer

[Peter Xu]

QCryptoTLSSession allows TLS premature termination in two cases, one of the
case is when the channel shutdown() is invoked on READ side.

It's possible the shutdown() happened after the read thread blocked at
gnutls_record_recv().  In this case, we should allow the premature
termination to happen.

The problem is by the time qcrypto_tls_session_read() was invoked,
tioc->shutdown may not have been set, so this may instead be treated as an
error if there is concurrent shutdown() calls.

To allow the flag to reflect the latest status of tioc->shutdown, move the
check upper into the QIOChannel level, so as to read the flag only after
QEMU gets an GNUTLS_E_PREMATURE_TERMINATION.

When at it, introduce qio_channel_tls_allow_premature_termination() helper
to make the condition checks easier to read.

This patch will fix a qemu qtest warning when running the preempt tls test,
reporting premature termination:

QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/postcopy/preempt/tls/psk
...
qemu-kvm: Cannot read from TLS channel: The TLS connection was non-properly terminated.
...

In this specific case, the error was set by postcopy_preempt_thread, which
normally will be concurrently shutdown()ed by the main thread.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/crypto/tlssession.h |  7 +------
 crypto/tlssession.c         |  7 ++-----
 io/channel-tls.c            | 21 +++++++++++++++++++--
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index 2f62ce2d67..6b4fcadee7 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -110,6 +110,7 @@
 typedef struct QCryptoTLSSession QCryptoTLSSession;
 
 #define QCRYPTO_TLS_SESSION_ERR_BLOCK -2
+#define QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION -3
 
 /**
  * qcrypto_tls_session_new:
@@ -259,7 +260,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
  * @sess: the TLS session object
  * @buf: to fill with plain text received
  * @len: the length of @buf
- * @gracefulTermination: treat premature termination as graceful EOF
  * @errp: pointer to hold returned error object
  *
  * Receive up to @len bytes of data from the remote peer
@@ -267,10 +267,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
  * qcrypto_tls_session_set_callbacks(), decrypt it and
  * store it in @buf.
  *
- * If @gracefulTermination is true, then a premature termination
- * of the TLS session will be treated as indicating EOF, as
- * opposed to an error.
- *
  * It is an error to call this before
  * qcrypto_tls_session_handshake() returns
  * QCRYPTO_TLS_HANDSHAKE_COMPLETE
@@ -282,7 +278,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
 ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess,
                                  char *buf,
                                  size_t len,
-                                 bool gracefulTermination,
                                  Error **errp);
 
 /**
diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 86d407a142..ac38c2121d 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -552,7 +552,6 @@ ssize_t
 qcrypto_tls_session_read(QCryptoTLSSession *session,
                          char *buf,
                          size_t len,
-                         bool gracefulTermination,
                          Error **errp)
 {
     ssize_t ret;
@@ -570,9 +569,8 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
     if (ret < 0) {
         if (ret == GNUTLS_E_AGAIN) {
             return QCRYPTO_TLS_SESSION_ERR_BLOCK;
-        } else if ((ret == GNUTLS_E_PREMATURE_TERMINATION) &&
-                   gracefulTermination){
-            return 0;
+        } else if (ret == GNUTLS_E_PREMATURE_TERMINATION) {
+            return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION;
         } else {
             if (session->rerr) {
                 error_propagate(errp, session->rerr);
@@ -789,7 +787,6 @@ ssize_t
 qcrypto_tls_session_read(QCryptoTLSSession *sess,
                          char *buf,
                          size_t len,
-                         bool gracefulTermination,
                          Error **errp)
 {
     error_setg(errp, "TLS requires GNUTLS support");
diff --git a/io/channel-tls.c b/io/channel-tls.c
index a8248a9216..5a2c8188ce 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -346,6 +346,19 @@ static void qio_channel_tls_finalize(Object *obj)
     qcrypto_tls_session_free(ioc->session);
 }
 
+static bool
+qio_channel_tls_allow_premature_termination(QIOChannelTLS *tioc, int flags)
+{
+    if (flags & QIO_CHANNEL_READ_FLAG_RELAXED_EOF) {
+        return true;
+    }
+
+    if (qatomic_read(&tioc->shutdown) & QIO_CHANNEL_SHUTDOWN_READ) {
+        return true;
+    }
+
+    return false;
+}
 
 static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
                                      const struct iovec *iov,
@@ -364,8 +377,6 @@ 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) {
             if (got) {
@@ -373,6 +384,12 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
             } else {
                 return QIO_CHANNEL_ERR_BLOCK;
             }
+        } else if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION) {
+            if (qio_channel_tls_allow_premature_termination(tioc, flags)) {
+                ret = 0;
+            } else {
+                return -1;
+            }
         } else if (ret < 0) {
             return -1;
         }
-- 
2.50.1

Re: [PATCH v2 1/3] io/crypto: Move tls premature termination handling into QIO layer

[Daniel P. Berrangé]

On Thu, Sep 11, 2025 at 05:23:53PM -0400, Peter Xu wrote:
> QCryptoTLSSession allows TLS premature termination in two cases, one of the
> case is when the channel shutdown() is invoked on READ side.
> 
> It's possible the shutdown() happened after the read thread blocked at
> gnutls_record_recv().  In this case, we should allow the premature
> termination to happen.
> 
> The problem is by the time qcrypto_tls_session_read() was invoked,
> tioc->shutdown may not have been set, so this may instead be treated as an
> error if there is concurrent shutdown() calls.
> 
> To allow the flag to reflect the latest status of tioc->shutdown, move the
> check upper into the QIOChannel level, so as to read the flag only after
> QEMU gets an GNUTLS_E_PREMATURE_TERMINATION.
> 
> When at it, introduce qio_channel_tls_allow_premature_termination() helper
> to make the condition checks easier to read.
> 
> This patch will fix a qemu qtest warning when running the preempt tls test,
> reporting premature termination:
> 
> QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/postcopy/preempt/tls/psk
> ...
> qemu-kvm: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> ...
> 
> In this specific case, the error was set by postcopy_preempt_thread, which
> normally will be concurrently shutdown()ed by the main thread.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/crypto/tlssession.h |  7 +------
>  crypto/tlssession.c         |  7 ++-----
>  io/channel-tls.c            | 21 +++++++++++++++++++--
>  3 files changed, 22 insertions(+), 13 deletions(-)

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

> diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
> index 2f62ce2d67..6b4fcadee7 100644
> --- a/include/crypto/tlssession.h
> +++ b/include/crypto/tlssession.h
> @@ -110,6 +110,7 @@
>  typedef struct QCryptoTLSSession QCryptoTLSSession;
>  
>  #define QCRYPTO_TLS_SESSION_ERR_BLOCK -2
> +#define QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION -3
>  
>  /**
>   * qcrypto_tls_session_new:
> @@ -259,7 +260,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
>   * @sess: the TLS session object
>   * @buf: to fill with plain text received
>   * @len: the length of @buf
> - * @gracefulTermination: treat premature termination as graceful EOF
>   * @errp: pointer to hold returned error object
>   *
>   * Receive up to @len bytes of data from the remote peer
> @@ -267,10 +267,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
>   * qcrypto_tls_session_set_callbacks(), decrypt it and
>   * store it in @buf.
>   *
> - * If @gracefulTermination is true, then a premature termination
> - * of the TLS session will be treated as indicating EOF, as
> - * opposed to an error.
> - *


Could you say something about QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION
being a possible return code here (no need to repost just for that). 

>   * It is an error to call this before
>   * qcrypto_tls_session_handshake() returns
>   * QCRYPTO_TLS_HANDSHAKE_COMPLETE
> @@ -282,7 +278,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
>  ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess,
>                                   char *buf,
>                                   size_t len,
> -                                 bool gracefulTermination,
>                                   Error **errp);
>  
>  /**

> +static bool
> +qio_channel_tls_allow_premature_termination(QIOChannelTLS *tioc, int flags)
> +{
> +    if (flags & QIO_CHANNEL_READ_FLAG_RELAXED_EOF) {
> +        return true;
> +    }
> +
> +    if (qatomic_read(&tioc->shutdown) & QIO_CHANNEL_SHUTDOWN_READ) {
> +        return true;
> +    }
> +
> +    return false;
> +}
>  
>  static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
>                                       const struct iovec *iov,
> @@ -364,8 +377,6 @@ 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);


The original code uses qatomic_load_acquire() while the new code
uses qatomic_read() which imposes weaker ordering constraints.

Does this matter ? I'm not familiar enough with atomics to say
which we need here ?

>          if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
>              if (got) {
> @@ -373,6 +384,12 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
>              } else {
>                  return QIO_CHANNEL_ERR_BLOCK;
>              }
> +        } else if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION) {
> +            if (qio_channel_tls_allow_premature_termination(tioc, flags)) {
> +                ret = 0;
> +            } else {
> +                return -1;
> +            }
>          } else if (ret < 0) {
>              return -1;
>          }
> -- 
> 2.50.1
> 

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: [PATCH v2 1/3] io/crypto: Move tls premature termination handling into QIO layer

[Peter Xu]

On Fri, Sep 12, 2025 at 12:18:18PM +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 11, 2025 at 05:23:53PM -0400, Peter Xu wrote:
> > QCryptoTLSSession allows TLS premature termination in two cases, one of the
> > case is when the channel shutdown() is invoked on READ side.
> > 
> > It's possible the shutdown() happened after the read thread blocked at
> > gnutls_record_recv().  In this case, we should allow the premature
> > termination to happen.
> > 
> > The problem is by the time qcrypto_tls_session_read() was invoked,
> > tioc->shutdown may not have been set, so this may instead be treated as an
> > error if there is concurrent shutdown() calls.
> > 
> > To allow the flag to reflect the latest status of tioc->shutdown, move the
> > check upper into the QIOChannel level, so as to read the flag only after
> > QEMU gets an GNUTLS_E_PREMATURE_TERMINATION.
> > 
> > When at it, introduce qio_channel_tls_allow_premature_termination() helper
> > to make the condition checks easier to read.
> > 
> > This patch will fix a qemu qtest warning when running the preempt tls test,
> > reporting premature termination:
> > 
> > QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/postcopy/preempt/tls/psk
> > ...
> > qemu-kvm: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> > ...
> > 
> > In this specific case, the error was set by postcopy_preempt_thread, which
> > normally will be concurrently shutdown()ed by the main thread.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/crypto/tlssession.h |  7 +------
> >  crypto/tlssession.c         |  7 ++-----
> >  io/channel-tls.c            | 21 +++++++++++++++++++--
> >  3 files changed, 22 insertions(+), 13 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> > diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
> > index 2f62ce2d67..6b4fcadee7 100644
> > --- a/include/crypto/tlssession.h
> > +++ b/include/crypto/tlssession.h
> > @@ -110,6 +110,7 @@
> >  typedef struct QCryptoTLSSession QCryptoTLSSession;
> >  
> >  #define QCRYPTO_TLS_SESSION_ERR_BLOCK -2
> > +#define QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION -3
> >  
> >  /**
> >   * qcrypto_tls_session_new:
> > @@ -259,7 +260,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
> >   * @sess: the TLS session object
> >   * @buf: to fill with plain text received
> >   * @len: the length of @buf
> > - * @gracefulTermination: treat premature termination as graceful EOF
> >   * @errp: pointer to hold returned error object
> >   *
> >   * Receive up to @len bytes of data from the remote peer
> > @@ -267,10 +267,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
> >   * qcrypto_tls_session_set_callbacks(), decrypt it and
> >   * store it in @buf.
> >   *
> > - * If @gracefulTermination is true, then a premature termination
> > - * of the TLS session will be treated as indicating EOF, as
> > - * opposed to an error.
> > - *
> 
> 
> Could you say something about QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION
> being a possible return code here (no need to repost just for that). 

Definitely, I overlooked the doc there..

I'll squash this when repost:

diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index 6b4fcadee7..2e9fe11cf6 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -273,7 +273,8 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
  *
  * Returns: the number of bytes received,
  * or QCRYPTO_TLS_SESSION_ERR_BLOCK if the receive would block,
- * or -1 on error.
+ * or QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION if a premature termination
+ * is detected, or -1 on error.
  */
 ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess,
                                  char *buf,

> 
> >   * It is an error to call this before
> >   * qcrypto_tls_session_handshake() returns
> >   * QCRYPTO_TLS_HANDSHAKE_COMPLETE
> > @@ -282,7 +278,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
> >  ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess,
> >                                   char *buf,
> >                                   size_t len,
> > -                                 bool gracefulTermination,
> >                                   Error **errp);
> >  
> >  /**
> 
> > +static bool
> > +qio_channel_tls_allow_premature_termination(QIOChannelTLS *tioc, int flags)
> > +{
> > +    if (flags & QIO_CHANNEL_READ_FLAG_RELAXED_EOF) {
> > +        return true;
> > +    }
> > +
> > +    if (qatomic_read(&tioc->shutdown) & QIO_CHANNEL_SHUTDOWN_READ) {
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> >  
> >  static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
> >                                       const struct iovec *iov,
> > @@ -364,8 +377,6 @@ 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);
> 
> 
> The original code uses qatomic_load_acquire() while the new code
> uses qatomic_read() which imposes weaker ordering constraints.
> 
> Does this matter ? I'm not familiar enough with atomics to say
> which we need here ?

My bad, I explicitly changed it but I forgot to mention it in the commit
message.

I don't think we need memory barriers here, because memory barriers are
only used to describe ordering of at least more than one memory operation.
Here we sololy want to read the flag which implies whether a shutdown READ
was initiated, so IMHO qatomic_read() is the thing we want.  Comparing to
raw access to tioc->shutdown, it's almost "volatile" making sure we fetch
from memory, so when another thread modifies it on the fly we'll see.

I'll explain it in the commit message when repost.

Thanks,

> 
> >          if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
> >              if (got) {
> > @@ -373,6 +384,12 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
> >              } else {
> >                  return QIO_CHANNEL_ERR_BLOCK;
> >              }
> > +        } else if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION) {
> > +            if (qio_channel_tls_allow_premature_termination(tioc, flags)) {
> > +                ret = 0;
> > +            } else {
> > +                return -1;
> > +            }
> >          } else if (ret < 0) {
> >              return -1;
> >          }
> > -- 
> > 2.50.1
> > 
> 
> 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 :|
> 

-- 
Peter Xu

Re: [PATCH v2 1/3] io/crypto: Move tls premature termination handling into QIO layer

[Daniel P. Berrangé]

On Fri, Sep 12, 2025 at 11:24:34AM -0400, Peter Xu wrote:
> On Fri, Sep 12, 2025 at 12:18:18PM +0100, Daniel P. Berrangé wrote:
> > On Thu, Sep 11, 2025 at 05:23:53PM -0400, Peter Xu wrote:
> > > QCryptoTLSSession allows TLS premature termination in two cases, one of the
> > > case is when the channel shutdown() is invoked on READ side.
> > > 
> > > It's possible the shutdown() happened after the read thread blocked at
> > > gnutls_record_recv().  In this case, we should allow the premature
> > > termination to happen.
> > > 
> > > The problem is by the time qcrypto_tls_session_read() was invoked,
> > > tioc->shutdown may not have been set, so this may instead be treated as an
> > > error if there is concurrent shutdown() calls.
> > > 
> > > To allow the flag to reflect the latest status of tioc->shutdown, move the
> > > check upper into the QIOChannel level, so as to read the flag only after
> > > QEMU gets an GNUTLS_E_PREMATURE_TERMINATION.
> > > 
> > > When at it, introduce qio_channel_tls_allow_premature_termination() helper
> > > to make the condition checks easier to read.
> > > 
> > > This patch will fix a qemu qtest warning when running the preempt tls test,
> > > reporting premature termination:
> > > 
> > > QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/postcopy/preempt/tls/psk
> > > ...
> > > qemu-kvm: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> > > ...
> > > 
> > > In this specific case, the error was set by postcopy_preempt_thread, which
> > > normally will be concurrently shutdown()ed by the main thread.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  include/crypto/tlssession.h |  7 +------
> > >  crypto/tlssession.c         |  7 ++-----
> > >  io/channel-tls.c            | 21 +++++++++++++++++++--
> > >  3 files changed, 22 insertions(+), 13 deletions(-)
> > 
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > > diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
> > > index 2f62ce2d67..6b4fcadee7 100644
> > > --- a/include/crypto/tlssession.h
> > > +++ b/include/crypto/tlssession.h
> > > @@ -110,6 +110,7 @@
> > >  typedef struct QCryptoTLSSession QCryptoTLSSession;
> > >  
> > >  #define QCRYPTO_TLS_SESSION_ERR_BLOCK -2
> > > +#define QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION -3
> > >  
> > >  /**
> > >   * qcrypto_tls_session_new:
> > > @@ -259,7 +260,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
> > >   * @sess: the TLS session object
> > >   * @buf: to fill with plain text received
> > >   * @len: the length of @buf
> > > - * @gracefulTermination: treat premature termination as graceful EOF
> > >   * @errp: pointer to hold returned error object
> > >   *
> > >   * Receive up to @len bytes of data from the remote peer
> > > @@ -267,10 +267,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
> > >   * qcrypto_tls_session_set_callbacks(), decrypt it and
> > >   * store it in @buf.
> > >   *
> > > - * If @gracefulTermination is true, then a premature termination
> > > - * of the TLS session will be treated as indicating EOF, as
> > > - * opposed to an error.
> > > - *
> > 
> > 
> > Could you say something about QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION
> > being a possible return code here (no need to repost just for that). 
> 
> Definitely, I overlooked the doc there..
> 
> I'll squash this when repost:
> 
> diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
> index 6b4fcadee7..2e9fe11cf6 100644
> --- a/include/crypto/tlssession.h
> +++ b/include/crypto/tlssession.h
> @@ -273,7 +273,8 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
>   *
>   * Returns: the number of bytes received,
>   * or QCRYPTO_TLS_SESSION_ERR_BLOCK if the receive would block,
> - * or -1 on error.
> + * or QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION if a premature termination
> + * is detected, or -1 on error.
>   */
>  ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess,
>                                   char *buf,

ACK,


> 
> > 
> > >   * It is an error to call this before
> > >   * qcrypto_tls_session_handshake() returns
> > >   * QCRYPTO_TLS_HANDSHAKE_COMPLETE
> > > @@ -282,7 +278,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
> > >  ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess,
> > >                                   char *buf,
> > >                                   size_t len,
> > > -                                 bool gracefulTermination,
> > >                                   Error **errp);
> > >  
> > >  /**
> > 
> > > +static bool
> > > +qio_channel_tls_allow_premature_termination(QIOChannelTLS *tioc, int flags)
> > > +{
> > > +    if (flags & QIO_CHANNEL_READ_FLAG_RELAXED_EOF) {
> > > +        return true;
> > > +    }
> > > +
> > > +    if (qatomic_read(&tioc->shutdown) & QIO_CHANNEL_SHUTDOWN_READ) {
> > > +        return true;
> > > +    }
> > > +
> > > +    return false;
> > > +}
> > >  
> > >  static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
> > >                                       const struct iovec *iov,
> > > @@ -364,8 +377,6 @@ 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);
> > 
> > 
> > The original code uses qatomic_load_acquire() while the new code
> > uses qatomic_read() which imposes weaker ordering constraints.
> > 
> > Does this matter ? I'm not familiar enough with atomics to say
> > which we need here ?
> 
> My bad, I explicitly changed it but I forgot to mention it in the commit
> message.
> 
> I don't think we need memory barriers here, because memory barriers are
> only used to describe ordering of at least more than one memory operation.
> Here we sololy want to read the flag which implies whether a shutdown READ
> was initiated, so IMHO qatomic_read() is the thing we want.  Comparing to
> raw access to tioc->shutdown, it's almost "volatile" making sure we fetch
> from memory, so when another thread modifies it on the fly we'll see.
> 
> I'll explain it in the commit message when repost.

Thanks, that all sounds good.


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: [PATCH v2 1/3] io/crypto: Move tls premature termination handling into QIO layer

[Juraj Marcin]

On 2025-09-11 17:23, Peter Xu wrote:
> QCryptoTLSSession allows TLS premature termination in two cases, one of the
> case is when the channel shutdown() is invoked on READ side.
> 
> It's possible the shutdown() happened after the read thread blocked at
> gnutls_record_recv().  In this case, we should allow the premature
> termination to happen.
> 
> The problem is by the time qcrypto_tls_session_read() was invoked,
> tioc->shutdown may not have been set, so this may instead be treated as an
> error if there is concurrent shutdown() calls.
> 
> To allow the flag to reflect the latest status of tioc->shutdown, move the
> check upper into the QIOChannel level, so as to read the flag only after
> QEMU gets an GNUTLS_E_PREMATURE_TERMINATION.
> 
> When at it, introduce qio_channel_tls_allow_premature_termination() helper
> to make the condition checks easier to read.
> 
> This patch will fix a qemu qtest warning when running the preempt tls test,
> reporting premature termination:
> 
> QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/postcopy/preempt/tls/psk
> ...
> qemu-kvm: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> ...
> 
> In this specific case, the error was set by postcopy_preempt_thread, which
> normally will be concurrently shutdown()ed by the main thread.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/crypto/tlssession.h |  7 +------
>  crypto/tlssession.c         |  7 ++-----
>  io/channel-tls.c            | 21 +++++++++++++++++++--
>  3 files changed, 22 insertions(+), 13 deletions(-)

Reviewed-by: Juraj Marcin <jmarcin@redhat.com>

Re: [PATCH v2 1/3] io/crypto: Move tls premature termination handling into QIO layer

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> QCryptoTLSSession allows TLS premature termination in two cases, one of the
> case is when the channel shutdown() is invoked on READ side.
>
> It's possible the shutdown() happened after the read thread blocked at
> gnutls_record_recv().  In this case, we should allow the premature
> termination to happen.
>
> The problem is by the time qcrypto_tls_session_read() was invoked,
> tioc->shutdown may not have been set, so this may instead be treated as an
> error if there is concurrent shutdown() calls.
>
> To allow the flag to reflect the latest status of tioc->shutdown, move the
> check upper into the QIOChannel level, so as to read the flag only after
> QEMU gets an GNUTLS_E_PREMATURE_TERMINATION.
>
> When at it, introduce qio_channel_tls_allow_premature_termination() helper
> to make the condition checks easier to read.
>
> This patch will fix a qemu qtest warning when running the preempt tls test,
> reporting premature termination:
>
> QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/postcopy/preempt/tls/psk
> ...
> qemu-kvm: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> ...
>
> In this specific case, the error was set by postcopy_preempt_thread, which
> normally will be concurrently shutdown()ed by the main thread.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 2/3] io/tls: Make qio_channel_tls_bye() always synchronous

[Peter Xu]

No issue I hit, the change is only from code observation when I am looking
at a TLS premature termination issue.

qio_channel_tls_bye() API needs to be synchronous.  When it's not, the
previous impl will attach an asynchronous task retrying but only until when
the channel gets the relevant GIO event. It may be problematic, because the
caller of qio_channel_tls_bye() may have invoked channel close() before
that, leading to premature termination of the TLS session.

Remove the asynchronous handling, instead retry it immediately.  Currently,
the only two possible cases that may lead to async task is either INTERRUPT
or EAGAIN.  It should be suffice to spin retry as of now, until a solid
proof showing that a more complicated retry logic is needed.

With that, we can remove the whole async model for the bye task.

When at it, making the function return bool, which looks like a common
pattern in QEMU when errp is used.

Side note on the tracepoints: previously the tracepoint bye_complete()
isn't used.  Start to use it in this patch.  bye_pending() and bye_cancel()
can be dropped now.  Adding bye_retry() instead.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/io/channel-tls.h |  5 ++-
 io/channel-tls.c         | 86 +++++-----------------------------------
 io/trace-events          |  3 +-
 3 files changed, 15 insertions(+), 79 deletions(-)

diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
index 7e9023570d..bcd14ffbd6 100644
--- a/include/io/channel-tls.h
+++ b/include/io/channel-tls.h
@@ -49,7 +49,6 @@ struct QIOChannelTLS {
     QCryptoTLSSession *session;
     QIOChannelShutdown shutdown;
     guint hs_ioc_tag;
-    guint bye_ioc_tag;
 };
 
 /**
@@ -60,8 +59,10 @@ struct QIOChannelTLS {
  * Perform the TLS session termination. This method will return
  * immediately and the termination will continue in the background,
  * provided the main loop is running.
+ *
+ * Returns: true on success, false on error (with errp set)
  */
-void qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp);
+bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp);
 
 /**
  * qio_channel_tls_new_server:
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 5a2c8188ce..8510a187a8 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -253,84 +253,25 @@ 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)
+bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp)
 {
-    GIOCondition condition;
-    QIOChannelTLSData *data;
     int status;
-    Error *err = NULL;
 
-    status = qcrypto_tls_session_bye(ioc->session, &err);
+    trace_qio_channel_tls_bye_start(ioc);
+retry:
+    status = qcrypto_tls_session_bye(ioc->session, errp);
 
     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;
+    } else if (status != QCRYPTO_TLS_BYE_COMPLETE) {
+        /* BYE event must be synchronous, retry immediately */
+        trace_qio_channel_tls_bye_retry(ioc, status);
+        goto retry;
     }
 
-    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);
+    trace_qio_channel_tls_bye_complete(ioc);
+    return true;
 }
 
 static void qio_channel_tls_init(Object *obj G_GNUC_UNUSED)
@@ -482,11 +423,6 @@ 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 dc3a63ba1f..67b3814192 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -45,10 +45,9 @@ 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_retry(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.50.1

Re: [PATCH v2 2/3] io/tls: Make qio_channel_tls_bye() always synchronous

[Daniel P. Berrangé]

On Thu, Sep 11, 2025 at 05:23:54PM -0400, Peter Xu wrote:
> No issue I hit, the change is only from code observation when I am looking
> at a TLS premature termination issue.
> 
> qio_channel_tls_bye() API needs to be synchronous.  When it's not, the
> previous impl will attach an asynchronous task retrying but only until when
> the channel gets the relevant GIO event. It may be problematic, because the
> caller of qio_channel_tls_bye() may have invoked channel close() before
> that, leading to premature termination of the TLS session.
> 
> Remove the asynchronous handling, instead retry it immediately.  Currently,
> the only two possible cases that may lead to async task is either INTERRUPT
> or EAGAIN.  It should be suffice to spin retry as of now, until a solid
> proof showing that a more complicated retry logic is needed.
> 
> With that, we can remove the whole async model for the bye task.
> 
> When at it, making the function return bool, which looks like a common
> pattern in QEMU when errp is used.
> 
> Side note on the tracepoints: previously the tracepoint bye_complete()
> isn't used.  Start to use it in this patch.  bye_pending() and bye_cancel()
> can be dropped now.  Adding bye_retry() instead.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/io/channel-tls.h |  5 ++-
>  io/channel-tls.c         | 86 +++++-----------------------------------
>  io/trace-events          |  3 +-
>  3 files changed, 15 insertions(+), 79 deletions(-)
> 
> diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
> index 7e9023570d..bcd14ffbd6 100644
> --- a/include/io/channel-tls.h
> +++ b/include/io/channel-tls.h
> @@ -49,7 +49,6 @@ struct QIOChannelTLS {
>      QCryptoTLSSession *session;
>      QIOChannelShutdown shutdown;
>      guint hs_ioc_tag;
> -    guint bye_ioc_tag;
>  };
>  
>  /**
> @@ -60,8 +59,10 @@ struct QIOChannelTLS {
>   * Perform the TLS session termination. This method will return
>   * immediately and the termination will continue in the background,
>   * provided the main loop is running.
> + *
> + * Returns: true on success, false on error (with errp set)
>   */
> -void qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp);
> +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp);
>  
>  /**
>   * qio_channel_tls_new_server:
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 5a2c8188ce..8510a187a8 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -253,84 +253,25 @@ 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)
> +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp)
>  {
> -    GIOCondition condition;
> -    QIOChannelTLSData *data;
>      int status;
> -    Error *err = NULL;
>  
> -    status = qcrypto_tls_session_bye(ioc->session, &err);
> +    trace_qio_channel_tls_bye_start(ioc);
> +retry:
> +    status = qcrypto_tls_session_bye(ioc->session, errp);
>  
>      if (status < 0) {
>          trace_qio_channel_tls_bye_fail(ioc);

snip

> +        return false;
> +    } else if (status != QCRYPTO_TLS_BYE_COMPLETE) {
> +        /* BYE event must be synchronous, retry immediately */
> +        trace_qio_channel_tls_bye_retry(ioc, status);
> +        goto retry;
>      }

We cannot do this. If the gnutls_bye() API needs to perform
socket I/O, and so when we're running over a non-blocking
socket we must expect EAGAIN. With this handling, QEMU will
busy loop burning 100% CPU when the socket is not ready.

A second point is that from a QIOChannel POV, we need to
ensure that all APIs can be used in a non-blocking scenario.
This is why in the QIOChannelSocket impl connect/listen APIs
we provide both _sync and _async variants of the APIs, or
in the QIOChannelTLS impl, the handshake API is always
async with a callback to be invokved on completion.

The QIOChanel 'bye' method is flawed in that it is
asynchronous, but has no callback for completion.

If migration is /always/ using a blocking socket for the
TLS channels this isn't a problem as gnutls will complete
immediately, but if any async sockets are used we have no
way to wait for completion. This requires improving the
API design in some manner.


>  
> -    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);
> +    trace_qio_channel_tls_bye_complete(ioc);
> +    return true;
>  }
>  
>  static void qio_channel_tls_init(Object *obj G_GNUC_UNUSED)
> @@ -482,11 +423,6 @@ 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 dc3a63ba1f..67b3814192 100644
> --- a/io/trace-events
> +++ b/io/trace-events
> @@ -45,10 +45,9 @@ 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_retry(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.50.1
> 

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: [PATCH v2 2/3] io/tls: Make qio_channel_tls_bye() always synchronous

[Peter Xu]

On Fri, Sep 12, 2025 at 12:27:52PM +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 11, 2025 at 05:23:54PM -0400, Peter Xu wrote:
> > No issue I hit, the change is only from code observation when I am looking
> > at a TLS premature termination issue.
> > 
> > qio_channel_tls_bye() API needs to be synchronous.  When it's not, the
> > previous impl will attach an asynchronous task retrying but only until when
> > the channel gets the relevant GIO event. It may be problematic, because the
> > caller of qio_channel_tls_bye() may have invoked channel close() before
> > that, leading to premature termination of the TLS session.
> > 
> > Remove the asynchronous handling, instead retry it immediately.  Currently,
> > the only two possible cases that may lead to async task is either INTERRUPT
> > or EAGAIN.  It should be suffice to spin retry as of now, until a solid
> > proof showing that a more complicated retry logic is needed.
> > 
> > With that, we can remove the whole async model for the bye task.
> > 
> > When at it, making the function return bool, which looks like a common
> > pattern in QEMU when errp is used.
> > 
> > Side note on the tracepoints: previously the tracepoint bye_complete()
> > isn't used.  Start to use it in this patch.  bye_pending() and bye_cancel()
> > can be dropped now.  Adding bye_retry() instead.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/io/channel-tls.h |  5 ++-
> >  io/channel-tls.c         | 86 +++++-----------------------------------
> >  io/trace-events          |  3 +-
> >  3 files changed, 15 insertions(+), 79 deletions(-)
> > 
> > diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
> > index 7e9023570d..bcd14ffbd6 100644
> > --- a/include/io/channel-tls.h
> > +++ b/include/io/channel-tls.h
> > @@ -49,7 +49,6 @@ struct QIOChannelTLS {
> >      QCryptoTLSSession *session;
> >      QIOChannelShutdown shutdown;
> >      guint hs_ioc_tag;
> > -    guint bye_ioc_tag;
> >  };
> >  
> >  /**
> > @@ -60,8 +59,10 @@ struct QIOChannelTLS {
> >   * Perform the TLS session termination. This method will return
> >   * immediately and the termination will continue in the background,
> >   * provided the main loop is running.
> > + *
> > + * Returns: true on success, false on error (with errp set)
> >   */
> > -void qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp);
> > +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp);
> >  
> >  /**
> >   * qio_channel_tls_new_server:
> > diff --git a/io/channel-tls.c b/io/channel-tls.c
> > index 5a2c8188ce..8510a187a8 100644
> > --- a/io/channel-tls.c
> > +++ b/io/channel-tls.c
> > @@ -253,84 +253,25 @@ 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)
> > +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp)
> >  {
> > -    GIOCondition condition;
> > -    QIOChannelTLSData *data;
> >      int status;
> > -    Error *err = NULL;
> >  
> > -    status = qcrypto_tls_session_bye(ioc->session, &err);
> > +    trace_qio_channel_tls_bye_start(ioc);
> > +retry:
> > +    status = qcrypto_tls_session_bye(ioc->session, errp);
> >  
> >      if (status < 0) {
> >          trace_qio_channel_tls_bye_fail(ioc);
> 
> snip
> 
> > +        return false;
> > +    } else if (status != QCRYPTO_TLS_BYE_COMPLETE) {
> > +        /* BYE event must be synchronous, retry immediately */
> > +        trace_qio_channel_tls_bye_retry(ioc, status);
> > +        goto retry;
> >      }
> 
> We cannot do this. If the gnutls_bye() API needs to perform
> socket I/O, and so when we're running over a non-blocking
> socket we must expect EAGAIN. With this handling, QEMU will
> busy loop burning 100% CPU when the socket is not ready.

Right.  That was the plan when drafting this, the hope is spinning will
almost not happen, and even if it happens it'll finish soon (migration is
completing, it means network mustn't be so bad), unless the network is
stuck exactly at when we send the bye().

> 
> A second point is that from a QIOChannel POV, we need to
> ensure that all APIs can be used in a non-blocking scenario.
> This is why in the QIOChannelSocket impl connect/listen APIs
> we provide both _sync and _async variants of the APIs, or
> in the QIOChannelTLS impl, the handshake API is always
> async with a callback to be invokved on completion.

I agree.  The issue is if so, migration code needs to be always be prepared
with a possible async op even if in 99.9999% cases it won't happen... we
need to complicate the multifd logic a lot for this, but the gain is
little..

This series still used patch 1 to fix the problem (rather than do real BYE
on preempt channels, for example) only because it's the easiest, after all
it's still a contract in tls channel impl to allow premature termination
for explicit shutdown()s on the host.

If we want to do 100% graceful shutdowns, we'll need to apply this to all
channels, and the async-possible model can definitely add more complexity
more than multifd.  I hope it won't be necessary.. but just to mention it.

> 
> The QIOChanel 'bye' method is flawed in that it is
> asynchronous, but has no callback for completion.
> 
> If migration is /always/ using a blocking socket for the
> TLS channels this isn't a problem as gnutls will complete
> immediately, but if any async sockets are used we have no
> way to wait for completion. This requires improving the
> API design in some manner.

I recall one of your future series on TLS would start to enable async for
all channels?  In all cases, we definitely don't want to have this call to
be relevant to the blocking mode of the channels.

Would it make sense to introduce a _sync() version of it, but keep the
original bye(), leaving the rest until a real async user appears?

I can also at least drop this patch as of now, because we can still wish it
almost always be synchronous.  However we have risk forgetting that forever
and hit it a few years later..

Thanks,

-- 
Peter Xu

Re: [PATCH v2 2/3] io/tls: Make qio_channel_tls_bye() always synchronous

[Daniel P. Berrangé]

On Fri, Sep 12, 2025 at 11:36:51AM -0400, Peter Xu wrote:
> On Fri, Sep 12, 2025 at 12:27:52PM +0100, Daniel P. Berrangé wrote:
> > On Thu, Sep 11, 2025 at 05:23:54PM -0400, Peter Xu wrote:
> > > No issue I hit, the change is only from code observation when I am looking
> > > at a TLS premature termination issue.
> > > 
> > > qio_channel_tls_bye() API needs to be synchronous.  When it's not, the
> > > previous impl will attach an asynchronous task retrying but only until when
> > > the channel gets the relevant GIO event. It may be problematic, because the
> > > caller of qio_channel_tls_bye() may have invoked channel close() before
> > > that, leading to premature termination of the TLS session.
> > > 
> > > Remove the asynchronous handling, instead retry it immediately.  Currently,
> > > the only two possible cases that may lead to async task is either INTERRUPT
> > > or EAGAIN.  It should be suffice to spin retry as of now, until a solid
> > > proof showing that a more complicated retry logic is needed.
> > > 
> > > With that, we can remove the whole async model for the bye task.
> > > 
> > > When at it, making the function return bool, which looks like a common
> > > pattern in QEMU when errp is used.
> > > 
> > > Side note on the tracepoints: previously the tracepoint bye_complete()
> > > isn't used.  Start to use it in this patch.  bye_pending() and bye_cancel()
> > > can be dropped now.  Adding bye_retry() instead.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  include/io/channel-tls.h |  5 ++-
> > >  io/channel-tls.c         | 86 +++++-----------------------------------
> > >  io/trace-events          |  3 +-
> > >  3 files changed, 15 insertions(+), 79 deletions(-)
> > > 

> > > diff --git a/io/channel-tls.c b/io/channel-tls.c
> > > index 5a2c8188ce..8510a187a8 100644
> > > --- a/io/channel-tls.c
> > > +++ b/io/channel-tls.c
> > > @@ -253,84 +253,25 @@ 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)
> > > +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp)
> > >  {
> > > -    GIOCondition condition;
> > > -    QIOChannelTLSData *data;
> > >      int status;
> > > -    Error *err = NULL;
> > >  
> > > -    status = qcrypto_tls_session_bye(ioc->session, &err);
> > > +    trace_qio_channel_tls_bye_start(ioc);
> > > +retry:
> > > +    status = qcrypto_tls_session_bye(ioc->session, errp);
> > >  
> > >      if (status < 0) {
> > >          trace_qio_channel_tls_bye_fail(ioc);
> > 
> > snip
> > 
> > > +        return false;
> > > +    } else if (status != QCRYPTO_TLS_BYE_COMPLETE) {
> > > +        /* BYE event must be synchronous, retry immediately */
> > > +        trace_qio_channel_tls_bye_retry(ioc, status);
> > > +        goto retry;
> > >      }
> > 
> > We cannot do this. If the gnutls_bye() API needs to perform
> > socket I/O, and so when we're running over a non-blocking
> > socket we must expect EAGAIN. With this handling, QEMU will
> > busy loop burning 100% CPU when the socket is not ready.
> 
> Right.  That was the plan when drafting this, the hope is spinning will
> almost not happen, and even if it happens it'll finish soon (migration is
> completing, it means network mustn't be so bad), unless the network is
> stuck exactly at when we send the bye().

Don't forget that the machine can be running 5 migrations
of different VMs concurrently, and so may not be as quick
to finish sending traffic as we expect. Since QEMU's mig
protocol is essentially undirectional, I wonder if the
send buffer could still be full of VMstate data waiting
to be sent ? Perhaps its fine, but I don't like relying
on luck, or hard-to-prove scenarios.

> > A second point is that from a QIOChannel POV, we need to
> > ensure that all APIs can be used in a non-blocking scenario.
> > This is why in the QIOChannelSocket impl connect/listen APIs
> > we provide both _sync and _async variants of the APIs, or
> > in the QIOChannelTLS impl, the handshake API is always
> > async with a callback to be invokved on completion.
> 
> I agree.  The issue is if so, migration code needs to be always be prepared
> with a possible async op even if in 99.9999% cases it won't happen... we
> need to complicate the multifd logic a lot for this, but the gain is
> little..
> 
> This series still used patch 1 to fix the problem (rather than do real BYE
> on preempt channels, for example) only because it's the easiest, after all
> it's still a contract in tls channel impl to allow premature termination
> for explicit shutdown()s on the host.
> 
> If we want to do 100% graceful shutdowns, we'll need to apply this to all
> channels, and the async-possible model can definitely add more complexity
> more than multifd.  I hope it won't be necessary.. but just to mention it.

Even if the migration code is relying on non-blocking sockets
for most of its work, at the time we're ready to invoke "bye",
perhaps the migration code could simply call

 qio_channel_set_blocking(ioc, true)

to switch the socket over to blocking mode.


> > The QIOChanel 'bye' method is flawed in that it is
> > asynchronous, but has no callback for completion.
> > 
> > If migration is /always/ using a blocking socket for the
> > TLS channels this isn't a problem as gnutls will complete
> > immediately, but if any async sockets are used we have no
> > way to wait for completion. This requires improving the
> > API design in some manner.
> 
> I recall one of your future series on TLS would start to enable async for
> all channels?  In all cases, we definitely don't want to have this call to
> be relevant to the blocking mode of the channels.
> 
> Would it make sense to introduce a _sync() version of it, but keep the
> original bye(), leaving the rest until a real async user appears?
> 
> I can also at least drop this patch as of now, because we can still wish it
> almost always be synchronous.  However we have risk forgetting that forever
> and hit it a few years later..

If we leave the current code as-is, and relying on migration switching
to blocking mode first before calling bye, we'll be ok

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: [PATCH v2 2/3] io/tls: Make qio_channel_tls_bye() always synchronous

[Peter Xu]

On Mon, Sep 15, 2025 at 07:40:33PM +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 12, 2025 at 11:36:51AM -0400, Peter Xu wrote:
> > On Fri, Sep 12, 2025 at 12:27:52PM +0100, Daniel P. Berrangé wrote:
> > > On Thu, Sep 11, 2025 at 05:23:54PM -0400, Peter Xu wrote:
> > > > No issue I hit, the change is only from code observation when I am looking
> > > > at a TLS premature termination issue.
> > > > 
> > > > qio_channel_tls_bye() API needs to be synchronous.  When it's not, the
> > > > previous impl will attach an asynchronous task retrying but only until when
> > > > the channel gets the relevant GIO event. It may be problematic, because the
> > > > caller of qio_channel_tls_bye() may have invoked channel close() before
> > > > that, leading to premature termination of the TLS session.
> > > > 
> > > > Remove the asynchronous handling, instead retry it immediately.  Currently,
> > > > the only two possible cases that may lead to async task is either INTERRUPT
> > > > or EAGAIN.  It should be suffice to spin retry as of now, until a solid
> > > > proof showing that a more complicated retry logic is needed.
> > > > 
> > > > With that, we can remove the whole async model for the bye task.
> > > > 
> > > > When at it, making the function return bool, which looks like a common
> > > > pattern in QEMU when errp is used.
> > > > 
> > > > Side note on the tracepoints: previously the tracepoint bye_complete()
> > > > isn't used.  Start to use it in this patch.  bye_pending() and bye_cancel()
> > > > can be dropped now.  Adding bye_retry() instead.
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  include/io/channel-tls.h |  5 ++-
> > > >  io/channel-tls.c         | 86 +++++-----------------------------------
> > > >  io/trace-events          |  3 +-
> > > >  3 files changed, 15 insertions(+), 79 deletions(-)
> > > > 
> 
> > > > diff --git a/io/channel-tls.c b/io/channel-tls.c
> > > > index 5a2c8188ce..8510a187a8 100644
> > > > --- a/io/channel-tls.c
> > > > +++ b/io/channel-tls.c
> > > > @@ -253,84 +253,25 @@ 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)
> > > > +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp)
> > > >  {
> > > > -    GIOCondition condition;
> > > > -    QIOChannelTLSData *data;
> > > >      int status;
> > > > -    Error *err = NULL;
> > > >  
> > > > -    status = qcrypto_tls_session_bye(ioc->session, &err);
> > > > +    trace_qio_channel_tls_bye_start(ioc);
> > > > +retry:
> > > > +    status = qcrypto_tls_session_bye(ioc->session, errp);
> > > >  
> > > >      if (status < 0) {
> > > >          trace_qio_channel_tls_bye_fail(ioc);
> > > 
> > > snip
> > > 
> > > > +        return false;
> > > > +    } else if (status != QCRYPTO_TLS_BYE_COMPLETE) {
> > > > +        /* BYE event must be synchronous, retry immediately */
> > > > +        trace_qio_channel_tls_bye_retry(ioc, status);
> > > > +        goto retry;
> > > >      }
> > > 
> > > We cannot do this. If the gnutls_bye() API needs to perform
> > > socket I/O, and so when we're running over a non-blocking
> > > socket we must expect EAGAIN. With this handling, QEMU will
> > > busy loop burning 100% CPU when the socket is not ready.
> > 
> > Right.  That was the plan when drafting this, the hope is spinning will
> > almost not happen, and even if it happens it'll finish soon (migration is
> > completing, it means network mustn't be so bad), unless the network is
> > stuck exactly at when we send the bye().
> 
> Don't forget that the machine can be running 5 migrations
> of different VMs concurrently, and so may not be as quick
> to finish sending traffic as we expect. Since QEMU's mig
> protocol is essentially undirectional, I wonder if the
> send buffer could still be full of VMstate data waiting
> to be sent ? Perhaps its fine, but I don't like relying
> on luck, or hard-to-prove scenarios.

Very rare conditional spinning is, IMHO, totally OK.  I wished all our
problems are as simple as cpu spinning.. then it's super straightforward to
debug when hit, and also benign.  We can add whatever smart tech after that.

I would just guess even if with this patch goes in, we will never observe
it considering the write buffer shouldn't be more than tens of MBs..

Said that..

> 
> > > A second point is that from a QIOChannel POV, we need to
> > > ensure that all APIs can be used in a non-blocking scenario.
> > > This is why in the QIOChannelSocket impl connect/listen APIs
> > > we provide both _sync and _async variants of the APIs, or
> > > in the QIOChannelTLS impl, the handshake API is always
> > > async with a callback to be invokved on completion.
> > 
> > I agree.  The issue is if so, migration code needs to be always be prepared
> > with a possible async op even if in 99.9999% cases it won't happen... we
> > need to complicate the multifd logic a lot for this, but the gain is
> > little..
> > 
> > This series still used patch 1 to fix the problem (rather than do real BYE
> > on preempt channels, for example) only because it's the easiest, after all
> > it's still a contract in tls channel impl to allow premature termination
> > for explicit shutdown()s on the host.
> > 
> > If we want to do 100% graceful shutdowns, we'll need to apply this to all
> > channels, and the async-possible model can definitely add more complexity
> > more than multifd.  I hope it won't be necessary.. but just to mention it.
> 
> Even if the migration code is relying on non-blocking sockets
> for most of its work, at the time we're ready to invoke "bye",
> perhaps the migration code could simply call
> 
>  qio_channel_set_blocking(ioc, true)
> 
> to switch the socket over to blocking mode.

... I think this is a good idea and should solve the problem indeed.  I
hope there's no loophole that it could still trigger the async path.

I'll respin with that, thanks!

-- 
Peter Xu

Re: [PATCH v2 2/3] io/tls: Make qio_channel_tls_bye() always synchronous

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> No issue I hit, the change is only from code observation when I am looking
> at a TLS premature termination issue.
>
> qio_channel_tls_bye() API needs to be synchronous.  When it's not, the
> previous impl will attach an asynchronous task retrying but only until when
> the channel gets the relevant GIO event. It may be problematic, because the
> caller of qio_channel_tls_bye() may have invoked channel close() before
> that, leading to premature termination of the TLS session.
>

I'm not super versed on socket APIs, so bear with me: Wouldn't the
subsequent shutdown() before close() ensure that the io watch gets
triggered? Assuming we're atomically installing the watch before the
shutdown() (at the moment, we're not).

> Remove the asynchronous handling, instead retry it immediately.  Currently,
> the only two possible cases that may lead to async task is either INTERRUPT
> or EAGAIN.  It should be suffice to spin retry as of now, until a solid
> proof showing that a more complicated retry logic is needed.
>
> With that, we can remove the whole async model for the bye task.
>

With the bye() being synchronous, do we still have the issue when
migration fails? I guess it depends on what the answer to my question
above is...

> When at it, making the function return bool, which looks like a common
> pattern in QEMU when errp is used.
>
> Side note on the tracepoints: previously the tracepoint bye_complete()
> isn't used.  Start to use it in this patch.  bye_pending() and bye_cancel()
> can be dropped now.  Adding bye_retry() instead.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/io/channel-tls.h |  5 ++-
>  io/channel-tls.c         | 86 +++++-----------------------------------
>  io/trace-events          |  3 +-
>  3 files changed, 15 insertions(+), 79 deletions(-)
>
> diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
> index 7e9023570d..bcd14ffbd6 100644
> --- a/include/io/channel-tls.h
> +++ b/include/io/channel-tls.h
> @@ -49,7 +49,6 @@ struct QIOChannelTLS {
>      QCryptoTLSSession *session;
>      QIOChannelShutdown shutdown;
>      guint hs_ioc_tag;
> -    guint bye_ioc_tag;
>  };
>  
>  /**
> @@ -60,8 +59,10 @@ struct QIOChannelTLS {
>   * Perform the TLS session termination. This method will return
>   * immediately and the termination will continue in the background,
>   * provided the main loop is running.
> + *
> + * Returns: true on success, false on error (with errp set)
>   */
> -void qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp);
> +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp);
>  
>  /**
>   * qio_channel_tls_new_server:
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 5a2c8188ce..8510a187a8 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -253,84 +253,25 @@ 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)
> +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp)
>  {
> -    GIOCondition condition;
> -    QIOChannelTLSData *data;
>      int status;
> -    Error *err = NULL;
>  
> -    status = qcrypto_tls_session_bye(ioc->session, &err);
> +    trace_qio_channel_tls_bye_start(ioc);
> +retry:
> +    status = qcrypto_tls_session_bye(ioc->session, errp);
>  
>      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;
> +    } else if (status != QCRYPTO_TLS_BYE_COMPLETE) {
> +        /* BYE event must be synchronous, retry immediately */
> +        trace_qio_channel_tls_bye_retry(ioc, status);
> +        goto retry;
>      }
>  
> -    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);
> +    trace_qio_channel_tls_bye_complete(ioc);
> +    return true;
>  }
>  
>  static void qio_channel_tls_init(Object *obj G_GNUC_UNUSED)
> @@ -482,11 +423,6 @@ 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 dc3a63ba1f..67b3814192 100644
> --- a/io/trace-events
> +++ b/io/trace-events
> @@ -45,10 +45,9 @@ 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_retry(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"

Re: [PATCH v2 2/3] io/tls: Make qio_channel_tls_bye() always synchronous

[Peter Xu]

On Thu, Sep 18, 2025 at 11:47:00AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > No issue I hit, the change is only from code observation when I am looking
> > at a TLS premature termination issue.
> >
> > qio_channel_tls_bye() API needs to be synchronous.  When it's not, the
> > previous impl will attach an asynchronous task retrying but only until when
> > the channel gets the relevant GIO event. It may be problematic, because the
> > caller of qio_channel_tls_bye() may have invoked channel close() before
> > that, leading to premature termination of the TLS session.
> >
> 
> I'm not super versed on socket APIs, so bear with me: Wouldn't the
> subsequent shutdown() before close() ensure that the io watch gets
> triggered? Assuming we're atomically installing the watch before the
> shutdown() (at the moment, we're not).

I think it won't.

First of all, AFAIU migration_cleanup() must be run in the main thread,
because it can register async tasks like the bye() task, and it registers
against context==NULL (in qio_channel_tls_bye_task(), for example), which I
believe means it'll be registered against the QEMU main loop.

Then, if we do a sequence of this:

  qio_channel_tls_bye()
  shutdown()
  close()

And if we do not yield anywhere within the process, IIUC it means it'll run
in sequence _without_ processing any events on the main loop even if some
events triggered.

So.. I think the close() will see the async task and remove it, never get a
chance to kick it off.

> 
> > Remove the asynchronous handling, instead retry it immediately.  Currently,
> > the only two possible cases that may lead to async task is either INTERRUPT
> > or EAGAIN.  It should be suffice to spin retry as of now, until a solid
> > proof showing that a more complicated retry logic is needed.
> >
> > With that, we can remove the whole async model for the bye task.
> >
> 
> With the bye() being synchronous, do we still have the issue when
> migration fails? I guess it depends on what the answer to my question
> above is...

When migration fails, IMHO it's fine to prematurely terminate the channels,
as I replied to one email that you commented on v1.  But we can discuss, I
am not sure if I missed things alone the lines.

Note, Dan suggested me to change the channel blocking mode as a smaller and
quicker fix, instead of throwing async model away, which seems to be
preferred to keep for any iochannel APIs.  So feel free to ignore this
patch too as of now.  I'll still need to investigate a bit on what would
happen if a concurrent update of fd nonblocking would affect other threads,
though.  In all cases, all results will be reflected in v3, but likely this
patch will be either dropped or replaced.

I know I let you read some of the things that we already planned to throw
away, my apologies. But it's partly your "fault" (to take holidays!). No,
I'm joking. :) It's still good to discuss these.

-- 
Peter Xu

[PATCH v2 3/3] migration: Make migration_has_failed() work even for CANCELLING

[Peter Xu]

No issue I hit, the change is only from code observation when I am looking
at a TLS premature termination issue.

We set CANCELLED very late, it means migration_has_failed() may not work
correctly if it's invoked before updating CANCELLING to CANCELLED.

Allow that state will make migration_has_failed() working as expected even
if it's invoked slightly earlier.

One current user is the multifd code for the TLS graceful termination,
where it's before updating to CANCELLED.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 10c216d25d..f6f6a6e202 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1702,7 +1702,8 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
 
 bool migration_has_failed(MigrationState *s)
 {
-    return (s->state == MIGRATION_STATUS_CANCELLED ||
+    return (s->state == MIGRATION_STATUS_CANCELLING ||
+            s->state == MIGRATION_STATUS_CANCELLED ||
             s->state == MIGRATION_STATUS_FAILED);
 }
 
-- 
2.50.1

Re: [PATCH v2 3/3] migration: Make migration_has_failed() work even for CANCELLING

[Juraj Marcin]

On 2025-09-11 17:23, Peter Xu wrote:
> No issue I hit, the change is only from code observation when I am looking
> at a TLS premature termination issue.
> 
> We set CANCELLED very late, it means migration_has_failed() may not work
> correctly if it's invoked before updating CANCELLING to CANCELLED.
> 
> Allow that state will make migration_has_failed() working as expected even
> if it's invoked slightly earlier.
> 
> One current user is the multifd code for the TLS graceful termination,
> where it's before updating to CANCELLED.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Juraj Marcin <jmarcin@redhat.com>

Re: [PATCH v2 3/3] migration: Make migration_has_failed() work even for CANCELLING

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> No issue I hit, the change is only from code observation when I am looking
> at a TLS premature termination issue.
>
> We set CANCELLED very late, it means migration_has_failed() may not work
> correctly if it's invoked before updating CANCELLING to CANCELLED.
>
> Allow that state will make migration_has_failed() working as expected even
> if it's invoked slightly earlier.
>
> One current user is the multifd code for the TLS graceful termination,
> where it's before updating to CANCELLED.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>