On 7/28/26 20:39, Peter Xu wrote:
It was overlooked that VMSTATE_VBUFFER_UINT64() won't really work with an
uint64_t, as vmstate core only treats the size as 32bits, and maximum
INT32_MAX (see vmstate_size()).
Considering that we do not need real 64bits for the size, stick with the 2G
limit, converting the size field into 32bits.
Since we can't touch the wire protocol on migration from an old QEMU, we
can't directly modify the type of size to uint32_t. Instead, we need to
introduce a temporary variable for this extremely rare issue __size_32bits
to be used only for VMSTATE_VBUFFER_UINT32(). Document it and name it
weird enough so people won't get confused on having two size variables.
Remove VMSTATE_VBUFFER_UINT64() altogether, because it was never going to
be used right. It means QEMU will only support 2G max for VMS_VBUFFER.
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3675
Reported-by: 김승중 <[email protected]>
Cc: Alexandr Moshkov <[email protected]>
Cc: Michael S. Tsirkin <[email protected]>
Cc: Fabiano Rosas <[email protected]>
Fixes: 3a80ff0721 ("vhost: add vmstate for inflight region with inner buffer")
Signed-off-by: Peter Xu <[email protected]>
---
PS1: I only did smoke test as I'm not fluent with vhost inflight feature.
Please kindly try it out if possible. In general, migrations from older
QEMU should work even after applied. One can also treat this as partly-RFC
from that.
It looks like inflight migration is broken:
qemu-system-x86_64: Missing section footer for
0000:00:02.0:00.0:00.0/vhost-user-blk
migrate_error error=load of migration failed: Invalid argument: Section
footer error, section_id: 53
qemu-system-x86_64: load of migration failed: Invalid argument: Section
footer error, section_id: 53
As far as I understand this happen because __size_32bits not initialized
on source - it's never set. So vmstate_size() reads 0, and zero-lenght
buffer is written into migration stream.
The destination then allocates the correct buffer but reads 0 bytes from
the stream, leaving unconsumed data and causing the section footer check
to fail.
It can be fixed with adding pre_save (or pre_save_errp) to
vmstate_vhost_inflight_region_buffer that initialize __size_32bits from
size:
static int vhost_inflight_buffer_pre_save(void *opaque)
{
struct vhost_inflight *inflight = opaque;
/* Only used in VMSTATE_VBUFFER_UINT32() */
inflight->__size_32bits = inflight->size;
return 0;
}
PS2: Michael, we have just discussed what we should define as CVE for
migration, and this one shouldn't fall into CVE category, please refer to:
https://lore.kernel.org/r/[email protected]
So I didn't yet attach CVE tag. Please correct if I'm wrong, thanks.
---
include/hw/virtio/vhost.h | 5 +++++
include/migration/vmstate.h | 10 ----------
hw/virtio/vhost.c | 19 ++++++++++++++-----
3 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 684bafcaad..1d1cc24c04 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -17,6 +17,11 @@ struct vhost_inflight {
int fd;
void *addr;
uint64_t size;
+ /*
+ * This is a temporary variable only used during migration loading to
+ * satisfy VMSTATE_VBUFFER_UINT32() typing. Please use @size otherwise.
+ */
+ uint32_t __size_32bits;
uint64_t offset;
uint16_t queue_size;
};
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1b7f295417..a349b2d84a 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -782,16 +782,6 @@ extern const VMStateInfo vmstate_info_g_byte_array;
.offset = offsetof(_state, _field), \
}
-#define VMSTATE_VBUFFER_UINT64(_field, _state, _version, _test, _field_size) { \
- .name = (stringify(_field)), \
- .version_id = (_version), \
- .field_exists = (_test), \
- .size_offset = vmstate_offset_value(_state, _field_size, uint64_t),\
- .info = &vmstate_info_buffer, \
- .flags = VMS_VBUFFER | VMS_POINTER, \
- .offset = offsetof(_state, _field), \
-}
-
#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, \
_test, _field_size) { \
.name = (stringify(_field)), \
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index af41841b52..e7c570d0f5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -2022,15 +2022,24 @@ void vhost_get_features_ex(struct vhost_dev *hdev,
static bool vhost_inflight_buffer_pre_load(void *opaque, Error **errp)
{
struct vhost_inflight *inflight = opaque;
-
int fd = -1;
- void *addr = qemu_memfd_alloc("vhost-inflight", inflight->size,
- F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
- &fd, errp);
+ void *addr;
+
+ if (inflight->size > INT32_MAX) {
+ error_setg(errp, "inflight size '%"PRIu64"' exceeds "
+ "migration limit '%"PRIu32"'", inflight->size, INT32_MAX);
+ return false;
+ }
+
+ addr = qemu_memfd_alloc("vhost-inflight", inflight->size,
+ F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
+ &fd, errp);
if (!addr) {
return false;
}
+ /* Only used in VMSTATE_VBUFFER_UINT32() */
+ inflight->__size_32bits = inflight->size;
inflight->offset = 0;
inflight->addr = addr;
inflight->fd = fd;
@@ -2042,7 +2051,7 @@ const VMStateDescription
vmstate_vhost_inflight_region_buffer = {
.name = "vhost-inflight-region/buffer",
.pre_load_errp = vhost_inflight_buffer_pre_load,
.fields = (const VMStateField[]) {
- VMSTATE_VBUFFER_UINT64(addr, struct vhost_inflight, 0, NULL, size),
+ VMSTATE_VBUFFER_UINT32(addr, struct vhost_inflight, 0, NULL,
__size_32bits),
VMSTATE_END_OF_LIST()
}
};
