migration/vmstate.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
migration/savevm.c contains some calls to vmstate_save() that are
followed by migrate_set_error() if the integer return value indicates an
error. migrate_set_error() requires that the `Error *` object passed to
it is set. Therefore, vmstate_save() is assumed to always set *errp on
error.
Right now, that assumption is not met: vmstate_save_state_v() (called
internally by vmstate_save()) will not set *errp if
vmstate_subsection_save() or vmsd->post_save() fail. Fix that by adding
an *errp parameter to vmstate_subsection_save(), and by generating a
generic error in case post_save() fails (as is already done for
pre_save()).
Without this patch, qemu will crash after vmstate_subsection_save() or
post_save() have failed inside of a vmstate_save() call (unless
migrate_set_error() then happen to discard the new error because
s->error is already set). This happens e.g. when receiving the state
from a virtio-fs back-end (virtiofsd) fails.
Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
migration/vmstate.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index ff5d589a6d..13532f2807 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -22,7 +22,8 @@
#include "trace.h"
static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, JSONWriter *vmdesc);
+ void *opaque, JSONWriter *vmdesc,
+ Error **errp);
static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque);
@@ -441,12 +442,13 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
json_writer_end_array(vmdesc);
}
- ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc);
+ ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
if (vmsd->post_save) {
int ps_ret = vmsd->post_save(opaque);
if (!ret) {
ret = ps_ret;
+ error_setg(errp, "post-save failed: %s", vmsd->name);
}
}
return ret;
@@ -518,7 +520,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
}
static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, JSONWriter *vmdesc)
+ void *opaque, JSONWriter *vmdesc,
+ Error **errp)
{
const VMStateDescription * const *sub = vmsd->subsections;
bool vmdesc_has_subsections = false;
@@ -546,7 +549,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
qemu_put_byte(f, len);
qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
qemu_put_be32(f, vmsdsub->version_id);
- ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc);
+ ret = vmstate_save_state_with_err(f, vmsdsub, opaque, vmdesc, errp);
if (ret) {
return ret;
}
--
2.45.2On Tue, Oct 15, 2024 at 04:15:15PM +0200, Hanna Czenczek wrote:
> migration/savevm.c contains some calls to vmstate_save() that are
> followed by migrate_set_error() if the integer return value indicates an
> error. migrate_set_error() requires that the `Error *` object passed to
> it is set. Therefore, vmstate_save() is assumed to always set *errp on
> error.
>
> Right now, that assumption is not met: vmstate_save_state_v() (called
> internally by vmstate_save()) will not set *errp if
> vmstate_subsection_save() or vmsd->post_save() fail. Fix that by adding
> an *errp parameter to vmstate_subsection_save(), and by generating a
> generic error in case post_save() fails (as is already done for
> pre_save()).
>
> Without this patch, qemu will crash after vmstate_subsection_save() or
> post_save() have failed inside of a vmstate_save() call (unless
> migrate_set_error() then happen to discard the new error because
> s->error is already set). This happens e.g. when receiving the state
> from a virtio-fs back-end (virtiofsd) fails.
>
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
> migration/vmstate.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index ff5d589a6d..13532f2807 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -22,7 +22,8 @@
> #include "trace.h"
>
> static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
> - void *opaque, JSONWriter *vmdesc);
> + void *opaque, JSONWriter *vmdesc,
> + Error **errp);
> static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque);
>
> @@ -441,12 +442,13 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> json_writer_end_array(vmdesc);
> }
>
> - ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc);
> + ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
>
> if (vmsd->post_save) {
> int ps_ret = vmsd->post_save(opaque);
> if (!ret) {
Perhaps here it needs to be "if (!ret && ps_ret)" now, otherwise the error
will be attached even if no error for both retvals?
Other than that it looks good.
Thanks,
> ret = ps_ret;
> + error_setg(errp, "post-save failed: %s", vmsd->name);
> }
> }
> return ret;
> @@ -518,7 +520,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
> - void *opaque, JSONWriter *vmdesc)
> + void *opaque, JSONWriter *vmdesc,
> + Error **errp)
> {
> const VMStateDescription * const *sub = vmsd->subsections;
> bool vmdesc_has_subsections = false;
> @@ -546,7 +549,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
> qemu_put_byte(f, len);
> qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
> qemu_put_be32(f, vmsdsub->version_id);
> - ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc);
> + ret = vmstate_save_state_with_err(f, vmsdsub, opaque, vmdesc, errp);
> if (ret) {
> return ret;
> }
> --
> 2.45.2
>
--
Peter Xu
On 15.10.24 18:06, Peter Xu wrote:
> On Tue, Oct 15, 2024 at 04:15:15PM +0200, Hanna Czenczek wrote:
>> migration/savevm.c contains some calls to vmstate_save() that are
>> followed by migrate_set_error() if the integer return value indicates an
>> error. migrate_set_error() requires that the `Error *` object passed to
>> it is set. Therefore, vmstate_save() is assumed to always set *errp on
>> error.
>>
>> Right now, that assumption is not met: vmstate_save_state_v() (called
>> internally by vmstate_save()) will not set *errp if
>> vmstate_subsection_save() or vmsd->post_save() fail. Fix that by adding
>> an *errp parameter to vmstate_subsection_save(), and by generating a
>> generic error in case post_save() fails (as is already done for
>> pre_save()).
>>
>> Without this patch, qemu will crash after vmstate_subsection_save() or
>> post_save() have failed inside of a vmstate_save() call (unless
>> migrate_set_error() then happen to discard the new error because
>> s->error is already set). This happens e.g. when receiving the state
>> from a virtio-fs back-end (virtiofsd) fails.
>>
>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>> ---
>> migration/vmstate.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index ff5d589a6d..13532f2807 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -22,7 +22,8 @@
>> #include "trace.h"
>>
>> static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>> - void *opaque, JSONWriter *vmdesc);
>> + void *opaque, JSONWriter *vmdesc,
>> + Error **errp);
>> static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>> void *opaque);
>>
>> @@ -441,12 +442,13 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>> json_writer_end_array(vmdesc);
>> }
>>
>> - ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc);
>> + ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
>>
>> if (vmsd->post_save) {
>> int ps_ret = vmsd->post_save(opaque);
>> if (!ret) {
> Perhaps here it needs to be "if (!ret && ps_ret)" now, otherwise the error
> will be attached even if no error for both retvals?
Not just perhaps, that seems kind of vital indeed...
(I blame my brain’s less-than-stellar pattern matching that might have
interpreted this as the error path *cough*)
> Other than that it looks good.
Thanks for the quick review! Will respin.
Hanna
>
> Thanks,
>
>> ret = ps_ret;
>> + error_setg(errp, "post-save failed: %s", vmsd->name);
>> }
>> }
>> return ret;
>> @@ -518,7 +520,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>> }
>>
>> static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>> - void *opaque, JSONWriter *vmdesc)
>> + void *opaque, JSONWriter *vmdesc,
>> + Error **errp)
>> {
>> const VMStateDescription * const *sub = vmsd->subsections;
>> bool vmdesc_has_subsections = false;
>> @@ -546,7 +549,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>> qemu_put_byte(f, len);
>> qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
>> qemu_put_be32(f, vmsdsub->version_id);
>> - ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc);
>> + ret = vmstate_save_state_with_err(f, vmsdsub, opaque, vmdesc, errp);
>> if (ret) {
>> return ret;
>> }
>> --
>> 2.45.2
>>
© 2016 - 2024 Red Hat, Inc.