* [PATCH v3] vmstate: validate VMStateDescription::fields upon registration @ 2026-03-13 23:35 Roman Kiryanov 2026-03-16 14:21 ` Fabiano Rosas 2026-03-17 18:50 ` Fabiano Rosas 0 siblings, 2 replies; 6+ messages in thread From: Roman Kiryanov @ 2026-03-13 23:35 UTC (permalink / raw) To: peterx, farosas; +Cc: qemu-devel, whollins, jansene, jpcottin, Roman Kiryanov The vmstate_save_state_v() function does not support NULL in VMStateDescription::fields and will crash if one is provided. This change prevents this situation from happening by explicitly crashing earlier. Suggested-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Peter Xu <peterx@redhat.com> Signed-off-by: Roman Kiryanov <rkir@google.com> --- v3: - Also added assert to virtio.c to validate VirtioDeviceClass instances which bypass vmstate_register_with_alias_id. - Updated the commit message to "Reviewed-by". v2: - Moved the assert from vmstate_save_state_v to the registration path (vmstate_register_with_alias_id) and the qtest validation path (vmstate_check) to catch missing fields earlier. --- hw/virtio/virtio.c | 6 ++++++ migration/savevm.c | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 0ba734d0bc..e4543de335 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -4054,6 +4054,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp) /* Devices should either use vmsd or the load/save methods */ assert(!vdc->vmsd || !vdc->load); + /* + * If a device has vmsd, it either MUST have valid fields + * or marked unmigratable. + */ + assert(!vdc->vmsd || (vdc->vmsd->unmigratable || vdc->vmsd->fields)); + if (vdc->realize != NULL) { vdc->realize(dev, &err); if (err != NULL) { diff --git a/migration/savevm.c b/migration/savevm.c index 197c89e0e6..20c2b99231 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -861,6 +861,8 @@ static void vmstate_check(const VMStateDescription *vmsd) const VMStateField *field = vmsd->fields; const VMStateDescription * const *subsection = vmsd->subsections; + assert(field || vmsd->unmigratable); + if (field) { while (field->name) { if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) { @@ -900,6 +902,9 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id, /* If this triggers, alias support can be dropped for the vmsd. */ assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id); + /* If vmsd is migratable it MUST have valid fields. */ + assert(vmsd->fields || vmsd->unmigratable); + se = g_new0(SaveStateEntry, 1); se->version_id = vmsd->version_id; se->section_id = savevm_state.global_section_id++; -- 2.53.0.851.ga537e3e6e9-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration 2026-03-13 23:35 [PATCH v3] vmstate: validate VMStateDescription::fields upon registration Roman Kiryanov @ 2026-03-16 14:21 ` Fabiano Rosas 2026-03-16 15:42 ` Peter Xu ` (2 more replies) 2026-03-17 18:50 ` Fabiano Rosas 1 sibling, 3 replies; 6+ messages in thread From: Fabiano Rosas @ 2026-03-16 14:21 UTC (permalink / raw) To: Roman Kiryanov, peterx Cc: qemu-devel, whollins, jansene, jpcottin, Roman Kiryanov Roman Kiryanov <rkir@google.com> writes: > The vmstate_save_state_v() function does not support > NULL in VMStateDescription::fields and will crash if > one is provided. > > This change prevents this situation from happening > by explicitly crashing earlier. > > Suggested-by: Fabiano Rosas <farosas@suse.de> > Reviewed-by: Peter Xu <peterx@redhat.com> > Signed-off-by: Roman Kiryanov <rkir@google.com> > --- > v3: > - Also added assert to virtio.c to validate > VirtioDeviceClass instances which bypass > vmstate_register_with_alias_id. > - Updated the commit message to "Reviewed-by". > v2: > - Moved the assert from vmstate_save_state_v to the registration > path (vmstate_register_with_alias_id) and the qtest validation path > (vmstate_check) to catch missing fields earlier. > --- > hw/virtio/virtio.c | 6 ++++++ > migration/savevm.c | 5 +++++ > 2 files changed, 11 insertions(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 0ba734d0bc..e4543de335 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -4054,6 +4054,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp) > /* Devices should either use vmsd or the load/save methods */ > assert(!vdc->vmsd || !vdc->load); > > + /* > + * If a device has vmsd, it either MUST have valid fields > + * or marked unmigratable. > + */ > + assert(!vdc->vmsd || (vdc->vmsd->unmigratable || vdc->vmsd->fields)); > + > if (vdc->realize != NULL) { > vdc->realize(dev, &err); > if (err != NULL) { > diff --git a/migration/savevm.c b/migration/savevm.c > index 197c89e0e6..20c2b99231 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -861,6 +861,8 @@ static void vmstate_check(const VMStateDescription *vmsd) > const VMStateField *field = vmsd->fields; > const VMStateDescription * const *subsection = vmsd->subsections; > > + assert(field || vmsd->unmigratable); > + > if (field) { > while (field->name) { > if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) { > @@ -900,6 +902,9 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id, > /* If this triggers, alias support can be dropped for the vmsd. */ > assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id); > > + /* If vmsd is migratable it MUST have valid fields. */ > + assert(vmsd->fields || vmsd->unmigratable); > + > se = g_new0(SaveStateEntry, 1); > se->version_id = vmsd->version_id; > se->section_id = savevm_state.global_section_id++; Looks like we'll also need to fix some stubs that define empty vmsds: qemu-system-mips64: ../migration/savevm.c:864: void vmstate_check(const VMStateDescription *): Assertion `field || vmsd->unmigratable' failed. Maybe something like the following. Peter, what do you think? -- >8 -- From 9bd63109e970ff231eb321e627f622910f9977ca Mon Sep 17 00:00:00 2001 From: Fabiano Rosas <farosas@suse.de> Date: Mon, 16 Mar 2026 11:16:22 -0300 Subject: [PATCH] fixup! vmstate: validate VMStateDescription::fields upon registration --- hw/acpi/acpi-cpu-hotplug-stub.c | 4 +++- hw/acpi/acpi-mem-hotplug-stub.c | 4 +++- hw/acpi/acpi-pci-hotplug-stub.c | 4 +++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c index 72c5f05f5c..4332f3fb7d 100644 --- a/hw/acpi/acpi-cpu-hotplug-stub.c +++ b/hw/acpi/acpi-cpu-hotplug-stub.c @@ -3,7 +3,9 @@ #include "hw/acpi/cpu.h" /* Following stubs are all related to ACPI cpu hotplug */ -const VMStateDescription vmstate_cpu_hotplug; +const VMStateDescription vmstate_cpu_hotplug = { + .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() }, +}; void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner, CPUHotplugState *state, hwaddr base_addr) diff --git a/hw/acpi/acpi-mem-hotplug-stub.c b/hw/acpi/acpi-mem-hotplug-stub.c index 7ad0fdcdf2..36c12a4e5f 100644 --- a/hw/acpi/acpi-mem-hotplug-stub.c +++ b/hw/acpi/acpi-mem-hotplug-stub.c @@ -2,7 +2,9 @@ #include "hw/acpi/memory_hotplug.h" #include "migration/vmstate.h" -const VMStateDescription vmstate_memory_hotplug; +const VMStateDescription vmstate_memory_hotplug = { + .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() }, +}; void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner, MemHotplugState *state, hwaddr io_base) diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c index d58ea726a8..3e3a484d3e 100644 --- a/hw/acpi/acpi-pci-hotplug-stub.c +++ b/hw/acpi/acpi-pci-hotplug-stub.c @@ -2,7 +2,9 @@ #include "hw/acpi/pcihp.h" #include "migration/vmstate.h" -const VMStateDescription vmstate_acpi_pcihp_pci_status; +const VMStateDescription vmstate_acpi_pcihp_pci_status = { + .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() }, +}; void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, MemoryRegion *address_space_io, uint16_t io_base) -- 2.51.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration 2026-03-16 14:21 ` Fabiano Rosas @ 2026-03-16 15:42 ` Peter Xu 2026-03-16 15:57 ` Peter Maydell 2026-03-16 16:23 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 6+ messages in thread From: Peter Xu @ 2026-03-16 15:42 UTC (permalink / raw) To: Fabiano Rosas; +Cc: Roman Kiryanov, qemu-devel, whollins, jansene, jpcottin On Mon, Mar 16, 2026 at 11:21:31AM -0300, Fabiano Rosas wrote: > Looks like we'll also need to fix some stubs that define empty vmsds: > > qemu-system-mips64: ../migration/savevm.c:864: void vmstate_check(const > VMStateDescription *): Assertion `field || vmsd->unmigratable' failed. > > Maybe something like the following. Peter, what do you think? Yep, sounds ok. -- Peter Xu ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration 2026-03-16 14:21 ` Fabiano Rosas 2026-03-16 15:42 ` Peter Xu @ 2026-03-16 15:57 ` Peter Maydell 2026-03-16 16:23 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 6+ messages in thread From: Peter Maydell @ 2026-03-16 15:57 UTC (permalink / raw) To: Fabiano Rosas Cc: Roman Kiryanov, peterx, qemu-devel, whollins, jansene, jpcottin On Mon, 16 Mar 2026 at 14:22, Fabiano Rosas <farosas@suse.de> wrote: > > Roman Kiryanov <rkir@google.com> writes: > > > The vmstate_save_state_v() function does not support > > NULL in VMStateDescription::fields and will crash if > > one is provided. > > > > This change prevents this situation from happening > > by explicitly crashing earlier. > > > > Suggested-by: Fabiano Rosas <farosas@suse.de> > > Reviewed-by: Peter Xu <peterx@redhat.com> > > Signed-off-by: Roman Kiryanov <rkir@google.com> > > --- > > v3: > > - Also added assert to virtio.c to validate > > VirtioDeviceClass instances which bypass > > vmstate_register_with_alias_id. > > - Updated the commit message to "Reviewed-by". > > v2: > > - Moved the assert from vmstate_save_state_v to the registration > > path (vmstate_register_with_alias_id) and the qtest validation path > > (vmstate_check) to catch missing fields earlier. > > --- > > hw/virtio/virtio.c | 6 ++++++ > > migration/savevm.c | 5 +++++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index 0ba734d0bc..e4543de335 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -4054,6 +4054,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp) > > /* Devices should either use vmsd or the load/save methods */ > > assert(!vdc->vmsd || !vdc->load); > > > > + /* > > + * If a device has vmsd, it either MUST have valid fields > > + * or marked unmigratable. > > + */ > > + assert(!vdc->vmsd || (vdc->vmsd->unmigratable || vdc->vmsd->fields)); > > + > > if (vdc->realize != NULL) { > > vdc->realize(dev, &err); > > if (err != NULL) { > > diff --git a/migration/savevm.c b/migration/savevm.c > > index 197c89e0e6..20c2b99231 100644 > > --- a/migration/savevm.c > > +++ b/migration/savevm.c > > @@ -861,6 +861,8 @@ static void vmstate_check(const VMStateDescription *vmsd) > > const VMStateField *field = vmsd->fields; > > const VMStateDescription * const *subsection = vmsd->subsections; > > > > + assert(field || vmsd->unmigratable); > > + > > if (field) { > > while (field->name) { > > if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) { > > @@ -900,6 +902,9 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id, > > /* If this triggers, alias support can be dropped for the vmsd. */ > > assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id); > > > > + /* If vmsd is migratable it MUST have valid fields. */ > > + assert(vmsd->fields || vmsd->unmigratable); > > + > > se = g_new0(SaveStateEntry, 1); > > se->version_id = vmsd->version_id; > > se->section_id = savevm_state.global_section_id++; > > Looks like we'll also need to fix some stubs that define empty vmsds: > > qemu-system-mips64: ../migration/savevm.c:864: void vmstate_check(const > VMStateDescription *): Assertion `field || vmsd->unmigratable' failed. > > Maybe something like the following. Peter, what do you think? Ah, I was about to report this -- commit 7aa563630b6b0 ("pc: Start with modern CPU hotplug interface by default") broke the MIPS Malta board migration, which now segfaults when you "savevm" because the stub vmstate_cpu_hotplug struct has a NULL fields pointer. This used to work because the relevant vmstate had a "needed" function that meant we never used the stub vmstate structs, but that commit dropped the "needed" functions. If the change you suggest is the right way to fix this then it ought to be its own patch with a Fixes: tag pointing at 7aa563630b6b0. thanks -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration 2026-03-16 14:21 ` Fabiano Rosas 2026-03-16 15:42 ` Peter Xu 2026-03-16 15:57 ` Peter Maydell @ 2026-03-16 16:23 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2026-03-16 16:23 UTC (permalink / raw) To: Fabiano Rosas, Roman Kiryanov, peterx Cc: qemu-devel, whollins, jansene, jpcottin On 16/3/26 15:21, Fabiano Rosas wrote: > -- >8 -- > From 9bd63109e970ff231eb321e627f622910f9977ca Mon Sep 17 00:00:00 2001 > From: Fabiano Rosas <farosas@suse.de> > Date: Mon, 16 Mar 2026 11:16:22 -0300 > Subject: [PATCH] fixup! vmstate: validate VMStateDescription::fields upon > registration > > --- > hw/acpi/acpi-cpu-hotplug-stub.c | 4 +++- > hw/acpi/acpi-mem-hotplug-stub.c | 4 +++- > hw/acpi/acpi-pci-hotplug-stub.c | 4 +++- > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c > index 72c5f05f5c..4332f3fb7d 100644 > --- a/hw/acpi/acpi-cpu-hotplug-stub.c > +++ b/hw/acpi/acpi-cpu-hotplug-stub.c > @@ -3,7 +3,9 @@ > #include "hw/acpi/cpu.h" > > /* Following stubs are all related to ACPI cpu hotplug */ > -const VMStateDescription vmstate_cpu_hotplug; > +const VMStateDescription vmstate_cpu_hotplug = { > + .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() }, > +}; > > void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner, > CPUHotplugState *state, hwaddr base_addr) > diff --git a/hw/acpi/acpi-mem-hotplug-stub.c b/hw/acpi/acpi-mem-hotplug-stub.c > index 7ad0fdcdf2..36c12a4e5f 100644 > --- a/hw/acpi/acpi-mem-hotplug-stub.c > +++ b/hw/acpi/acpi-mem-hotplug-stub.c > @@ -2,7 +2,9 @@ > #include "hw/acpi/memory_hotplug.h" > #include "migration/vmstate.h" > > -const VMStateDescription vmstate_memory_hotplug; > +const VMStateDescription vmstate_memory_hotplug = { > + .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() }, > +}; > > void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner, > MemHotplugState *state, hwaddr io_base) > diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c > index d58ea726a8..3e3a484d3e 100644 > --- a/hw/acpi/acpi-pci-hotplug-stub.c > +++ b/hw/acpi/acpi-pci-hotplug-stub.c > @@ -2,7 +2,9 @@ > #include "hw/acpi/pcihp.h" > #include "migration/vmstate.h" > > -const VMStateDescription vmstate_acpi_pcihp_pci_status; > +const VMStateDescription vmstate_acpi_pcihp_pci_status = { > + .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() }, > +}; > > void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, > MemoryRegion *address_space_io, uint16_t io_base) Missing: -- >8 -- diff --git a/hw/display/ramfb-stubs.c b/hw/display/ramfb-stubs.c index b83551357bb..283af44ecec 100644 --- a/hw/display/ramfb-stubs.c +++ b/hw/display/ramfb-stubs.c @@ -2,7 +2,9 @@ #include "qapi/error.h" #include "hw/display/ramfb.h" -const VMStateDescription ramfb_vmstate = {}; +const VMStateDescription ramfb_vmstate = { + .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() }, +}; void ramfb_display_update(QemuConsole *con, RAMFBState *s) { --- Otherwise LGTM (agreeing with Peter to post separately as a preliminary Fixes patch). ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration 2026-03-13 23:35 [PATCH v3] vmstate: validate VMStateDescription::fields upon registration Roman Kiryanov 2026-03-16 14:21 ` Fabiano Rosas @ 2026-03-17 18:50 ` Fabiano Rosas 1 sibling, 0 replies; 6+ messages in thread From: Fabiano Rosas @ 2026-03-17 18:50 UTC (permalink / raw) To: Roman Kiryanov, peterx Cc: qemu-devel, whollins, jansene, jpcottin, Roman Kiryanov Roman Kiryanov <rkir@google.com> writes: > The vmstate_save_state_v() function does not support > NULL in VMStateDescription::fields and will crash if > one is provided. > > This change prevents this situation from happening > by explicitly crashing earlier. > > Suggested-by: Fabiano Rosas <farosas@suse.de> > Reviewed-by: Peter Xu <peterx@redhat.com> > Signed-off-by: Roman Kiryanov <rkir@google.com> > --- > v3: > - Also added assert to virtio.c to validate > VirtioDeviceClass instances which bypass > vmstate_register_with_alias_id. > - Updated the commit message to "Reviewed-by". > v2: > - Moved the assert from vmstate_save_state_v to the registration > path (vmstate_register_with_alias_id) and the qtest validation path > (vmstate_check) to catch missing fields earlier. > --- > hw/virtio/virtio.c | 6 ++++++ > migration/savevm.c | 5 +++++ > 2 files changed, 11 insertions(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 0ba734d0bc..e4543de335 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -4054,6 +4054,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp) > /* Devices should either use vmsd or the load/save methods */ > assert(!vdc->vmsd || !vdc->load); > > + /* > + * If a device has vmsd, it either MUST have valid fields > + * or marked unmigratable. > + */ > + assert(!vdc->vmsd || (vdc->vmsd->unmigratable || vdc->vmsd->fields)); > + > if (vdc->realize != NULL) { > vdc->realize(dev, &err); > if (err != NULL) { > diff --git a/migration/savevm.c b/migration/savevm.c > index 197c89e0e6..20c2b99231 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -861,6 +861,8 @@ static void vmstate_check(const VMStateDescription *vmsd) > const VMStateField *field = vmsd->fields; > const VMStateDescription * const *subsection = vmsd->subsections; > > + assert(field || vmsd->unmigratable); > + > if (field) { > while (field->name) { > if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) { > @@ -900,6 +902,9 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id, > /* If this triggers, alias support can be dropped for the vmsd. */ > assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id); > > + /* If vmsd is migratable it MUST have valid fields. */ > + assert(vmsd->fields || vmsd->unmigratable); > + > se = g_new0(SaveStateEntry, 1); > se->version_id = vmsd->version_id; > se->section_id = savevm_state.global_section_id++; Hi, this patch missed the train. It's crashing the ./scripts/device-crash-test The vmstate_virtio_snd_device needs to be fixed first. --- Some thoughts, no action needed: I'm cautions after the issue with the stubs. We should expect that a stub with all fields zeroed could still reach the vmstate code. Maybe it'll be fine with the (eventual) fix for mips, but we need to test it thoroughly. There is also the -dump-vmstate command line option, which can certainly process vmsds with incorrect fields as it takes them directly from the device class. It doesn't crash, but prints stuff like: "Fields": [{ "field": "cpuhp_state", "version_id": 1, "field_exists": false, "size": 304, "Description": { "name": "(null)", <--- "version_id": 0, "minimum_version_id": 0 }}] Perhaps it should even do a validation pass and warn or add a comment to the output. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-17 18:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-13 23:35 [PATCH v3] vmstate: validate VMStateDescription::fields upon registration Roman Kiryanov 2026-03-16 14:21 ` Fabiano Rosas 2026-03-16 15:42 ` Peter Xu 2026-03-16 15:57 ` Peter Maydell 2026-03-16 16:23 ` Philippe Mathieu-Daudé 2026-03-17 18:50 ` Fabiano Rosas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.