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