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

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

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