Re: [PATCH v4 08/33] migration/multifd: Allow premature EOF on TLS incoming channels

[Peter Xu <peterx@redhat.com>]

On Mon, Feb 03, 2025 at 10:41:32PM +0100, Maciej S. Szmigiero wrote:
> On 3.02.2025 21:20, Peter Xu wrote:
> > On Mon, Feb 03, 2025 at 07:53:00PM +0100, Maciej S. Szmigiero wrote:
> > > On 3.02.2025 19:20, Peter Xu wrote:
> > > > On Thu, Jan 30, 2025 at 11:08:29AM +0100, Maciej S. Szmigiero wrote:
> > > > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> > > > > 
> > > > > Multifd send channels are terminated by calling
> > > > > qio_channel_shutdown(QIO_CHANNEL_SHUTDOWN_BOTH) in
> > > > > multifd_send_terminate_threads(), which in the TLS case essentially
> > > > > calls shutdown(SHUT_RDWR) on the underlying raw socket.
> > > > > 
> > > > > Unfortunately, this does not terminate the TLS session properly and
> > > > > the receive side sees this as a GNUTLS_E_PREMATURE_TERMINATION error.
> > > > > 
> > > > > The only reason why this wasn't causing migration failures is because
> > > > > the current migration code apparently does not check for migration
> > > > > error being set after the end of the multifd receive process.
> > > > > 
> > > > > However, this will change soon so the multifd receive code has to be
> > > > > prepared to not return an error on such premature TLS session EOF.
> > > > > Use the newly introduced QIOChannelTLS method for that.
> > > > > 
> > > > > It's worth noting that even if the sender were to be changed to terminate
> > > > > the TLS connection properly the receive side still needs to remain
> > > > > compatible with older QEMU bit stream which does not do this.
> > > > 
> > > > If this is an existing bug, we could add a Fixes.
> > > 
> > > It is an existing issue but only uncovered by this patch set.
> > > 
> > > As far as I can see it was always there, so it would need some
> > > thought where to point that Fixes tag.
> > 
> > If there's no way to trigger a real functional bug anyway, it's also ok we
> > omit the Fixes.
> > 
> > > > Two pure questions..
> > > > 
> > > >     - What is the correct way to terminate the TLS session without this flag?
> > > 
> > > I guess one would need to call gnutls_bye() like in this GnuTLS example:
> > > https://gitlab.com/gnutls/gnutls/-/blob/2b8c3e4c71ad380bbbffb32e6003b34ecad596e3/doc/examples/ex-client-anon.c#L102
> > > 
> > > >     - Why this is only needed by multifd sessions?
> > > 
> > > What uncovered the issue was switching the load threads to using
> > > migrate_set_error() instead of their own result variable
> > > (load_threads_ret) which you had requested during the previous
> > > patch set version review:
> > > https://lore.kernel.org/qemu-devel/Z1DbH5fwBaxtgrvH@x1n/
> > > 
> > > Turns out that the multifd receive code always returned
> > > error in the TLS case, just nothing was previously checking for
> > > that error presence.
> > 
> > What I was curious is whether this issue also exists for the main migration
> > channel when with tls, especially when e.g. multifd not enabled at all.  As
> > I don't see anywhere that qemu uses gnutls_bye() for any tls session.
> > 
> > I think it's a good to find that we overlooked this before.. and IMHO it's
> > always good we could fix this.
> > 
> > Does it mean we need proper gnutls_bye() somewhere?
> > 
> > If we need an explicit gnutls_bye(), then I wonder if that should be done
> > on the main channel as well.
> 
> That's a good question and looking at the code qemu_loadvm_state_main() exits
> on receiving "QEMU_VM_EOF" section (that's different from receiving socket EOF)
> and then optionally "QEMU_VM_VMDESCRIPTION" section is read with explicit size
> in qemu_loadvm_state() - so still not until channel EOF.

I had a closer look, I do feel like such pre-mature termination is caused
by explicit shutdown()s of the iochannels, looks like that can cause issue
even after everything is sent.  Then I noticed indeed multifd sender
iochannels will get explicit shutdown()s since commit 077fbb5942, while we
don't do that for the main channel.  Maybe that is a major difference.

Now I wonder whether we should shutdown() the channel at all if migration
succeeded, because looks like it can cause tls session to interrupt even if
the shutdown() is done after sent everything, and if so it'll explain why
you hit the issue with tls.

> 
> Then I can't see anything else reading the channel until it is closed in
> migration_incoming_state_destroy().
> 
> So most likely the main migration channel will never read far enough to
> reach that GNUTLS_E_PREMATURE_TERMINATION error.
> 
> > If we don't need gnutls_bye(), then should we always ignore pre-mature
> > termination of tls no matter if it's multifd or non-multifd channel (or
> > even a tls session that is not migration-related)?
> 
> So basically have this patch extended to calling
> qio_channel_tls_set_premature_eof_okay() also on the main migration channel?

If above theory can stand, then eof-okay could be a workaround papering
over the real problem that we shouldn't always shutdown()..

Could you have a look at below patch and see whether it can fix the problem
you hit too, in replace of these two patches (including the previous
iochannel change)?

Thanks,

===8<===
From 3147084174b0e0bda076ad205ae139f8fc433892 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Mon, 3 Feb 2025 17:27:45 -0500
Subject: [PATCH] migration: Avoid shutdown multifd channels if migration
 succeeded
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Multifd channels behave differently from the main channel when shutting
down: the sender side will always shutdown() on the multifd iochannels, no
matter whether migration succeeded or not.  QEMU doesn't do that on src.

Such behavior was introduced in commit 077fbb5942 ("multifd: Shut down the
QIO channels to avoid blocking the send threads when they are terminated.")
to fix a hang issue when multifd enabled.

This might be problematic though, especially in TLS context, because it
looks like such shutdown() on src (even if succeeded) could cause
destination multifd iochannels to receive pre-mature terminations of TLS
sessions.

It's debatable whether such shutdown() should be explicitly done even for a
succeeded migration.  This patch moves the shutdown() instead from
finalization phase into qmp_migrate_cancel(), so that we only do the
shutdown() when cancels, and we should avoid such when it succeeds.

When at it, keep all the shutdown() code together, moving the return path
shutdown() (which seems a bit redundant, but no harm to do) to where the
rest channels are shutdown.

Cc: Li Zhang <lizhang@suse.de>
Cc: Dr. David Alan Gilbert <dave@treblig.org>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Reported-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.h   |  1 +
 migration/migration.c | 24 +++++++++++++++++-------
 migration/multifd.c   | 14 +++++++++++---
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index bd785b9873..26ef94ac93 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -340,6 +340,7 @@ static inline void multifd_send_prepare_header(MultiFDSendParams *p)
 
 void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc);
 bool multifd_send(MultiFDSendData **send_data);
+void multifd_send_shutdown_iochannels(void);
 MultiFDSendData *multifd_send_data_alloc(void);
 
 static inline uint32_t multifd_ram_page_size(void)
diff --git a/migration/migration.c b/migration/migration.c
index 74c50cc72c..e43f8222dc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1565,13 +1565,6 @@ static void migrate_fd_cancel(MigrationState *s)
 
     trace_migrate_fd_cancel();
 
-    WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
-        if (s->rp_state.from_dst_file) {
-            /* shutdown the rp socket, so causing the rp thread to shutdown */
-            qemu_file_shutdown(s->rp_state.from_dst_file);
-        }
-    }
-
     do {
         old_state = s->state;
         if (!migration_is_running()) {
@@ -1594,6 +1587,23 @@ static void migrate_fd_cancel(MigrationState *s)
             if (s->to_dst_file) {
                 qemu_file_shutdown(s->to_dst_file);
             }
+            /*
+             * Above should work already, because the iochannel is shared
+             * between outgoing and return path qemufiles, however just to
+             * be on the safe side to set qemufile error on return path too
+             * if existed.
+             */
+            if (s->rp_state.from_dst_file) {
+                qemu_file_shutdown(s->rp_state.from_dst_file);
+            }
+        }
+
+        /*
+         * We need to shutdown multifd channels too if they are available,
+         * to make sure no multifd send threads will be stuck at syscalls.
+         */
+        if (migrate_multifd()) {
+            multifd_send_shutdown_iochannels();
         }
     }
 
diff --git a/migration/multifd.c b/migration/multifd.c
index ab73d6d984..96bcbb1e0c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -384,6 +384,17 @@ static void multifd_send_set_error(Error *err)
     }
 }
 
+void multifd_send_shutdown_iochannels(void)
+{
+    QIOChannel *c;
+    int i;
+
+    for (i = 0; i < migrate_multifd_channels(); i++) {
+        c = multifd_send_state->params[i].c;
+        qio_channel_shutdown(c, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+    }
+}
+
 static void multifd_send_terminate_threads(void)
 {
     int i;
@@ -404,9 +415,6 @@ static void multifd_send_terminate_threads(void)
         MultiFDSendParams *p = &multifd_send_state->params[i];
 
         qemu_sem_post(&p->sem);
-        if (p->c) {
-            qio_channel_shutdown(p->c, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
-        }
     }
 
     /*
-- 
2.47.0


-- 
Peter Xu