[PATCH v2 0/5] migration: Hardening fixes for 11.2

[PATCH v2 0/5] migration: Hardening fixes for 11.2

[Peter Xu]

v2:
- Added R-bs
- Patch 2/3, update commit message on wrong use of term "release buillds"
- Patch 4, fix bug Dan pointed out, use assert() for !coroutine, update
  commit log

This is a small batch of fixes from recent reports on migration, the plan
is this is only for 11.2; we don't need to rush on any of them.

RDMA migration has its own small batch.. It'll take slightly longer so I'll
send them separately.

Reviews welcomed.

Peter Xu (5):
  migration: Fix possible overflow in vmstate_handle_alloc()
  migration/multifd: Validate next_packet_size in zlib/zstd recv
  migration/multifd: Replace assert() with error_setg() in recv paths
  migration: Fix rare hang of migration_channel_read_peek()
  migration/ram: Check for RAMBlock size mismatch when parsing

 migration/channel.c        | 11 ++++-
 migration/multifd-qatzip.c |  5 ++-
 migration/multifd-qpl.c    | 24 ++++++++--
 migration/multifd-uadk.c   | 24 ++++++++--
 migration/multifd-zlib.c   | 11 ++++-
 migration/multifd-zstd.c   | 11 ++++-
 migration/ram.c            | 11 ++++-
 migration/vmstate.c        | 91 ++++++++++++++++++++++++++++++++------
 8 files changed, 159 insertions(+), 29 deletions(-)

-- 
2.54.0

[PATCH v2 1/5] migration: Fix possible overflow in vmstate_handle_alloc()

[Peter Xu]

Migration incoming side almost always trusted the stream data and allows
allocation to happen with whatever size received.  With it, malicious
migration stream can manipulate destination QEMU behavior on g_malloc(), in
path of vmstate_handle_alloc() on specific VMSD fields.  Fix it by limiting
all sizes with int32_t positive values (INT_MAX) explicitly.

We have quite a few bug reports recently leveraging this defect. It can be
reproduced in many ways for (I think) all archs binaries, but the simplest
reproducer is:

  $ hexdump -C ./vm.img
  00000000  51 45 56 4d 00 00 00 03  07 80 00 00 00 00 00 00  |QEVM............|

  $ ./qemu-system-x86_64 -incoming file:./vm.img
  VNC server running on ::1:5900
  qemu-system-x86_64: GLib: ../glib/gmem.c:106: failed to allocate 18446744071562067968 bytes
  Aborted                    (core dumped) ./qemu-system-x86_64 -incoming file:./vm.img

We could assert here, but since we have errp right above the stack this
patch routes the errp over to allow destination QEMU fail gracefully.  This
means there's no way to DoS coredumpctl as well because we don't generate
core dumps at all. The output message could also hopefully help triage
issues when it's not a malicious stream but only wrong image used.

When at this, making sure multiplex also won't overflow.

After patched:

  $ ./qemu-system-x86_64 -incoming file:./vm.img
  VNC server running on ::1:5900
  qemu-system-x86_64: load of migration failed: Invalid argument: vmstate_size: VMState field 'name' overflow

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3805
Reported-by: Feifan Qian <bea1e@proton.me>
Reported-by: dong ling (@dongling226655)
Cc: qemu-stable <qemu-stable@nongnu.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/vmstate.c | 91 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 77 insertions(+), 14 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 50ebe37845..7bf0c2bae5 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -78,9 +78,10 @@ vmsd_init_ptr_marker_field(VMStateField *fake, const VMStateField *field)
     };
 }
 
-static int vmstate_n_elems(void *opaque, const VMStateField *field)
+static int32_t vmstate_n_elems(void *opaque, const VMStateField *field,
+                               Error **errp)
 {
-    int n_elems = 1;
+    int32_t n_elems = 1;
 
     if (field->flags & VMS_ARRAY) {
         n_elems = field->num;
@@ -94,18 +95,35 @@ static int vmstate_n_elems(void *opaque, const VMStateField *field)
         n_elems = *(uint8_t *)(opaque + field->num_offset);
     }
 
+    if (n_elems < 0) {
+        error_setg(errp, "%s: VMState field '%s' num_offset overflow",
+                   __func__, field->name);
+        return -EINVAL;
+    }
+
     trace_vmstate_n_elems(field->name, n_elems);
+
     return n_elems;
 }
 
-static int vmstate_size(void *opaque, const VMStateField *field)
+static int32_t vmstate_size(void *opaque, const VMStateField *field,
+                            Error **errp)
 {
-    int size;
+    int32_t size;
 
     if (field->flags & VMS_VBUFFER) {
+        /* For both int32_t/uint32_t we only allow 2GB limit for VBUFFER */
         size = *(int32_t *)(opaque + field->size_offset);
+
+        /* Check this explicitly for untrusted length input first */
+        if (size < 0) {
+            goto overflow;
+        }
+
         if (field->flags & VMS_MULTIPLY) {
-            size *= field->size;
+            if (smul32_overflow(field->size, size, &size)) {
+                goto overflow;
+            }
         }
     } else if (field->flags & VMS_ARRAY_OF_POINTER) {
         /*
@@ -115,21 +133,45 @@ static int vmstate_size(void *opaque, const VMStateField *field)
         size = sizeof(void *);
     } else {
         size = field->size;
+        assert(size >= 0);
     }
 
     return size;
+
+overflow:
+    error_setg(errp, "%s: VMState field '%s' overflow",
+               __func__, field->name);
+    return -EINVAL;
 }
 
-static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
-                                 void *opaque)
+static bool vmstate_handle_alloc(void *ptr, const VMStateField *field,
+                                 void *opaque, Error **errp)
 {
     if (field->flags & VMS_POINTER && field->flags & VMS_ALLOC) {
-        gsize size = vmstate_size(opaque, field);
-        size *= vmstate_n_elems(opaque, field);
+        int32_t size, n;
+
+        size = vmstate_size(opaque, field, errp);
+        if (size < 0) {
+            return false;
+        }
+
+        n = vmstate_n_elems(opaque, field, errp);
+        if (n < 0) {
+            return false;
+        }
+
+        if (smul32_overflow(size, n, &size)) {
+            error_setg(errp, "%s: VMState field '%s' multiply overflow",
+                       __func__, field->name);
+            return false;
+        }
+
         if (size) {
             *(void **)ptr = g_malloc(size);
         }
     }
+
+    return true;
 }
 
 static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
@@ -335,10 +377,22 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
 
         if (exists) {
             void *first_elem = opaque + field->offset;
-            int i, n_elems = vmstate_n_elems(opaque, field);
-            int size = vmstate_size(opaque, field);
+            int i, n_elems = vmstate_n_elems(opaque, field, errp);
+            int size;
+
+            if (n_elems < 0) {
+                return false;
+            }
+
+            size = vmstate_size(opaque, field, errp);
+            if (size < 0) {
+                return false;
+            }
+
+            if (!vmstate_handle_alloc(first_elem, field, opaque, errp)) {
+                return false;
+            }
 
-            vmstate_handle_alloc(first_elem, field, opaque);
             if (field->flags & VMS_POINTER) {
                 first_elem = *(void **)first_elem;
                 assert(first_elem || !n_elems || !size);
@@ -650,8 +704,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
     while (field->name) {
         if (vmstate_field_exists(vmsd, field, opaque, version_id)) {
             void *first_elem = opaque + field->offset;
-            int i, n_elems = vmstate_n_elems(opaque, field);
-            int size = vmstate_size(opaque, field);
+            int i, n_elems = vmstate_n_elems(opaque, field, errp);
             JSONWriter *vmdesc_loop = vmdesc;
             bool is_prev_null = false;
             /*
@@ -660,6 +713,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
              */
             bool use_dynamic_array =
                 field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
+            int32_t size;
+
+            if (n_elems < 0) {
+                return false;
+            }
+
+            size = vmstate_size(opaque, field, errp);
+            if (size < 0) {
+                return false;
+            }
 
             trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
             if (field->flags & VMS_POINTER) {
-- 
2.54.0

[PATCH v2 2/5] migration/multifd: Validate next_packet_size in zlib/zstd recv

[Peter Xu]

The zlib and zstd multifd compression backends read next_packet_size from
the incoming migration stream and use it directly as the read length into a
fixed-size buffer (MULTIFD_PACKET_SIZE * 2 = 1MB).  A malicious migration
source can set next_packet_size bigger than allocated, causing a heap
buffer overflow write on the destination.

Add a check against zbuff_len before reading, matching what the qatzip
backend already does.  Also replace the assert(in_size == 0) for empty
packets with proper error reporting, since the value is wire-controlled,
meanwhile assert() stops working with -DNDEBUG builds.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3737
Reported-by: xlabai <xlabai@tencent.com>
Reported-by: Jules Denardou <jules.denardou@datadoghq.com>
Reported-by: Tristan Madani <tristan@talencesecurity.com>
Reported-by: david korczynski (@david1766)
Reported-by: huntr bubble (@bubblehuntr)
Cc: qemu-stable <qemu-stable@nongnu.org>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd-zlib.c | 11 ++++++++++-
 migration/multifd-zstd.c | 11 ++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
index 8820b2a787..400146566e 100644
--- a/migration/multifd-zlib.c
+++ b/migration/multifd-zlib.c
@@ -216,10 +216,19 @@ static int multifd_zlib_recv(MultiFDRecvParams *p, Error **errp)
         return -1;
     }
 
+    if (in_size > z->zbuff_len) {
+        error_setg(errp, "multifd %u: next_packet_size %"PRIu32
+                   " exceeds allocated %"PRIu32, p->id, in_size, z->zbuff_len);
+        return -1;
+    }
+
     multifd_recv_zero_page_process(p);
 
     if (!p->normal_num) {
-        assert(in_size == 0);
+        if (in_size != 0) {
+            error_setg(errp, "multifd %u: expected empty packet", p->id);
+            return -1;
+        }
         return 0;
     }
 
diff --git a/migration/multifd-zstd.c b/migration/multifd-zstd.c
index 3c2dcf76b0..69ef1a5f38 100644
--- a/migration/multifd-zstd.c
+++ b/migration/multifd-zstd.c
@@ -210,10 +210,19 @@ static int multifd_zstd_recv(MultiFDRecvParams *p, Error **errp)
         return -1;
     }
 
+    if (in_size > z->zbuff_len) {
+        error_setg(errp, "multifd %u: next_packet_size %"PRIu32
+                   " exceeds allocated %"PRIu32, p->id, in_size, z->zbuff_len);
+        return -1;
+    }
+
     multifd_recv_zero_page_process(p);
 
     if (!p->normal_num) {
-        assert(in_size == 0);
+        if (in_size != 0) {
+            error_setg(errp, "multifd %u: expected empty packet", p->id);
+            return -1;
+        }
         return 0;
     }
 
-- 
2.54.0

[PATCH v2 3/5] migration/multifd: Replace assert() with error_setg() in recv paths

[Peter Xu]

QPL and UADK multifd backends use assert() to validate wire-controlled
fields like per-page compressed lengths and packet size consistency.  These
asserts will stop working with -DNDEBUG builds, so may stop working.

Replace all assert() calls in the receive path with proper error_setg() so
validation failures are reported gracefully rather than crashing or
silently ignored.

While at it, touch up an assert() in qatzip recv path too.

Cc: qemu-stable <qemu-stable@nongnu.org>
Cc: Yuan Liu <yuan1.liu@intel.com>
Cc: Yichen Wang <yichen.wang@bytedance.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd-qatzip.c |  5 ++++-
 migration/multifd-qpl.c    | 24 ++++++++++++++++++++----
 migration/multifd-uadk.c   | 24 ++++++++++++++++++++----
 3 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/migration/multifd-qatzip.c b/migration/multifd-qatzip.c
index 7419e5dc0d..0262e81eac 100644
--- a/migration/multifd-qatzip.c
+++ b/migration/multifd-qatzip.c
@@ -348,7 +348,10 @@ static int qatzip_recv(MultiFDRecvParams *p, Error **errp)
 
     multifd_recv_zero_page_process(p);
     if (!p->normal_num) {
-        assert(in_size == 0);
+        if (in_size != 0) {
+            error_setg(errp, "multifd %u: expected empty packet", p->id);
+            return -1;
+        }
         return 0;
     }
 
diff --git a/migration/multifd-qpl.c b/migration/multifd-qpl.c
index 52902eb00c..3826e7f340 100644
--- a/migration/multifd-qpl.c
+++ b/migration/multifd-qpl.c
@@ -664,26 +664,42 @@ static int multifd_qpl_recv(MultiFDRecvParams *p, Error **errp)
     }
     multifd_recv_zero_page_process(p);
     if (!p->normal_num) {
-        assert(in_size == 0);
+        if (in_size != 0) {
+            error_setg(errp, "multifd %u: expected empty packet", p->id);
+            return -1;
+        }
         return 0;
     }
 
     /* read compressed page lengths */
     len = p->normal_num * sizeof(uint32_t);
-    assert(len < in_size);
+    if (len >= in_size) {
+        error_setg(errp, "multifd %u: header len %"PRIu32
+                   " >= packet size %"PRIu32, p->id, len, in_size);
+        return -1;
+    }
     ret = qio_channel_read_all(p->c, (void *) qpl->zlen, len, errp);
     if (ret != 0) {
         return ret;
     }
     for (int i = 0; i < p->normal_num; i++) {
         qpl->zlen[i] = be32_to_cpu(qpl->zlen[i]);
-        assert(qpl->zlen[i] <= multifd_ram_page_size());
+        if (qpl->zlen[i] > multifd_ram_page_size()) {
+            error_setg(errp, "multifd %u: page %d compressed len %"
+                       PRIu32" too large", p->id, i, qpl->zlen[i]);
+            return -1;
+        }
         zbuf_len += qpl->zlen[i];
         ramblock_recv_bitmap_set_offset(p->block, p->normal[i]);
     }
 
     /* read compressed pages */
-    assert(in_size == len + zbuf_len);
+    if (in_size != len + zbuf_len) {
+        error_setg(errp, "multifd %u: packet size %"PRIu32
+                   " != header %"PRIu32" + data %"PRIu32,
+                   p->id, in_size, len, zbuf_len);
+        return -1;
+    }
     ret = qio_channel_read_all(p->c, (void *) qpl->zbuf, zbuf_len, errp);
     if (ret != 0) {
         return ret;
diff --git a/migration/multifd-uadk.c b/migration/multifd-uadk.c
index fd7cd9b5e8..d373615ba8 100644
--- a/migration/multifd-uadk.c
+++ b/migration/multifd-uadk.c
@@ -245,12 +245,19 @@ static int multifd_uadk_recv(MultiFDRecvParams *p, Error **errp)
 
     multifd_recv_zero_page_process(p);
     if (!p->normal_num) {
-        assert(in_size == 0);
+        if (in_size != 0) {
+            error_setg(errp, "multifd %u: expected empty packet", p->id);
+            return -1;
+        }
         return 0;
     }
 
     /* read compressed data lengths */
-    assert(hdr_len < in_size);
+    if (hdr_len >= in_size) {
+        error_setg(errp, "multifd %u: header len %"PRIu32
+                   " >= packet size %"PRIu32, p->id, hdr_len, in_size);
+        return -1;
+    }
     ret = qio_channel_read_all(p->c, (void *) uadk_data->buf_hdr,
                                hdr_len, errp);
     if (ret != 0) {
@@ -259,12 +266,21 @@ static int multifd_uadk_recv(MultiFDRecvParams *p, Error **errp)
 
     for (int i = 0; i < p->normal_num; i++) {
         uadk_data->buf_hdr[i] = be32_to_cpu(uadk_data->buf_hdr[i]);
+        if (uadk_data->buf_hdr[i] > page_size) {
+            error_setg(errp, "multifd %u: page %d compressed len %"PRIu32
+                       " too large", p->id, i, uadk_data->buf_hdr[i]);
+            return -1;
+        }
         data_len += uadk_data->buf_hdr[i];
-        assert(uadk_data->buf_hdr[i] <= page_size);
     }
 
     /* read compressed data */
-    assert(in_size == hdr_len + data_len);
+    if (in_size != hdr_len + data_len) {
+        error_setg(errp, "multifd %u: packet size %"PRIu32
+                   " != header %"PRIu32" + data %"PRIu32,
+                   p->id, in_size, hdr_len, data_len);
+        return -1;
+    }
     ret = qio_channel_read_all(p->c, (void *)buf, data_len, errp);
     if (ret != 0) {
         return ret;
-- 
2.54.0

[PATCH v2 4/5] migration: Fix rare hang of migration_channel_read_peek()

[Peter Xu]

In an unlikely case, when a migration stream is attached to the destination
QEMU and only send <4 bytes to the channel as magic, it's possible that
migration_channel_read_peek() may spin forever.

Fix it by adding a manual sleep for partial read.

Since the path isn't attached to a coroutine, it means when partial read
happens, there's yet not much we can do but hang the main thread, it will
happen even for len==0 case.  It means monitors can hang due to this,
either partial read or no data arrived (but connection established).

Leave this for later, the hope is this is extremely rare in production.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
Reported-by: Feifan Qian <bea1e@proton.me>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/channel.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/migration/channel.c b/migration/channel.c
index 1e2935f926..f446561b59 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -296,9 +296,16 @@ int migration_channel_read_peek(QIOChannel *ioc,
 
         if (len == buflen) {
             break;
+        } else if (len == 0) {
+            qio_channel_wait_cond(ioc, G_IO_IN);
+        } else {
+            /*
+             * When partially ready, we can't use qio_channel_wait_cond()
+             * because it will return immediately.  Apply a manual wait.
+             */
+            assert(!qemu_in_coroutine());
+            g_usleep(1000);
         }
-
-        qio_channel_wait_cond(ioc, G_IO_IN);
     }
 
     return 0;
-- 
2.54.0

[PATCH v2 5/5] migration/ram: Check for RAMBlock size mismatch when parsing

[Peter Xu]

Add an underflow check for the subtract of total RAMBlock size to make sure
it won't underflow.  It should not happen in production systems but only if
the migration stream was hijacked, which is not a real concern since
migration channel is trusted.  Still protect against it.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4013
Reported-by: Tristan Madani <tristan@talencesecurity.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 8918b2f03b..85feff578c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4268,7 +4268,7 @@ static int parse_ramblocks(QEMUFile *f, ram_addr_t total_ram_bytes)
     int ret = 0;
 
     /* Synchronize RAM block list */
-    while (!ret && total_ram_bytes) {
+    while (total_ram_bytes) {
         RAMBlock *block;
         char id[256];
         ram_addr_t length;
@@ -4285,8 +4285,15 @@ static int parse_ramblocks(QEMUFile *f, ram_addr_t total_ram_bytes)
             error_report("Unknown ramblock \"%s\", cannot accept "
                          "migration", id);
             ret = -EINVAL;
+            break;
+        }
+
+        if (usub64_overflow(total_ram_bytes, length, &total_ram_bytes)) {
+            error_report("%s: RAMBlock '%s' size underflow total RAM size",
+                         __func__, block->idstr);
+            ret = -EFAULT;
+            break;
         }
-        total_ram_bytes -= length;
     }
 
     return ret;
-- 
2.54.0

Re: [PATCH v2 4/5] migration: Fix rare hang of migration_channel_read_peek()

[Peter Xu]

On Tue, Jul 28, 2026 at 05:04:16PM -0400, Peter Xu wrote:
> In an unlikely case, when a migration stream is attached to the destination
> QEMU and only send <4 bytes to the channel as magic, it's possible that
> migration_channel_read_peek() may spin forever.
> 
> Fix it by adding a manual sleep for partial read.
> 
> Since the path isn't attached to a coroutine, it means when partial read
> happens, there's yet not much we can do but hang the main thread, it will
> happen even for len==0 case.  It means monitors can hang due to this,
> either partial read or no data arrived (but connection established).
> 
> Leave this for later, the hope is this is extremely rare in production.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
> Reported-by: Feifan Qian <bea1e@proton.me>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/channel.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index 1e2935f926..f446561b59 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -296,9 +296,16 @@ int migration_channel_read_peek(QIOChannel *ioc,
>  
>          if (len == buflen) {
>              break;
> +        } else if (len == 0) {

I think this should be QIO_CHANNEL_ERR_BLOCK, not 0..  I'll fix it when I
post v3, and I'll do some more tests.

> +            qio_channel_wait_cond(ioc, G_IO_IN);
> +        } else {
> +            /*
> +             * When partially ready, we can't use qio_channel_wait_cond()
> +             * because it will return immediately.  Apply a manual wait.
> +             */
> +            assert(!qemu_in_coroutine());
> +            g_usleep(1000);
>          }
> -
> -        qio_channel_wait_cond(ioc, G_IO_IN);
>      }
>  
>      return 0;
> -- 
> 2.54.0
> 

-- 
Peter Xu

Re: [PATCH v2 4/5] migration: Fix rare hang of migration_channel_read_peek()

[Peter Xu]

On Tue, Jul 28, 2026 at 05:29:35PM -0400, Peter Xu wrote:
> On Tue, Jul 28, 2026 at 05:04:16PM -0400, Peter Xu wrote:
> > In an unlikely case, when a migration stream is attached to the destination
> > QEMU and only send <4 bytes to the channel as magic, it's possible that
> > migration_channel_read_peek() may spin forever.
> > 
> > Fix it by adding a manual sleep for partial read.
> > 
> > Since the path isn't attached to a coroutine, it means when partial read
> > happens, there's yet not much we can do but hang the main thread, it will
> > happen even for len==0 case.  It means monitors can hang due to this,
> > either partial read or no data arrived (but connection established).
> > 
> > Leave this for later, the hope is this is extremely rare in production.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
> > Reported-by: Feifan Qian <bea1e@proton.me>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/channel.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/migration/channel.c b/migration/channel.c
> > index 1e2935f926..f446561b59 100644
> > --- a/migration/channel.c
> > +++ b/migration/channel.c
> > @@ -296,9 +296,16 @@ int migration_channel_read_peek(QIOChannel *ioc,
> >  
> >          if (len == buflen) {
> >              break;
> > +        } else if (len == 0) {
> 
> I think this should be QIO_CHANNEL_ERR_BLOCK, not 0..  I'll fix it when I
> post v3, and I'll do some more tests.

I just found that I cannot really hit this path.. because essentially
migration_channel_read_peek() is too early, and we haven't set iochannel to
be in non-blocking (QEMU only does it for the main channel; for the rest
channels they're always in blocking mode).

But still, instead of asserting it, I plan to keep this line so this code
is generic to blocking mode too in the future.

> 
> > +            qio_channel_wait_cond(ioc, G_IO_IN);
> > +        } else {
> > +            /*
> > +             * When partially ready, we can't use qio_channel_wait_cond()
> > +             * because it will return immediately.  Apply a manual wait.
> > +             */
> > +            assert(!qemu_in_coroutine());
> > +            g_usleep(1000);
> >          }
> > -
> > -        qio_channel_wait_cond(ioc, G_IO_IN);
> >      }
> >  
> >      return 0;
> > -- 
> > 2.54.0
> > 
> 
> -- 
> Peter Xu

-- 
Peter Xu

Re: [PATCH v2 4/5] migration: Fix rare hang of migration_channel_read_peek()

[Daniel P. Berrangé]

On Tue, Jul 28, 2026 at 05:29:35PM -0400, Peter Xu wrote:
> On Tue, Jul 28, 2026 at 05:04:16PM -0400, Peter Xu wrote:
> > In an unlikely case, when a migration stream is attached to the destination
> > QEMU and only send <4 bytes to the channel as magic, it's possible that
> > migration_channel_read_peek() may spin forever.
> > 
> > Fix it by adding a manual sleep for partial read.
> > 
> > Since the path isn't attached to a coroutine, it means when partial read
> > happens, there's yet not much we can do but hang the main thread, it will
> > happen even for len==0 case.  It means monitors can hang due to this,
> > either partial read or no data arrived (but connection established).
> > 
> > Leave this for later, the hope is this is extremely rare in production.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
> > Reported-by: Feifan Qian <bea1e@proton.me>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/channel.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/migration/channel.c b/migration/channel.c
> > index 1e2935f926..f446561b59 100644
> > --- a/migration/channel.c
> > +++ b/migration/channel.c
> > @@ -296,9 +296,16 @@ int migration_channel_read_peek(QIOChannel *ioc,
> >  
> >          if (len == buflen) {
> >              break;
> > +        } else if (len == 0) {
> 
> I think this should be QIO_CHANNEL_ERR_BLOCK, not 0..  I'll fix it when I
> post v3, and I'll do some more tests.

Yep, 0 == EOF, so you'll need ERR_BLOCK.

> 
> > +            qio_channel_wait_cond(ioc, G_IO_IN);
> > +        } else {
> > +            /*
> > +             * When partially ready, we can't use qio_channel_wait_cond()
> > +             * because it will return immediately.  Apply a manual wait.
> > +             */
> > +            assert(!qemu_in_coroutine());
> > +            g_usleep(1000);

So this will block the main loop, but this is OK because

 * If the network is untrusted, migration should have been configured
   to use TLS with a certificate allow-list. This peek takes place
   after the TLS handshake so is protected
 * On the dest host, it doesn't hugely matter that we block the main
   loop as there's no running guest yet that will stall. Just any
   use of QMP commands on the dest will stall.

> >          }
> > -
> > -        qio_channel_wait_cond(ioc, G_IO_IN);
> >      }
> >  
> >      return 0;
> > -- 
> > 2.54.0
> > 
> 
> -- 
> Peter Xu
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH v2 4/5] migration: Fix rare hang of migration_channel_read_peek()

[Peter Xu]

On Wed, Jul 29, 2026 at 03:43:43PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 28, 2026 at 05:29:35PM -0400, Peter Xu wrote:
> > On Tue, Jul 28, 2026 at 05:04:16PM -0400, Peter Xu wrote:
> > > In an unlikely case, when a migration stream is attached to the destination
> > > QEMU and only send <4 bytes to the channel as magic, it's possible that
> > > migration_channel_read_peek() may spin forever.
> > > 
> > > Fix it by adding a manual sleep for partial read.
> > > 
> > > Since the path isn't attached to a coroutine, it means when partial read
> > > happens, there's yet not much we can do but hang the main thread, it will
> > > happen even for len==0 case.  It means monitors can hang due to this,
> > > either partial read or no data arrived (but connection established).
> > > 
> > > Leave this for later, the hope is this is extremely rare in production.
> > > 
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
> > > Reported-by: Feifan Qian <bea1e@proton.me>
> > > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  migration/channel.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/migration/channel.c b/migration/channel.c
> > > index 1e2935f926..f446561b59 100644
> > > --- a/migration/channel.c
> > > +++ b/migration/channel.c
> > > @@ -296,9 +296,16 @@ int migration_channel_read_peek(QIOChannel *ioc,
> > >  
> > >          if (len == buflen) {
> > >              break;
> > > +        } else if (len == 0) {
> > 
> > I think this should be QIO_CHANNEL_ERR_BLOCK, not 0..  I'll fix it when I
> > post v3, and I'll do some more tests.
> 
> Yep, 0 == EOF, so you'll need ERR_BLOCK.
> 
> > 
> > > +            qio_channel_wait_cond(ioc, G_IO_IN);
> > > +        } else {
> > > +            /*
> > > +             * When partially ready, we can't use qio_channel_wait_cond()
> > > +             * because it will return immediately.  Apply a manual wait.
> > > +             */
> > > +            assert(!qemu_in_coroutine());
> > > +            g_usleep(1000);
> 
> So this will block the main loop, but this is OK because
> 
>  * If the network is untrusted, migration should have been configured
>    to use TLS with a certificate allow-list. This peek takes place
>    after the TLS handshake so is protected
>  * On the dest host, it doesn't hugely matter that we block the main
>    loop as there's no running guest yet that will stall. Just any
>    use of QMP commands on the dest will stall.

Yes, it's more about hanging the monitors.  I think that's indeed an
unwanted behavior, it's just that such behavior existed since the read peek
was merged so it's a while, and it won't happen in normal cases.

To fix it, we can consider creating the coroutine early, but my gut feeling
is it'll be hard because we can have quite some assumptions that all things
will be serialized during establishing all channels, so I suspect we're not
ready having >1 coroutines each on setting up some channel.

A better way might be that if we can land the threadify loadvm series:

https://lore.kernel.org/r/20251022192612.2737648-1-peterx@redhat.com

Then we can offload all channel operations into the thread and I think it's
easier creating the thread early instead, since everything will still be
serialized in that thread with no worry of half-processed coroutines. The
main thread should forward each new iochannel to the thread and continue
the event loop.

Thanks,

-- 
Peter Xu