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

["Daniel P. Berrangé" <berrange@redhat.com>]

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