On Wed, Jul 29, 2026 at 10:00:13AM -0400, Peter Xu wrote:
> On Tue, Jul 28, 2026 at 05:57:56PM -0300, Fabiano Rosas wrote:
> > Peter Xu <[email protected]> writes:
> >
> > > 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.
> > >
> > > 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), \
> > > -}
> >
> > Why don't you just leave this macro around, put a "do not use" comment
> > above it and remove the type check for this one instance? You're already
> > checking against INT32_MAX during pre_load.
>
> What's the benefit of keeping this global macro if we only allow one user
> to use it?
>
> Commenting to say "don't use" is not guaranteeing anything, at least we
> should also rename the macro to some weird name to make people notice.
> Keeping the macro name like this OTOH suggests abuse. But if we think it
> should only be used in 1 place, fixing the one place might be better?
>
> >
> > We could even lift that check up into vmstate_pre_load and reject
> > there globally, don't even touch the virtio code.
>
> Personally, I don't like the idea leaking VBUFFER impl details into
> vmstate_pre_load() which is so far only a wrapper, especially only for this
> one instance.. which we do not suggest future users to use.
>
> If we go this route, I'd rather merge Michael's version to support u64,
> even if we don't need a u64 size. But I really don't want to introduce yet
> another VMS flag just for this... we'll have no real use if we have noticed
> this problem when the vhost inflight patch was reviewed. It will be a
> uint32_t or int32_t already. I just can't come up with some users need
> size >2G.
It's not that size needs to be >2G. But it might be nice for some code
to keep it as u64 for its own reasons to avoid issues like overruns
during math ops.
>
> The recent AI reports just make such feeling stronger: we just used the
> reason "max 2G, should be fine" when AI reports that unlimited size
> allocation problem, now we will need to wait for another AI report which
> says "it's not 2G anymore, 1<<64-1 that is", if we don't come up with a
> proper limit for VMSD field allocations.
>
> >
> >
> > /rant
> > ... what's even the point of having per-type variants of VMSTATE macros
> > that exist just to type-check the extra offsets (.num_offset,
> > .size_offset as opposed to .offset)?
> >
> > For instance, look at vmstate_n_elems casting opaque data to 32 and
> > sub-32bit size and assigning to an int! What do we gain from that? It's
> > circular reasoning that does nothing aside from bothering the person
> > writing the vmstate.
> >
> > Right? Maybe I'm missing something, it's the end of the day already.
>
> I can also only guess, which is.. I believe Michael was right, the initial
> vmstate code was simply broken where it should have considered the type of
> size fields of all kinds, but forgot, and it just worked because nobody
> needed u64 for a size field.
>
> Said that, I still want to see if we can avoid introducing that at all. To
> me, I still prefer this patch (after fixing pre_save()..). But let me know
> if I didn't convince any of you.. we can discuss.
>
> Thanks,
The issue is that 2G is not a sane limit either.
So what we really want is a macro that supplies
a size limit. That would be an improvement.
> >
> > > -
> > > #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()
> > > }
> > > };
> >
>
> --
> Peter Xu
