Re: [PATCH] vhost/migration: Fix incorrect size used in inflight->addr in VMSD

Peter Xu <[email protected]> writes:

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

I was thinking of fixing this on the vmstate size and not changing the
virtio code. But yes, it's better to fix properly.

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

Hm, but isn't it better to have a global cap anyway? So we don't need
every device code to change with similar checks.

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

I agree with making all u64. I think we can actually remove all the
extra VMS_VARRAY_* and VBUFFER_* flags and instead doa single type-check
of "int <= 64bit". Give me a couple of hours and I will post an RFC.

> 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,
>
>> 
>> > -
>> >  #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()
>> >      }
>> >  };
>>