| Message ID | 20241015141515.150754-1-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Ensure vmstate_save() sets errp | expand |
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? 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 >
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 >>
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; }
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(-)