Message ID | 20181012025623.38792-1-jialina01@baidu.com |
---|---|
State | New |
Headers | show |
Series | migration: avoid segmentfault when take a snapshot of a VM which being migrated | expand |
* jialina01 (jialina01@baidu.com) wrote: > During an active background migraion, snapshot will trigger a > segmentfault. As snapshot clears the "current_migration" struct > and updates "to_dst_file" before it finds out that there is a > migration task, Migration accesses the null pointer in > "current_migration" struct and qemu crashes eventually. > > Signed-off-by: jialina01 <jialina01@baidu.com> > Signed-off-by: chaiwen <chaiwen@baidu.com> > Signed-off-by: zhangyu <zhangyu31@baidu.com> Yes, that looks like an improvement, but is it enough? With that change does qemu_savevm_state fail cleanly if attempted during a migration? I suspect it will still try and do a snapshot, because I don't see where it's checking the current state (like the check in migrate_prepare that does a QERR_MIGRATIION_ACTIVE). Dave > --- > migration/savevm.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/migration/savevm.c b/migration/savevm.c > index 2d10e45582..9cb97ca343 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -1319,21 +1319,18 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp) > MigrationState *ms = migrate_get_current(); > MigrationStatus status; > > - migrate_init(ms); > - > - ms->to_dst_file = f; > - > if (migration_is_blocked(errp)) { > - ret = -EINVAL; > - goto done; > + return -EINVAL; > } > > if (migrate_use_block()) { > error_setg(errp, "Block migration and snapshots are incompatible"); > - ret = -EINVAL; > - goto done; > + return -EINVAL; > } > > + migrate_init(ms); > + ms->to_dst_file = f; > + > qemu_mutex_unlock_iothread(); > qemu_savevm_state_header(f); > qemu_savevm_state_setup(f); > @@ -1355,7 +1352,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp) > error_setg_errno(errp, -ret, "Error while writing VM state"); > } > > -done: > if (ret != 0) { > status = MIGRATION_STATUS_FAILED; > } else { > -- > 2.13.2.windows.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
在 2018/10/12 下午5:01, "Dr. David Alan Gilbert" <dgilbert@redhat.com> 写入: >* jialina01 (jialina01@baidu.com) wrote: >> During an active background migraion, snapshot will trigger a >> segmentfault. As snapshot clears the "current_migration" struct >> and updates "to_dst_file" before it finds out that there is a >> migration task, Migration accesses the null pointer in >> "current_migration" struct and qemu crashes eventually. >> >> Signed-off-by: jialina01 <jialina01@baidu.com> >> Signed-off-by: chaiwen <chaiwen@baidu.com> >> Signed-off-by: zhangyu <zhangyu31@baidu.com> > >Yes, that looks like an improvement, but is it enough? Indeed it’s not enough, this patch fails to handle the case that doing a snapshot under an active non-block migration. >With that change does qemu_savevm_state fail cleanly if attempted >during a migration? I suspect it will still try and do a snapshot, >because I don't see where it's checking the current state >(like the check in migrate_prepare that does a QERR_MIGRATIION_ACTIVE). Yes, migration_is_setup_or_active looks like a more sensible detection for this case, and a proper error message it better to be set. Thanks for your comment, we will send a v2 patch later. Thanks Chai Wen > >Dave > > >> --- >> migration/savevm.c | 14 +++++--------- >> 1 file changed, 5 insertions(+), 9 deletions(-) >> >> diff --git a/migration/savevm.c b/migration/savevm.c >> index 2d10e45582..9cb97ca343 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -1319,21 +1319,18 @@ static int qemu_savevm_state(QEMUFile *f, Error >>**errp) >> MigrationState *ms = migrate_get_current(); >> MigrationStatus status; >> >> - migrate_init(ms); >> - >> - ms->to_dst_file = f; >> - >> if (migration_is_blocked(errp)) { >> - ret = -EINVAL; >> - goto done; >> + return -EINVAL; >> } >> >> if (migrate_use_block()) { >> error_setg(errp, "Block migration and snapshots are >>incompatible"); >> - ret = -EINVAL; >> - goto done; >> + return -EINVAL; >> } >> >> + migrate_init(ms); >> + ms->to_dst_file = f; >> + >> qemu_mutex_unlock_iothread(); >> qemu_savevm_state_header(f); >> qemu_savevm_state_setup(f); >> @@ -1355,7 +1352,6 @@ static int qemu_savevm_state(QEMUFile *f, Error >>**errp) >> error_setg_errno(errp, -ret, "Error while writing VM state"); >> } >> >> -done: >> if (ret != 0) { >> status = MIGRATION_STATUS_FAILED; >> } else { >> -- >> 2.13.2.windows.1 >> >-- >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/savevm.c b/migration/savevm.c index 2d10e45582..9cb97ca343 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1319,21 +1319,18 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp) MigrationState *ms = migrate_get_current(); MigrationStatus status; - migrate_init(ms); - - ms->to_dst_file = f; - if (migration_is_blocked(errp)) { - ret = -EINVAL; - goto done; + return -EINVAL; } if (migrate_use_block()) { error_setg(errp, "Block migration and snapshots are incompatible"); - ret = -EINVAL; - goto done; + return -EINVAL; } + migrate_init(ms); + ms->to_dst_file = f; + qemu_mutex_unlock_iothread(); qemu_savevm_state_header(f); qemu_savevm_state_setup(f); @@ -1355,7 +1352,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp) error_setg_errno(errp, -ret, "Error while writing VM state"); } -done: if (ret != 0) { status = MIGRATION_STATUS_FAILED; } else {