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

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

[Peter Xu]

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        | 26 +++++++++--
 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, 173 insertions(+), 30 deletions(-)

-- 
2.54.0

[PATCH 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>
Cc: 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 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 release 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>
Cc: 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 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 released version of binaries, 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: Fabiano Rosas <farosas@suse.de>
Cc: Yuan Liu <yuan1.liu@intel.com>
Cc: Yichen Wang <yichen.wang@bytedance.com>
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 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 without yielding in the main
thread causing two unwanted consequences:

- CPU will spin 100% waiting for the rest bytes until it reaches 4
- (more importantly..) Main thread is stuck during this process as the qio
  operation won't really yield the coroutine

Fix it by consuming the bytes that arrived.

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

diff --git a/migration/channel.c b/migration/channel.c
index 1e2935f926..28fe1d2906 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -294,11 +294,31 @@ int migration_channel_read_peek(QIOChannel *ioc,
             return -1;
         }
 
-        if (len == buflen) {
+        if (len == iov.iov_len) {
             break;
-        }
+        } else if (len == 0) {
+            qio_channel_wait_cond(ioc, G_IO_IN);
+        } else {
+            ssize_t received = len;
 
-        qio_channel_wait_cond(ioc, G_IO_IN);
+            /*
+             * Partially arrived, read out to make qio_channel_wait_cond()
+             * won't return immediately, causing an unwanted spin on this
+             * CPU.
+             */
+            iov.iov_len = len;
+            len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL, 0, errp);
+            /*
+             * QIO_CHANNEL_ERR_BLOCK also shouldn't happen, due to the
+             * prior peek just happened.  We should be pretty sure we will
+             * read what we peeked, or the channel was broken.
+             */
+            if (len != received) {
+                return -1;
+            }
+            iov.iov_base += received;
+            iov.iov_len = buflen - received;
+        }
     }
 
     return 0;
-- 
2.54.0

[PATCH 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>
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 4/5] migration: Fix rare hang of migration_channel_read_peek()

[Daniel P. Berrangé]

On Tue, Jul 28, 2026 at 11:52:46AM -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 without yielding in the main
> thread causing two unwanted consequences:
> 
> - CPU will spin 100% waiting for the rest bytes until it reaches 4
> - (more importantly..) Main thread is stuck during this process as the qio
>   operation won't really yield the coroutine
> 
> Fix it by consuming the bytes that arrived.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Reported-by: Feifan Qian <bea1e@proton.me>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/channel.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index 1e2935f926..28fe1d2906 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -294,11 +294,31 @@ int migration_channel_read_peek(QIOChannel *ioc,
>              return -1;
>          }
>  
> -        if (len == buflen) {
> +        if (len == iov.iov_len) {
>              break;
> -        }
> +        } else if (len == 0) {
> +            qio_channel_wait_cond(ioc, G_IO_IN);
> +        } else {
> +            ssize_t received = len;
>  
> -        qio_channel_wait_cond(ioc, G_IO_IN);
> +            /*
> +             * Partially arrived, read out to make qio_channel_wait_cond()
> +             * won't return immediately, causing an unwanted spin on this
> +             * CPU.
> +             */
> +            iov.iov_len = len;
> +            len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL, 0, errp);

Sure this breaks the API behaviour that the caller is expecting to
see.

migration_channel_identify will call migration_channel_read_peek
to match the magic bytes.

But something later in the flow will actually try to read the magic
bytes.  By consuming them in this migration_channel_read_peek
method, surely we're breaking the code that wants to read the bytes
later.

> +            /*
> +             * QIO_CHANNEL_ERR_BLOCK also shouldn't happen, due to the
> +             * prior peek just happened.  We should be pretty sure we will
> +             * read what we peeked, or the channel was broken.
> +             */
> +            if (len != received) {
> +                return -1;
> +            }
> +            iov.iov_base += received;
> +            iov.iov_len = buflen - received;
> +        }
>      }
>  
>      return 0;
> -- 
> 2.54.0
> 

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 4/5] migration: Fix rare hang of migration_channel_read_peek()

[Peter Xu]

On Tue, Jul 28, 2026 at 05:24:36PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 28, 2026 at 11:52:46AM -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 without yielding in the main
> > thread causing two unwanted consequences:
> > 
> > - CPU will spin 100% waiting for the rest bytes until it reaches 4
> > - (more importantly..) Main thread is stuck during this process as the qio
> >   operation won't really yield the coroutine
> > 
> > Fix it by consuming the bytes that arrived.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Reported-by: Feifan Qian <bea1e@proton.me>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/channel.c | 26 +++++++++++++++++++++++---
> >  1 file changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/migration/channel.c b/migration/channel.c
> > index 1e2935f926..28fe1d2906 100644
> > --- a/migration/channel.c
> > +++ b/migration/channel.c
> > @@ -294,11 +294,31 @@ int migration_channel_read_peek(QIOChannel *ioc,
> >              return -1;
> >          }
> >  
> > -        if (len == buflen) {
> > +        if (len == iov.iov_len) {
> >              break;
> > -        }
> > +        } else if (len == 0) {
> > +            qio_channel_wait_cond(ioc, G_IO_IN);
> > +        } else {
> > +            ssize_t received = len;
> >  
> > -        qio_channel_wait_cond(ioc, G_IO_IN);
> > +            /*
> > +             * Partially arrived, read out to make qio_channel_wait_cond()
> > +             * won't return immediately, causing an unwanted spin on this
> > +             * CPU.
> > +             */
> > +            iov.iov_len = len;
> > +            len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL, 0, errp);
> 
> Sure this breaks the API behaviour that the caller is expecting to
> see.
> 
> migration_channel_identify will call migration_channel_read_peek
> to match the magic bytes.
> 
> But something later in the flow will actually try to read the magic
> bytes.  By consuming them in this migration_channel_read_peek
> method, surely we're breaking the code that wants to read the bytes
> later.

Ah right, stupid me.  The best then is for partial read we apply a manual
wait.

I can also revert 604bb1badc ("migration: Properly wait on G_IO_IN when
peeking messages"), looping with 1ms interval for data isn't too bad.  But
the best is we only do that for partial, so:

diff --git a/migration/channel.c b/migration/channel.c
index 1e2935f926..88f89521f8 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -296,9 +296,19 @@ 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.
+             */
+            if (qemu_in_coroutine()) {
+                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
+            } else {
+                g_usleep(1000);
+            }
         }
-
-        qio_channel_wait_cond(ioc, G_IO_IN);
     }

     return 0;

Any preference?

Thanks,

> 
> > +            /*
> > +             * QIO_CHANNEL_ERR_BLOCK also shouldn't happen, due to the
> > +             * prior peek just happened.  We should be pretty sure we will
> > +             * read what we peeked, or the channel was broken.
> > +             */
> > +            if (len != received) {
> > +                return -1;
> > +            }
> > +            iov.iov_base += received;
> > +            iov.iov_len = buflen - received;
> > +        }
> >      }
> >  
> >      return 0;
> > -- 
> > 2.54.0
> > 
> 
> 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 :|
> 

-- 
Peter Xu

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

[Daniel P. Berrangé]

On Tue, Jul 28, 2026 at 01:09:03PM -0400, Peter Xu wrote:
> On Tue, Jul 28, 2026 at 05:24:36PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jul 28, 2026 at 11:52:46AM -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 without yielding in the main
> > > thread causing two unwanted consequences:
> > > 
> > > - CPU will spin 100% waiting for the rest bytes until it reaches 4
> > > - (more importantly..) Main thread is stuck during this process as the qio
> > >   operation won't really yield the coroutine
> > > 
> > > Fix it by consuming the bytes that arrived.
> > > 
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
> > > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > > Reported-by: Feifan Qian <bea1e@proton.me>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  migration/channel.c | 26 +++++++++++++++++++++++---
> > >  1 file changed, 23 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/migration/channel.c b/migration/channel.c
> > > index 1e2935f926..28fe1d2906 100644
> > > --- a/migration/channel.c
> > > +++ b/migration/channel.c
> > > @@ -294,11 +294,31 @@ int migration_channel_read_peek(QIOChannel *ioc,
> > >              return -1;
> > >          }
> > >  
> > > -        if (len == buflen) {
> > > +        if (len == iov.iov_len) {
> > >              break;
> > > -        }
> > > +        } else if (len == 0) {
> > > +            qio_channel_wait_cond(ioc, G_IO_IN);
> > > +        } else {
> > > +            ssize_t received = len;
> > >  
> > > -        qio_channel_wait_cond(ioc, G_IO_IN);
> > > +            /*
> > > +             * Partially arrived, read out to make qio_channel_wait_cond()
> > > +             * won't return immediately, causing an unwanted spin on this
> > > +             * CPU.
> > > +             */
> > > +            iov.iov_len = len;
> > > +            len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL, 0, errp);
> > 
> > Sure this breaks the API behaviour that the caller is expecting to
> > see.
> > 
> > migration_channel_identify will call migration_channel_read_peek
> > to match the magic bytes.
> > 
> > But something later in the flow will actually try to read the magic
> > bytes.  By consuming them in this migration_channel_read_peek
> > method, surely we're breaking the code that wants to read the bytes
> > later.
> 
> Ah right, stupid me.  The best then is for partial read we apply a manual
> wait.
> 
> I can also revert 604bb1badc ("migration: Properly wait on G_IO_IN when
> peeking messages"), looping with 1ms interval for data isn't too bad.  But
> the best is we only do that for partial, so:
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index 1e2935f926..88f89521f8 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -296,9 +296,19 @@ 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.
> +             */
> +            if (qemu_in_coroutine()) {
> +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
> +            } else {
> +                g_usleep(1000);
> +            }
>          }
> -
> -        qio_channel_wait_cond(ioc, G_IO_IN);
>      }

This approach looks best - a "normal" QEMU will never hit the sleep
loop, so we'll only get this spin if the client is intentionally
playing games by sending short packets.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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