Re: [PATCH 6/7] migration: Fix incoming channel race with multifd and preempt_pre_7_2

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Sep 24, 2026 at 06:54:25PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > PROBLEM
>> > =======
>> >
>> > Migration reliably hangs with old machine types (7.2-) when these migration
>> > capabilities are enabled: multifd, postcopy-ram, postcopy-preempt.
>> >
>> > Simplest reproducer :
>> >
>> >   $ QTEST_QEMU_MACHINE_TYPE=pc-q35-7.2 \
>> >       QTEST_QEMU_BINARY=./qemu-system-x86_64 \
>> >       ./tests/qtest/migration-test --full \
>> >       -r /x86_64/migration/multifd+postcopy/preempt/plain
>> >
>> > Current workaround is to kill destination QEMU, cancel the migration on
>> > source (with/without yank), disable one or more of the features, and retry.
>> >
>> > The hang is caused by receiving preempt channel before most of the multifd
>> > channels, the peek buffer operation will stuck forever.
>> >
>> > Incoming side QEMU hang (with relevant tracepoints enabled):
>> >
>> >   migrate_set_state new state setup
>> >   migration_socket_incoming_accepted
>> >   migration_set_incoming_channel ioc=0x557d82c1caa0 ioctype=qio-channel-socket
>> >   migration_socket_incoming_accepted
>> >   migration_set_incoming_channel ioc=0x557d83023840 ioctype=qio-channel-socket
>> >   multifd_recv_new_channel channel 1
>> >   migration_socket_incoming_accepted
>> >   migration_set_incoming_channel ioc=0x557d830c08c0 ioctype=qio-channel-socket
>> >   multifd_recv_new_channel channel 0
>> >   multifd_recv_thread_start 1
>> >   migration_socket_incoming_accepted
>> >   migration_set_incoming_channel ioc=0x557d8284a010 ioctype=qio-channel-socket
>> >   multifd_recv_thread_start 0
>> >   multifd_recv_unfill channel 0 packet_num 0 flags 0x1 next packet size 0
>> >   multifd_recv_unfill channel 1 packet_num 1 flags 0x1 next packet size 0
>> >   [dest qemu hangs here]
>> >
>> > Incoming side backtrace:
>> >
>> >   __syscall_cancel_arch
>> >   __internal_syscall_cancel
>> >   __syscall_cancel
>> >   recvmsg
>> >   qio_channel_socket_readv
>> >   qio_channel_readv_full
>> >   migration_channel_read_peek
>> >   migration_channel_identify
>> >   migration_channel_process_incoming
>> >   socket_accept_incoming_migration
>> >   qio_net_listener_channel_func
>> >   qio_channel_fd_source_dispatch
>> >   g_main_context_dispatch_unlocked.lto_priv
>> >   g_main_context_dispatch
>> >   glib_pollfds_poll
>> >   os_host_main_loop_wait
>> >   main_loop_wait
>> >   qemu_main_loop
>> >   qemu_default_main
>> >   main
>> >
>> > This patch tries to fix it, from incoming side first.  This patch alone
>> > should enable single-direction migration to this new QEMU.  The next patch
>> > will propose a fix for the outgoing path, fixing new->old migrations.
>> >
>> > BACKGROUND
>> > ==========
>> >
>> > preempt_pre_7_2 used to be a flag marking QEMU releases 7.1/7.2 only
>> > because they have a flaw on channel establishment on preempt channel.
>> >
>> > Now, after we have multifd + postcopy support, new QEMU binaries can
>> > migrate old machine types with both features enabled.  Juraj reported
>> > constant migration hang at the very beginning for those old machine types
>> > in our downstream systems with such setup.  Juraj found it's because of
>> > this flag.
>> >
>> > QEMU source initiates channels in this order on current master when both
>> > features enabled with postcopy-preempt mode:
>> >
>> >   - First, main channel connect() happens,
>> >   - Second, postcopy_preempt_setup() invokes connect() for preempt channel,
>> >   - Last, in migration_thread(), multifd_send_setup() invokes connect()s
>> >     for all the multifd channels
>> >
>> > However on an old QEMU, it expect the channels arrive in order of
>> > "main"->"multifd"->"preempt".  Even on a new QEMU, the destination expects
>> > the same order of arrival on the channels when the channels are not
>> > peekable.
>> >
>> > For peekable channels (non-tls sockets), this time we'll likely see preempt
>> > channel appearing very soon, peeking at it causing main thread hang, like
>> > mentioned in the PROBLEM section above.
>> >
>> > SOLUTION
>> > ========
>> >
>> > One major problem of current hang is we try to peek at preempt channel,
>> > which offers nothing at the beginning.
>> >
>> > Since we'll need to accept stream from old binaries, arrival of preempt
>> > channel early is unavoidable.
>> >
>> > What we can do, though, is to allow concurrent monitoring of all channels
>> > rather than one.  Meanwhile, we shouldn't peek at anything that has no
>> > data.
>> >
>> > migration_channel_read_peek() almost does the wait, but it's too late.
>> > This patch proposes to watch all channels from the start without reading at
>> > them at all, relying on an IO watch attached.  This will make sure (1) for
>> > preempt channel, we never read on it (watch can be gracefully disarmed when
>> > we know it's preempt channel later) (2) since we have one IO watch for each
>> > channel, then we can allow accept() to happen in almost any order, process
>> > only when there's some header to read.
>> >
>> > To maintain these watches, a new GArray @channels_early is introduced, with
>> > each element to be a (ioc, GSource) tuple (@MigEarlyIncomingChannel), so
>> > that QEMU can manage all the IO watches later with it.  Here I used GSource
>> > rather than io_tag because the GSource may or may not be attached to the
>> > default main thread gcontext: when OOB is used, it may be the iothread
>> > context instead.  IOW, g_source_remove() on io_tags stops working, which
>> > only works for default main context.
>> >
>> > The trick this patch used is when we have the ability to wait on all
>> > channel headers at the same time, we pick one by one from main and multifd
>> > channels (since both of the types will contain headers dumped right at the
>> > very beginning), leave preempt channel alone.  It means, after dest QEMU
>> > finds all main and multifd channels, there are three possibilities:
>> >
>> >   - If preempt not enabled, we finished full channel establishment, done.
>> >
>> >   - If preempt enabled,
>> >
>> >     - If we have one registered IO watch with the IOC, it must be the
>> >       preempt channel,
>> >
>> >     - If we have no further IO watch, wait for the next accept(), which
>> >       must be the preempt channel.
>> >
>> > This will make channel establish work on incoming side even with the old
>> > QEMU binaries.
>> >
>> > While working on it, I managed to refactor the peeking logic quite a bit:
>> > we used to have a very convoluted migration_channel_identify() function,
>> > taking care of two different cases:
>> >
>> > (1) when the channel is peekable (like normal socket channels),
>> > (2) when the channel is not peekable (like TLS or file migrations).
>> >
>> > This patch refactored that part to make it crystal clear on how we make
>> > decisions on which channel is which, and split the peek/identify paths.
>> >
>> > One example incoming migration from pc-q35-7.2 after this patch applied,
>> > and with relevant tracepoints set (8 multifd channels, 1 preempt channel):
>> >
>> >   migrate_set_state new state setup
>> >   migration_channel_process_incoming ioc=0x557dc8af4e20 ioctype=qio-channel-socket
>> >   migration_incoming_channel_watch ioc=0x557dc8af4e20 ioctype=qio-channel-socket
>> >   migration_channel_process_incoming ioc=0x557dc8abc5d0 ioctype=qio-channel-socket
>> >   migration_incoming_channel_watch ioc=0x557dc8abc5d0 ioctype=qio-channel-socket
>> >   migration_channel_process_incoming ioc=0x557dc953d680 ioctype=qio-channel-socket
>> >   migration_incoming_channel_watch ioc=0x557dc953d680 ioctype=qio-channel-socket
>> >   migration_set_incoming_channel ioc=0x557dc8abc5d0 ioctype=qio-channel-socket type=MULTIFD
>> >   migration_channel_process_incoming ioc=0x557dc898a820 ioctype=qio-channel-socket
>> >   migration_incoming_channel_watch ioc=0x557dc898a820 ioctype=qio-channel-socket
>> >   migration_set_incoming_channel ioc=0x557dc953d680 ioctype=qio-channel-socket type=MULTIFD
>> >   migration_channel_process_incoming ioc=0x557dc919bd70 ioctype=qio-channel-socket
>> >   migration_incoming_channel_watch ioc=0x557dc919bd70 ioctype=qio-channel-socket
>> >   migration_set_incoming_channel ioc=0x557dc898a820 ioctype=qio-channel-socket type=MULTIFD
>> >   migration_channel_process_incoming ioc=0x557dc8734b20 ioctype=qio-channel-socket
>> >   migration_incoming_channel_watch ioc=0x557dc8734b20 ioctype=qio-channel-socket
>> >   migration_set_incoming_channel ioc=0x557dc919bd70 ioctype=qio-channel-socket type=MULTIFD
>> >   migration_channel_process_incoming ioc=0x557dc898bdf0 ioctype=qio-channel-socket
>> >   migration_incoming_channel_watch ioc=0x557dc898bdf0 ioctype=qio-channel-socket
>> >   migration_set_incoming_channel ioc=0x557dc8734b20 ioctype=qio-channel-socket type=MULTIFD
>> >   migration_channel_process_incoming ioc=0x557dc968cfd0 ioctype=qio-channel-socket
>> >   migration_incoming_channel_watch ioc=0x557dc968cfd0 ioctype=qio-channel-socket
>> >   migration_set_incoming_channel ioc=0x557dc898bdf0 ioctype=qio-channel-socket type=MULTIFD
>> >   migration_channel_process_incoming ioc=0x557dc8c31810 ioctype=qio-channel-socket
>> >   migration_incoming_channel_watch ioc=0x557dc8c31810 ioctype=qio-channel-socket
>> >   migration_set_incoming_channel ioc=0x557dc968cfd0 ioctype=qio-channel-socket type=MULTIFD
>> >   migration_set_incoming_channel ioc=0x557dc8c31810 ioctype=qio-channel-socket type=MULTIFD
>> >   migration_set_incoming_channel ioc=0x557dc8af4e20 ioctype=qio-channel-socket type=MAIN
>> >   migration_start_incoming
>> >   migrate_set_state new state active
>> >   migration_channel_process_incoming ioc=0x557dc8966160 ioctype=qio-channel-socket
>> >   migration_set_incoming_channel ioc=0x557dc8966160 ioctype=qio-channel-socket type=PREEMPT
>> >
>> > While at it, I renamed the four channel incoming/outgoing tracepoints, make
>> > all to follow migration_[incoming|outgoing]_channel_*().  Especially for
>> > incoming, then we can enable migration_incoming_channel* for incoming
>> > channel debuggings.
>> >
>> > FINAL NOTE ON TLS
>> > =================
>> >
>> > Note that this solution isn't a magic bullet fixing everything.  TLS is not
>> > fixable with this approach because TLS isn't peekable channel, hence the
>> > watch won't help.
>> >
>> > We can have other ways to hack it to make it work, e.g. we can make TLS
>> > channels peekable too, but I don't want to pollute the TLS iochannel only
>> > for this: let's hope handshake will arrive soon so we can drop these code.
>> >
>> > For now, when TLS is involved with such old machine type (7.2-), one needs
>> > to migrate it out with any combination of features except enabling all
>> > four (multifd + TLS + postcopy + preempt) to a patched binary, after that,
>> > all combinations will start working.
>> >
>> > BugLink: https://redhat.atlassian.net/browse/RHEL-254933
>> > Fixes: e27418861288 ("migration: enable multifd and postcopy together")
>> > Reported-by: Juraj Marcin <jmarcin@redhat.com>
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  migration/channel.h    |   3 +
>> >  migration/migration.h  |  35 ++++
>> >  migration/channel.c    | 414 +++++++++++++++++++++++++++++++++++------
>> >  migration/migration.c  |  23 ++-
>> >  migration/trace-events |   7 +-
>> >  5 files changed, 421 insertions(+), 61 deletions(-)
>> 
>> Hi Peter, I think the code looks good, but there are some twists and
>> turns that I think we can get rid of. I put a fixup patch [1] at the end
>> with my suggestions and mentioned inline what I changed and why.
>
> Thanks, especially for covering the tests while suggesting.
>
> I believe I agree with many of them, I'll comment on the rest, see inline.
>
>> 
>> >
>> > diff --git a/migration/channel.h b/migration/channel.h
>> > index 678712f291..40bd339a3e 100644
>> > --- a/migration/channel.h
>> > +++ b/migration/channel.h
>> > @@ -49,4 +49,7 @@ bool migration_channel_parse_input(const char *uri,
>> >                                     MigrationChannel **main_channelp,
>> >                                     MigrationChannel **cpr_channelp,
>> >                                     Error **errp);
>> > +
>> > +void migration_incoming_free_early_channels(MigrationIncomingState *mis);
>> > +
>> >  #endif
>> > diff --git a/migration/migration.h b/migration/migration.h
>> > index 683bc7bdd5..6f23e1805b 100644
>> > --- a/migration/migration.h
>> > +++ b/migration/migration.h
>> > @@ -90,6 +90,28 @@ typedef enum {
>> >      PREEMPT_THREAD_QUIT,
>> >  } PreemptThreadStatus;
>> >  
>> > +typedef struct {
>> > +    /*
>> > +     * The GSource we have allocated to track G_IO_IN events for the
>> > +     * channel.  Note: it may not be attached to the default main gcontext,
>> > +     * for example, when in case of io watch created during an OOB
>> > +     * migrate-recover command.
>> > +     */
>> > +    GSource *source;
>> > +    /* The channel we will identify later after getting some buffers to read */
>> > +    QIOChannel *ioc;
>> > +} MigEarlyIncomingChannel;
>> > +
>> > +typedef struct {
>> > +    GArray *channels;
>> > +    /*
>> > +     * In most cases, channels are only operated with BQl, but since we
>> > +     * have OOB command support, don't assume it, just use a standalone
>> > +     * mutex to protect any access to the channels array.
>> > +     */
>> > +    QemuMutex mutex;
>> > +} MigEarlyIncomingChannels;
>> > +
>> >  /* State for the incoming migration */
>> >  struct MigrationIncomingState {
>> >      QEMUFile *from_src_file;
>> > @@ -255,6 +277,19 @@ struct MigrationIncomingState {
>> >  
>> >      /* Do exit on incoming migration failure */
>> >      bool exit_on_error;
>> > +
>> > +    /*
>> > +     * Maintains all channels only at early stage.  After incoming
>> > +     * migration started, this should be an empty array and no use anymore.
>> > +     *
>> > +     * TODO: we logically only need this to make new QEMU binaries work for
>> > +     * old machines who have @preempt_pre_7_2 set on the source, but it's
>> > +     * also good to have this early stage anyway, making sure when peek the
>> > +     * channel there will be something in the channel.  Ideally, we should
>> > +     * manage all migration channels in the future with proper headers,
>> > +     * then we can drop this.
>> > +     */
>> > +    MigEarlyIncomingChannels channels_early;
>> >  };
>> >  
>> >  MigrationIncomingState *migration_incoming_get_current(void);
>> > diff --git a/migration/channel.c b/migration/channel.c
>> > index b9065efd03..acb7ef5206 100644
>> > --- a/migration/channel.c
>> > +++ b/migration/channel.c
>> > @@ -12,6 +12,8 @@
>> >  
>> >  #include "qemu/osdep.h"
>> >  #include "qemu/cutils.h"
>> > +#include "qemu/main-loop.h"
>> > +#include "qemu/error-report.h"
>> >  #include "channel.h"
>> >  #include "exec.h"
>> >  #include "fd.h"
>> > @@ -146,61 +148,84 @@ bool migration_has_all_channels(void)
>> >      return true;
>> >  }
>> >  
>> > -static MigChannelType migration_channel_identify(MigrationIncomingState *mis,
>> > -                                                 QIOChannel *ioc, Error **errp)
>> > +static bool qio_channel_is_peekable(QIOChannel *ioc)
>> > +{
>> > +    return qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK);
>> > +}
>> > +
>> > +/*
>> > + * With multiple channels, it is possible that we receive channels out of
>> > + * order on destination side, causing incorrect mapping of source channels
>> > + * on destination side.
>> > + *
>> > + * When the channel is peekable (e.g. non-TLS socket channels), check
>> > + * channel MAGIC to decide type of channel.
>> > + *
>> > + * Please note this is best effort, postcopy preempt channel does not send
>> > + * any magic number so avoid it for postcopy live migration.
>> > + *
>> > + * Returns: >0 if success, ==0 (CH_NONE) if error.  If error happened,
>> > + * *errp must be set.
>> > + */
>> > +static MigChannelType migration_channel_peek(MigrationIncomingState *mis,
>> > +                                             QIOChannel *ioc,
>> > +                                             Error **errp)
>> >  {
>> >      MigChannelType channel = CH_NONE;
>> >      uint32_t channel_magic = 0;
>> >      int ret = 0;
>> >  
>> > -    if (!migration_has_main_and_multifd_channels()) {
>> > -        if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
>> > -            /*
>> > -             * With multiple channels, it is possible that we receive channels
>> > -             * out of order on destination side, causing incorrect mapping of
>> > -             * source channels on destination side. Check channel MAGIC to
>> > -             * decide type of channel. Please note this is best effort,
>> > -             * postcopy preempt channel does not send any magic number so
>> > -             * avoid it for postcopy live migration. Also tls live migration
>> > -             * already does tls handshake while initializing main channel so
>> > -             * with tls this issue is not possible.
>> > -             */
>> > -            ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
>> > -                                              sizeof(channel_magic), errp);
>> > -            if (ret != 0) {
>> > -                goto out;
>> > -            }
>> > +    assert(qio_channel_is_peekable(ioc));
>> >  
>> > -            channel_magic = be32_to_cpu(channel_magic);
>> > -            if (channel_magic == QEMU_VM_FILE_MAGIC) {
>> > -                channel = CH_MAIN;
>> > -            } else if (channel_magic == MULTIFD_MAGIC) {
>> > -                assert(migrate_multifd());
>> > -                channel = CH_MULTIFD;
>> > -            } else if (!mis->from_src_file &&
>> > -                        mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
>> > -                /* reconnect main channel for postcopy recovery */
>> > -                channel = CH_MAIN;
>> > -            } else {
>> > -                error_setg(errp, "unknown channel magic: %u", channel_magic);
>> > -            }
>> > -        } else if (mis->from_src_file && migrate_multifd()) {
>> > -            /*
>> > -             * Non-peekable channels like tls/file are processed as
>> > -             * multifd channels when multifd is enabled.
>> > -             */
>> > -            channel = CH_MULTIFD;
>> > -        } else if (!mis->from_src_file) {
>> > -            channel = CH_MAIN;
>> > -        } else {
>> > -            error_setg(errp, "non-peekable channel used without multifd");
>> > -        }
>> > +    ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
>> > +                                      sizeof(channel_magic), errp);
>> > +    if (ret != 0) {
>> > +        /* Failed */
>> > +        return channel;
>> > +    }
>> > +
>> > +    channel_magic = be32_to_cpu(channel_magic);
>> > +
>> > +    if (channel_magic == QEMU_VM_FILE_MAGIC) {
>> > +        channel = CH_MAIN;
>> > +    } else if (channel_magic == MULTIFD_MAGIC) {
>> > +        assert(migrate_multifd());
>> > +        channel = CH_MULTIFD;
>> > +    } else if (!mis->from_src_file &&
>> > +               mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
>> > +        /* reconnect main channel for postcopy recovery */
>> > +        channel = CH_MAIN;
>> 
>> This is not peeking, maybe it should be somewhere else. Not this patch's
>> fault, we can think about it some other day.
>
> If we separate "peek" from "identify", this one still falls into "peek".
>
> Per my definition, the latter "identify" only does "pick channels by order
> they arrive".  Then this doesn't belong to that. As it needs to "peek for
> at least something readable", rather than blindly rely on the order of
> arrival.
>
> But yeah, it'll be good we leave this for later regardless.
>
>> 
>> >      } else {
>> > +        error_setg(errp, "Unknown channel magic: %u", channel_magic);
>> > +    }
>> > +
>> > +    return channel;
>> > +}
>> > +
>> > +/*
>> > + * Returns: >0 if success, ==0 (CH_NONE) if error.  If error happened,
>> > + * *errp must be set.
>> > + */
>> > +static MigChannelType migration_channel_identify(MigrationIncomingState *mis,
>> > +                                                 QIOChannel *ioc, Error **errp)
>> > +{
>> > +    MigChannelType channel = CH_NONE;
>> > +
>> > +    if (migration_has_main_and_multifd_channels()) {
>> >          assert(migrate_postcopy_preempt());
>> >          channel = CH_POSTCOPY;
>> 
>> This check is the same as the one at
>> migration_channel_process_incoming(). I think we can simplify this.
>
> True, this path should be dead code when original patch applied.  I have a
> slight preference on keeping the other one instead (which I know you tend
> to think "awkward").  I'll explain below, and we'll see.
>
>> 
>> See [1].
>> 
>> > +    } else if (!mis->from_src_file) {
>> > +        channel = CH_MAIN;
>> > +    } else if (migrate_multifd()) {
>> > +        /*
>> > +         * Non-peekable channels like tls/file are processed as
>> > +         * multifd channels when multifd is enabled.
>> > +         */
>> > +        channel = CH_MULTIFD;
>> > +    } else {
>> > +        error_setg(errp, "Unexpected non-peekable channel observed");
>> >      }
>> >  
>> > -out:
>> >      return channel;
>> >  }
>> >  
>> > @@ -214,31 +239,275 @@ static void migration_incoming_error_propagate(MigrationIncomingState *mis,
>> >      }
>> >  }
>> >  
>> > +/* Must be with mis->channels_early.mutex held */
>> > +static void
>> > +migration_incoming_early_channel_insert(MigrationIncomingState *mis,
>> > +                                        QIOChannel *ioc,
>> > +                                        GSource *source)
>> > +{
>> > +    MigEarlyIncomingChannel chan = {
>> > +        .source = source,
>> > +        .ioc = ioc,
>> > +    };
>> > +
>> > +    object_ref(OBJECT(ioc));
>> > +    g_source_ref(source);
>> > +
>> > +    g_array_append_val(mis->channels_early.channels, chan);
>> > +}
>> > +
>> > +static void migration_incoming_early_channel_free(GArray *channels, int i)
>> > +{
>> > +    MigEarlyIncomingChannel *chan;
>> > +
>> > +    chan = &g_array_index(channels, MigEarlyIncomingChannel, i);
>> > +
>> > +    g_source_destroy(chan->source);
>> > +    g_source_unref(chan->source);
>> > +    object_unref(OBJECT(chan->ioc));
>> > +
>> > +    g_array_remove_index_fast(channels, i);
>> > +}
>> > +
>> > +/*
>> > + * Remove this channel from the monitoring of @channels_early array.
>> > + * Return true if found and successful, false otherwise.
>> > + */
>> > +static bool
>> > +migration_incoming_early_channel_remove(MigrationIncomingState *mis,
>> > +                                        QIOChannel *ioc)
>> > +{
>> > +    GArray *channels = mis->channels_early.channels;
>> > +    MigEarlyIncomingChannel *chan;
>> > +    int i;
>> > +
>> > +    QEMU_LOCK_GUARD(&mis->channels_early.mutex);
>> > +
>> > +    for (i = 0; i < channels->len; i++) {
>> > +        chan = &g_array_index(channels, MigEarlyIncomingChannel, i);
>> > +        if (chan->ioc != ioc) {
>> > +            continue;
>> > +        }
>> > +        migration_incoming_early_channel_free(channels, i);
>> > +        return true;
>> > +    }
>> > +
>> > +    return false;
>> > +}
>> > +
>> > +void migration_incoming_free_early_channels(MigrationIncomingState *mis)
>> > +{
>> > +    GArray *channels = mis->channels_early.channels;
>> > +
>> > +    QEMU_LOCK_GUARD(&mis->channels_early.mutex);
>> > +
>> > +    while (channels->len) {
>> > +        migration_incoming_early_channel_free(channels, 0);
>> > +    }
>> > +}
>> > +
>> > +static bool migration_incoming_setup_channel(QIOChannel *ioc,
>> > +                                             MigChannelType ch,
>> > +                                             Error **errp)
>> > +{
>> > +    trace_migration_incoming_channel_set(ioc,
>> > +                                         object_get_typename(OBJECT(ioc)),
>> > +                                         mig_channel_str[ch]);
>> > +    migration_ioc_register_yank(ioc);
>> > +    /* TODO: make this return the success status instead */
>> > +    migration_incoming_setup(ioc, ch, errp);
>> 
>> I implemented this TODO. It allows us to skip the main+mfd check after
>> starting migration because migration_incoming_setup() already returns
>> that value. See [1].
>
> Actually now looking at this patch, this TODO is so trivial.. yes I'll do
> it.
>
> But when I do it, I think it too error prone to return a value returned
> from migration_has_main_and_multifd_channels() for this function.  I
> changed it to simple true/false for success or not instead.  I'll attach a
> 2nd version of the fixup patch at the end, applying most changes you
> suggested but stick with returning true/false for this one.
>
>> 
>> > +
>> > +    return *errp == NULL;
>> > +}
>> > +
>> >  static bool migration_incoming_channel_install(MigrationIncomingState *mis,
>> >                                                 QIOChannel *ioc,
>> >                                                 Error **errp)
>> >  {
>> > -    MigChannelType ch = migration_channel_identify(mis, ioc, errp);
>> > +    MigChannelType ch;
>> > +    bool ret;
>> > +
>> > +    if (qio_channel_is_peekable(ioc)) {
>> 
>> This check is redundant, we can only arrive here via the watch and we
>> checked peekable already at migration_channel_process_incoming. See [1].
>
> Shouldn't? Because the file path uses migration_incoming_channel_install()
> too, afaict.
>
> But I see how your question came, which I'm guessing: I used this function
> in a peekable only path, so here what I can do minimum is I can move the
> migration_channel_peek() to the peekable path, then move the other path of
> migration_channel_identify() to the e.g. file path.  I'll do that.
>
>> 
>> > +        ch = migration_channel_peek(mis, ioc, errp);
>> > +    } else {
>> > +        ch = migration_channel_identify(mis, ioc, errp);
>> > +    }
>> >  
>> >      if (!ch) {
>> > -        assert(*errp);
>> >          return false;
>> >      }
>> >  
>> > -    trace_migration_set_incoming_channel(ioc,
>> > -                                         object_get_typename(OBJECT(ioc)),
>> > -                                         mig_channel_str[ch]);
>> > -    migration_ioc_register_yank(ioc);
>> > +    ret = migration_incoming_setup_channel(ioc, ch, errp);
>> > +    if (!ret) {
>> > +        return false;
>> > +    }
>> >  
>> > -    if (migration_incoming_setup(ioc, ch, errp)) {
>> > +    /* Installation succeeded, kickoff migration if needed */
>> > +    if (migration_has_main_and_multifd_channels()) {
>> >          migration_start_incoming();
>> >      }
>> >  
>> > -    if (*errp) {
>> > -        return false;
>> > +    return true;
>> > +}
>> > +
>> > +static void
>> > +migration_incoming_channel_preempt_setup(QIOChannel *ioc)
>> > +{
>> > +    assert(migrate_postcopy_preempt());
>> > +    /* Installation of preempt channel should never fail */
>> > +    migration_incoming_setup_channel(ioc, CH_POSTCOPY, &error_abort);
>> > +}
>> > +
>> > +static void
>> > +migration_incoming_detect_preempt_channel(MigrationIncomingState *mis)
>> > +{
>> > +    GArray *channels = mis->channels_early.channels;
>> > +    MigEarlyIncomingChannel *chan;
>> > +
>> > +    QEMU_LOCK_GUARD(&mis->channels_early.mutex);
>> > +
>> > +    /* When preempt mode not enabled, nothing to detect.. */
>> > +    if (!migrate_postcopy_preempt()) {
>> > +        /*
>> > +         * .. but if we found something pending, throw an error only, which
>> > +         * should not happen.  Even if it happens, resources will still be
>> > +         * released after incoming migration is completedly.
>> > +         */
>> > +        if (channels->len) {
>> > +            error_report("%s: Found %u unused channels",
>> > +                         __func__, channels->len);
>> > +        }
>> > +        return;
>> >      }
>> >  
>> > -    return true;
>> > +    /* Preempt channel hasn't yet arrived?  Process it later */
>> > +    if (!channels->len) {
>> > +        return;
>> > +    }
>> > +
>> > +    /*
>> > +     * More than one channel should never happen.. capture it in case if
>> > +     * it happens, then there's not much we can do.
>> > +     */
>> > +    if (channels->len > 1) {
>> > +        error_report("%s: Found %u unused channels, "
>> > +                     "can't identify preempt channel",
>> > +                     __func__, channels->len);
>> > +        return;
>> > +    }
>> > +
>> > +    assert(channels->len == 1);
>> > +    /* This is the preempt channel, install it directly */
>> > +    chan = &g_array_index(channels, MigEarlyIncomingChannel, 0);
>> > +    migration_incoming_channel_preempt_setup(chan->ioc);
>> > +    migration_incoming_early_channel_free(channels, 0);
>> > +}
>> > +
>> > +static gboolean migration_incoming_channel_readable(QIOChannel *ioc,
>> > +                                                    GIOCondition condition,
>> > +                                                    gpointer opaque)
>> > +{
>> > +    MigrationIncomingState *mis = opaque;
>> > +    Error *local_err = NULL;
>> > +
>> > +    /*
>> > +     * No need to monitor this channel anymore as long as anything arrived,
>> > +     * remove it from tracking.
>> > +     *
>> > +     * NOTE: this means if partial data arrived we may still block here,
>> > +     * but it shouldn't happen in production, only malicious stream.  Since
>> > +     * migration stream is trusted (either due to trusted network, or TLS),
>> > +     * that's non-issue.
>> 
>> Could we check the amount of data and keep waiting?
>
> We could, but it is about the security side of things, I'd not bother in
> this patch, and maybe we don't bother forever, hopping that at the end of
> this year we enable handshake in next release, then this is a ioc read()
> block reading, as simple as that.
>

Ok, no worries.

>> 
>> > +     *
>> > +     * NOTE2: this will also release the ioc ref that we used to hold, but
>> > +     * it's fine since we have at least one more refcount in the current
>> > +     * event handler.
>> 
>> I don't understand "one more ref in the event handler". Looks like
>> you're trying to say the refcount is consistent, but why even mention
>> it? I may be missing something.
>
> Not about consistency, it's about holding at least one refcount to make sure
> migration_incoming_early_channel_remove() won't already free the IOC.
>
> But I think maybe you're right: likely nobody will notice the context of
> this path processing the gsource, and maybe this sentence brings more
> confusion than clarity?  Let me know, I can drop this line.
>

You mean the inc at qio_channel_create_fd_watch()? Then we don't need
this comment.

>> 
>> > +     *
>> > +     * NOTE3: it's theoretically possible that this entry is gone reaching
>> > +     * here. Example: the main thread is doing incoming cleanup having this
>> > +     * one removed, while this watch can be registered on the monitor
>> > +     * iothread's context and fired at the exact same time but in the
>> > +     * iothread instead.  If it happens, skip the rest.  I'm not sure if
>> > +     * this could happen at all, may depend on iothread lifespan management
>> > +     * in the main thread, but be prepared.
>> 
>> I wonder if we should extend the locking a bit further. Not because of
>> what you mentioned here^, but because everything the code running on the
>> monitor thread touches can race with changes from the main loop. All of
>> these migrate_foo_bar() helpers, the QEMUFiles, caps, params, etc.
>
> Is that about the new lock (mis->channels_early.mutex) or existing code?
> Possible to do on top, or a new concern introduced from this patch?
>

If we can reach migration_channel_process_incoming() via OOB command,
then everything below it needs to be audited to ensure we're not
changing global state concurrently. Mostly pre-existing, but you are
adding more code inside the danger window.

>> 
>> > +     */
>> > +    if (!migration_incoming_early_channel_remove(mis, ioc)) {
>> > +        goto out;
>> > +    }
>> > +
>> 
>> I moved the peek into here and passed the already identified
>> channel into migration_incoming_channel_install(). See [1].
>
> Agree on this change, better.
>
>> 
>> > +    if (!migration_incoming_channel_install(mis, ioc, &local_err)) {
>> > +        goto out;
>> > +    }
>> > +
>> > +    if (migration_has_main_and_multifd_channels()) {
>> 
>> This check became redundant because the _setup routine already returns
>> it. See [1].
>
> I didn't adopt this in my new fixup, because I want to avoid returning that
> info deep from stack.  Let's continue the discussion in my fixup there as a
> reply.
>

I wrote a migration_can_start() routine and tried to incorporate some
preempt channel knowledge into it, but it didn't work so well. I find it
a bit opaque to look only at main+mfd channels to start the
migration. Since nothing better came up so far, I agree you keep this
here.

>> 
>> > +        /*
>> > +         * Possibilities when reaching here:
>> > +         *
>> > +         * (1) if preempt not enabled, this should be no-op, all done,
>> > +         * (2) if preempt enabled,
>> > +         *   (2.a) preempt channel arrived @channels_early, handle it now
>> > +         *   (2.b) preempt channel not arrived, to be handled in
>> > +         *         migration_channel_process_incoming() later
>> > +         */
>> > +        migration_incoming_detect_preempt_channel(mis);
>> 
>> Moved this into migration_incoming_channel_install(). See [1].
>
> Since my version used migration_incoming_channel_install() for both
> peekable and non-peekable, this path is not needed for non-peekable.
> I still left it here, assuming it's clearer it's only needed for peekable.
> But you can reply with your thought.  I hope at least we can be more
> focused with most of the rest addressed.
>

I tend to prefer having a single path and make it a noop via code than
to have two paths and use our knowledge of the code to decide what goes
where. Here we can't escape having two paths, so it's not a huge deal.

>> 
>> > +    }
>> > +
>> > +out:
>> > +    if (local_err) {
>> > +        migration_incoming_error_propagate(mis, local_err);
>> > +    }
>> > +
>> > +    /*
>> > +     * NOTE: we should have already detached the GSource, returning
>> > +     * G_SOURCE_REMOVE to be logically consistent only.
>> > +     */
>> > +    return G_SOURCE_REMOVE;
>> > +}
>> > +
>> > +static void migration_incoming_channel_watch(MigrationIncomingState *mis,
>> > +                                             QIOChannel *ioc)
>> > +{
>> > +    GMainContext *context = g_main_context_get_thread_default();
>> > +    GSource *source;
>> > +    guint io_tag;
>> > +
>> > +    trace_migration_incoming_channel_watch(ioc,
>> > +                                           object_get_typename(OBJECT(ioc)));
>> > +
>> > +    /*
>> > +     * Careful: this can be run from either the main thread or the monitor
>> > +     * iothread when migrate_recover is used with OOB=on, so we need to
>> > +     * take the lock and use the full version to specify the correct
>> > +     * context.
>> > +     */
>> > +    QEMU_LOCK_GUARD(&mis->channels_early.mutex);
>> > +
>> > +    /*
>> > +     * We should never watch an @ioc that is not peekable, because there's
>> > +     * no point.  What is worse is we lose the real order of accept()s via
>> > +     * the asynchronous IO watch operation.
>> > +     *
>> > +     * Another note is TLS channel (non-peekable) may or may not work
>> > +     * properly with IO watch due to its current .io_create_watch() impl,
>> > +     * which is another story.  Just guard both points.
>> > +     */
>> > +    assert(qio_channel_is_peekable(ioc));
>> > +    io_tag = qio_channel_add_watch_full(ioc, G_IO_IN,
>> > +                                        migration_incoming_channel_readable,
>> > +                                        mis, NULL, context);
>> > +
>> > +    /*
>> > +     * Replace this if one day qio_channel_add_watch*() API can directly
>> > +     * return the GSource*.. for now, stick with it.
>> > +     */
>> > +    source = g_main_context_find_source_by_id(context, io_tag);
>> > +    /*
>> > +     * Nothing can race with adding the IO watch, aka, concurrent removal
>> > +     * is not possible when we have the lock.  So it must be present.
>> > +     */
>> > +    assert(source);
>> > +
>> > +    migration_incoming_early_channel_insert(mis, ioc, source);
>> >  }
>> >  
>> >  /**
>> > @@ -254,13 +523,44 @@ void migration_channel_process_incoming(QIOChannel *ioc)
>> >      MigrationIncomingState *mis = migration_incoming_get_current();
>> >      Error *local_err = NULL;
>> >  
>> > -    trace_migration_channel_process_incoming(
>> > +    trace_migration_incoming_channel_process(
>> >          ioc, object_get_typename(OBJECT(ioc)));
>> >  
>> >      if (migrate_channel_requires_tls_upgrade(ioc)) {
>> >          migration_tls_channel_process_incoming(ioc, &local_err);
>> >      } else {
>> > -        migration_incoming_channel_install(mis, ioc, &local_err);
>> > +        if (migration_has_main_and_multifd_channels()) {
>> 
>> The fact that this check must come before everything is a major
>> weirdness. In fact, this is conceptually the same case as the first
>> branch in migration_channel_identify(). I therefore moved that function
>> to this point and added a comment explaining why this must come
>> first. See [1].
>
> This is one part that I left alone so far.  My reason is that preempt
> channel needs special care at least for peekable, it's because it's not
> peekable there when hitting this case.  So we need to have this separate
> path.  This is also relevant to below, where..
>

You removed the check from _identify, so it's less confusing now.

>> 
>> > +            /*
>> > +             * If all main+multifd channels are present already, this must
>> > +             * be the preempt channel.
>> > +             *
>> > +             * QEMU can't register an IO watch for it if there is only the
>> > +             * last preempt channel left, because it means the IO watch
>> > +             * will never fire and nobody will pick it up: we rely on the
>> > +             * one before the last one to pick both.
>> > +             *
>> > +             * See comment in migration_incoming_channel_readable() on
>> > +             * the migration_incoming_detect_preempt_channel() call.
>> > +             */
>> > +            migration_incoming_channel_preempt_setup(ioc);
>> > +        } else {
>> > +            /*
>> > +             * Register an IO watch for peekable channels, so that channels
>> > +             * can be accept()ed with any order.
>> > +             *
>> > +             * Non-peekable channels (file, TLS, etc.) cannot register IO
>> > +             * watch, not only because there's no data to look at to help
>> > +             * making the decision, but also because after registering we
>> > +             * will lose the real ordering we get from accept(), which is
>> > +             * still so far the only source of truth to identify a channel
>> > +             * in such case.
>> > +             */
>> > +            if (qio_channel_is_peekable(ioc)) {
>> 
>> I moved this line into migration_channel_identify(). That way we ensure
>> the ordering regarding the main+mfd check. The watch is still installed
>> at this point. See [1].
>
> ... you suggested moving this into migration_channel_identify().
>
> Personally I think it's against my purpose of split: I wanted to make
> migration_channel_identify() to only rely on order of arrival to identify
> channels, nothing else.  Then for all peekable handling, it should be
> inside the _peek() function.
>

ok

>> 
>> > +                migration_incoming_channel_watch(mis, ioc);
>> > +            } else {
>> > +                migration_incoming_channel_install(mis, ioc, &local_err);
>> 
>> This invocation of install does not check for the preempt channel after
>> it returns. Is this simply assuming that if a non-peekable channel is
>> present then there will be no preempt channel at all?
>> 
>> Or if main+TLS is the first channel, then the next channel will be
>> preempt and already enter the first branch?
>
> Yes.  It's because for non-peekable, preempt channel is only processed
> above (in my patch logic, with/without my new fixup to be attached).
>
> For peekable, preempt channel may be picked up by the 2nd last channel,
> which is not possible for non-peekable, so that is special way for peekable
> channels only.
>
>> 
>> > +            }
>> > +        }
>> >      }
>> >  
>> >      if (local_err) {
>> > @@ -270,7 +570,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
>> >  
>> >  void migration_channel_connect_outgoing(MigrationState *s, QIOChannel *ioc)
>> >  {
>> > -    trace_migration_set_outgoing_channel(ioc, object_get_typename(OBJECT(ioc)));
>> > +    trace_migration_outgoing_channel_set(ioc, object_get_typename(OBJECT(ioc)));
>> >  
>> >      if (migrate_channel_requires_tls_upgrade(ioc)) {
>> >          Error *local_err = NULL;
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index cfadcc79c2..4cc4077a76 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -334,6 +334,10 @@ void migration_object_init(void)
>> >      current_incoming->page_requested = g_tree_new(page_request_addr_cmp);
>> >  
>> >      current_incoming->exit_on_error = INMIGRATE_DEFAULT_EXIT_ON_ERROR;
>> > +    /* zero_terminated=false, clear_=true */
>> 
>> There's an extra _ after 'clear' I think.
>
> See?  I know someone else will get confused too. :)
>

hahaha crap. Put a +1 next to that comment and let's increment it every
time someone mentions this.

> https://docs.gtk.org/glib/type_func.Array.new.html
>
>     GArray*
>     g_array_new (
>         gboolean zero_terminated,
>         gboolean clear_,
>         guint element_size
>     )
>
>     ...
>
>     clear_
>     Type: gboolean
>
>     If true, GArray elements should be automatically cleared to 0 when they are allocated.
>
>> 
>> > +    current_incoming->channels_early.channels =
>> > +        g_array_new(FALSE, TRUE, sizeof(MigEarlyIncomingChannel));
>> > +    qemu_mutex_init(&current_incoming->channels_early.mutex);
>> >  
>> >      migration_object_check(current_migration, &error_fatal);
>> >  
>> > @@ -447,6 +451,24 @@ void migration_incoming_state_destroy(void)
>> >      MigrationIncomingState *mis = migration_incoming_get_current();
>> >      PostcopyState ps = postcopy_state_get();
>> >  
>> > +    /* Cleanup listener to make sure no further accept() for sockets */
>> > +    migration_incoming_transport_cleanup(mis);
>> > +
>> > +    /*
>> > +     * It's safer to free channel watches earlier than most of the rest, in
>> > +     * case the IO watches could fire in the monitor iothread concurrently
>> > +     * against this function.
>> 
>> Now I'm getting worried. Cleanup races are super annoying. This is
>> "just" about the recovery command, right?
>
> I think so.
>
> Note: I didn't have any way to trigger any crash at all, this is something
> when I had AI review my code and it pointed out this.  I thought I should
> be safe than sorry.
>

It will happen in CI one day when we least expect it, don't worry. =)

It's fine, I don't think it's good use of our time to overthink this at
the moment.

> Below is my fixup.  The hope is I addressed most of them but not all, with
> reasonings inline above.  Let me know.
>
>
> ===8<===
> From eb03b1b648b91a317a8a7559240186c87d835b8c Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Mon, 28 Sep 2026 11:35:20 -0400
> Subject: [PATCH] fixup! migration: Fix incoming channel race with multifd and
>  preempt_pre_7_2
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/channel.c   | 42 +++++++++++++++++-------------------------
>  migration/migration.c |  9 +++------
>  2 files changed, 20 insertions(+), 31 deletions(-)
>
> diff --git a/migration/channel.c b/migration/channel.c
> index acb7ef5206..72de9c9778 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -204,17 +204,14 @@ static MigChannelType migration_channel_peek(MigrationIncomingState *mis,
>  
>  /*
>   * Returns: >0 if success, ==0 (CH_NONE) if error.  If error happened,
> - * *errp must be set.
> + * *errp must be set.  NOTE: preempt channel is done separately.
>   */
>  static MigChannelType migration_channel_identify(MigrationIncomingState *mis,
>                                                   QIOChannel *ioc, Error **errp)
>  {
>      MigChannelType channel = CH_NONE;
>  
> -    if (migration_has_main_and_multifd_channels()) {
> -        assert(migrate_postcopy_preempt());
> -        channel = CH_POSTCOPY;

Fair.

> -    } else if (!mis->from_src_file) {
> +    if (!mis->from_src_file) {
>          channel = CH_MAIN;
>      } else if (migrate_multifd()) {
>          /*
> @@ -314,31 +311,16 @@ static bool migration_incoming_setup_channel(QIOChannel *ioc,
>                                           object_get_typename(OBJECT(ioc)),
>                                           mig_channel_str[ch]);
>      migration_ioc_register_yank(ioc);
> -    /* TODO: make this return the success status instead */
> -    migration_incoming_setup(ioc, ch, errp);
>  
> -    return *errp == NULL;
> +    return migration_incoming_setup(ioc, ch, errp);
>  }
>  
>  static bool migration_incoming_channel_install(MigrationIncomingState *mis,
>                                                 QIOChannel *ioc,
> +                                               MigChannelType ch,
>                                                 Error **errp)
>  {
> -    MigChannelType ch;
> -    bool ret;
> -
> -    if (qio_channel_is_peekable(ioc)) {
> -        ch = migration_channel_peek(mis, ioc, errp);
> -    } else {
> -        ch = migration_channel_identify(mis, ioc, errp);
> -    }
> -
> -    if (!ch) {
> -        return false;
> -    }
> -
> -    ret = migration_incoming_setup_channel(ioc, ch, errp);
> -    if (!ret) {
> +    if (!migration_incoming_setup_channel(ioc, ch, errp)) {
>          return false;
>      }
>  
> @@ -409,6 +391,7 @@ static gboolean migration_incoming_channel_readable(QIOChannel *ioc,
>  {
>      MigrationIncomingState *mis = opaque;
>      Error *local_err = NULL;
> +    MigChannelType ch;
>  
>      /*
>       * No need to monitor this channel anymore as long as anything arrived,
> @@ -435,7 +418,12 @@ static gboolean migration_incoming_channel_readable(QIOChannel *ioc,
>          goto out;
>      }
>  
> -    if (!migration_incoming_channel_install(mis, ioc, &local_err)) {
> +    ch = migration_channel_peek(mis, ioc, &local_err);
> +    if (!ch) {
> +        goto out;
> +    }
> +
> +    if (!migration_incoming_channel_install(mis, ioc, ch, &local_err)) {
>          goto out;
>      }
>  
> @@ -522,6 +510,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      Error *local_err = NULL;
> +    MigChannelType ch;
>  
>      trace_migration_incoming_channel_process(
>          ioc, object_get_typename(OBJECT(ioc)));
> @@ -558,7 +547,10 @@ void migration_channel_process_incoming(QIOChannel *ioc)
>              if (qio_channel_is_peekable(ioc)) {
>                  migration_incoming_channel_watch(mis, ioc);

Please carry my "async" comment if possible. It helps when navigating
the code because there's a migration_incoming_channel_install() just
below so we can jump to it without having to read
migration_incoming_channel_watch() definition.

>              } else {
> -                migration_incoming_channel_install(mis, ioc, &local_err);
> +                ch = migration_channel_identify(mis, ioc, &local_err);
> +                if (ch) {
> +                    migration_incoming_channel_install(mis, ioc, ch, &local_err);
> +                }
>              }
>          }
>      }
> diff --git a/migration/migration.c b/migration/migration.c
> index adba154e46..242b8ed812 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -854,10 +854,7 @@ out:
>      migrate_incoming_unref_outgoing_state();
>  }
>  
> -/*
> - * Returns whether all the necessary channels to proceed with the
> - * incoming migration have been established without error.
> - */
> +/* Return true if success, false otherwise (with *errp set) */

Probably not needed, this is the expected way.

>  bool migration_incoming_setup(QIOChannel *ioc, uint8_t channel, Error **errp)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
> @@ -886,13 +883,13 @@ bool migration_incoming_setup(QIOChannel *ioc, uint8_t channel, Error **errp)
>          assert(!mis->postcopy_qemufile_dst);
>          f = qemu_file_new_input(ioc);
>          postcopy_preempt_new_channel(mis, f);
> -        return false;
> +        break;

I see. Maybe a separate preliminary patch for this change?

>  
>      default:
>          g_assert_not_reached();
>      }
>  
> -    return migration_has_main_and_multifd_channels();
> +    return true;
>  }
>  
>  void migration_outgoing_setup(QIOChannel *ioc)
> -- 
> 2.55.0