migration: avoid segmentfault when take a snapshot of a VM which being migrated

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>
---
 migration/savevm.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

* 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