Re: [PATCH 1/1] migration/multifd: fix channel count TOCTOU race on cancel and retry

[Peter Xu <peterx@redhat.com>]

On Thu, Apr 23, 2026 at 04:41:39PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, Apr 23, 2026 at 03:13:42PM -0300, Fabiano Rosas wrote:
> >> Looking again at this argument I put (too many variables), I notice we
> >> also have multifd_send_state->channels_ready and
> >
> > channels_ready seems to be special?  In busy systems I think it should
> > normally always be less than the number of threads on sender side, because
> > some of them will be busy.
> >
> 
> Argh, sorry, I meant channels_created!

Yeah, this one looks like a slight duplicate over channels_ready.  However
it's still slightly different in that it's also used in failure path of
channel establishments.

IOW, multifd_send_channel_created() can be invoked in failure paths where
multifd_channel_connect() won't.

We could still consider reusing channels_ready, but it will introduce a few
complexities, namely:

- The name becomes slightly ambiguous: we may need to listen to
  channels_ready even if the channel creation failed..  if to be fair,
  channels_created also implies a success.. s I assume not a major concern.

- Multifd sender side relies on channels_ready to be posted by default when
  migration just starts (says, "all channels are free to use").  It means
  if we consume that sem here waiting for channels, then we need to kick
  all threads once more just to give it back, hence one more roundtrip of
  sem notifies.  We also need a small touchup in send thread to allow that
  to happen; patch attached at the end to show what I mean (not tested at
  all, please treat it as pesudo code.. so it's definitely not a complete
  patch).

I think we can still leave it there to make the establishment path simple.

What's your take?

===8<===
diff --git a/migration/multifd.c b/migration/multifd.c
index 035cb70f7b..570ff8c017 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -736,16 +736,9 @@ static void *multifd_send_thread(void *opaque)
              * multifd_send().
              */
             qatomic_store_release(&p->pending_job, false);
-        } else {
+        } else if (qatomic_read(&p->pending_sync)) {
             MultiFDSyncReq req = qatomic_read(&p->pending_sync);
 
-            /*
-             * If not a normal job, must be a sync request.  Note that
-             * pending_sync is a standalone flag (unlike pending_job), so
-             * it doesn't require explicit memory barriers.
-             */
-            assert(req != MULTIFD_SYNC_NONE);
-
             /* Only push the SYNC message if it involves a remote sync */
             if (req == MULTIFD_SYNC_ALL) {
                 p->flags = MULTIFD_FLAG_SYNC;
@@ -964,7 +957,14 @@ bool multifd_send_setup(void)
      * past this point.
      */
     for (i = 0; i < thread_count; i++) {
-        qemu_sem_wait(&multifd_send_state->channels_created);
+        MultiFDSendParams *p = &multifd_send_state->params[i];
+
+        qemu_sem_wait(&multifd_send_state->channels_ready);
+        /*
+         * Re-kick the thread to recover the channels_ready event we
+         * consumed for detecting channel establish event.
+         */
+        qemu_sem_post(&p->sem);
     }
 
     if (ret) {

-- 
Peter Xu