[Qemu-devel] [PATCH v1 0/1] qemu/migration: fix the migration bug found by qemu-iotests case 068

[Qemu-devel] [PATCH v1 0/1] qemu/migration: fix the migration bug found by qemu-iotests case 068

[QingFeng Hao]

Hi all,
This patch is to fix the migration bug found by qemu-iotests case 068
and based on upstream master's commit:
cb8b8ef4578:  Merge remote-tracking branch 'remotes/elmarco/tags/chrfe-pull-request' into staging.
The bug was introduced by commit "660819b migration: shut src return path unconditionally".
Thanks!

QingFeng Hao (1):
  qemu/migration: fix the double free problem on from_src_file

 migration/savevm.c | 1 -
 1 file changed, 1 deletion(-)

-- 
2.11.2

[Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file

[QingFeng Hao]

In load_vmstate, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.

This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
    --- tests/qemu-iotests/068.out	2017-05-06 01:00:26.417270437 +0200
    +++ 068.out.bad	2017-06-03 13:59:55.360274640 +0200
    @@ -6,6 +6,8 @@
     QEMU X.Y.Z monitor - type 'help' for more information
     (qemu) savevm 0
     (qemu) quit
    +./common.config: line 107: 242472 Illegal instruction     (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
    +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
    +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
     QEMU X.Y.Z monitor - type 'help' for more information
    -(qemu) quit
    -*** done
    +(qemu) *** done

Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
---
 migration/savevm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
 
     aio_context_acquire(aio_context);
     ret = qemu_loadvm_state(f);
-    qemu_fclose(f);
     aio_context_release(aio_context);
 
     migration_incoming_state_destroy();
-- 
2.11.2

Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file

[Dr. David Alan Gilbert]

* QingFeng Hao (haoqf@linux.vnet.ibm.com) wrote:
> In load_vmstate, mis->from_src_file is freed twice, the first free is by
> qemu_fclose, the second is by migration_incoming_state_destroy and
> it causes Illegal instruction exception. The fix is just to remove the
> first free.
> 
> This problem is found by qemu-iotests case 068 since commit
> "660819b migration: shut src return path unconditionally". The error is:
> 068 1s ... - output mismatch (see 068.out.bad)
>     --- tests/qemu-iotests/068.out	2017-05-06 01:00:26.417270437 +0200
>     +++ 068.out.bad	2017-06-03 13:59:55.360274640 +0200
>     @@ -6,6 +6,8 @@
>      QEMU X.Y.Z monitor - type 'help' for more information
>      (qemu) savevm 0
>      (qemu) quit
>     +./common.config: line 107: 242472 Illegal instruction     (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
>     +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>     +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>      QEMU X.Y.Z monitor - type 'help' for more information
>     -(qemu) quit
>     -*** done
>     +(qemu) *** done
> 
> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
> ---
>  migration/savevm.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9c320f59d0..853e14e34e 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
>  
>      aio_context_acquire(aio_context);
>      ret = qemu_loadvm_state(f);
> -    qemu_fclose(f);
>      aio_context_release(aio_context);

Thanks!

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  
>      migration_incoming_state_destroy();
> -- 
> 2.11.2
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file

[Peter Xu]

On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
> In load_vmstate, mis->from_src_file is freed twice, the first free is by
> qemu_fclose, the second is by migration_incoming_state_destroy and
> it causes Illegal instruction exception. The fix is just to remove the
> first free.
> 
> This problem is found by qemu-iotests case 068 since commit
> "660819b migration: shut src return path unconditionally". The error is:
> 068 1s ... - output mismatch (see 068.out.bad)
>     --- tests/qemu-iotests/068.out	2017-05-06 01:00:26.417270437 +0200
>     +++ 068.out.bad	2017-06-03 13:59:55.360274640 +0200
>     @@ -6,6 +6,8 @@
>      QEMU X.Y.Z monitor - type 'help' for more information
>      (qemu) savevm 0
>      (qemu) quit
>     +./common.config: line 107: 242472 Illegal instruction     (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
>     +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>     +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>      QEMU X.Y.Z monitor - type 'help' for more information
>     -(qemu) quit
>     -*** done
>     +(qemu) *** done
> 
> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
> ---
>  migration/savevm.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9c320f59d0..853e14e34e 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
>  
>      aio_context_acquire(aio_context);
>      ret = qemu_loadvm_state(f);
> -    qemu_fclose(f);
>      aio_context_release(aio_context);
>  
>      migration_incoming_state_destroy();

Thanks QingFeng for the fix!

Reviewed-by: Peter Xu <peterx@redhat.com>

Though here instead of removing the fclose(), not sure whether this is
nicer:

diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f5..1feb519 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
     QEMUFile *f;
     int ret;
     AioContext *aio_context;
-    MigrationIncomingState *mis = migration_incoming_get_current();
 
     if (!bdrv_all_can_snapshot(&bs)) {
         error_setg(errp,
@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
     }
 
     qemu_system_reset(SHUTDOWN_CAUSE_NONE);
-    mis->from_src_file = f;
 
     aio_context_acquire(aio_context);
     ret = qemu_loadvm_state(f);

Since I see we setup from_src_file but we don't really use it. If we
remove that line, we can also remove the
migration_incoming_get_current() call altogether.

Either way work for me. So my r-b stands always. :-)

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file

[QingFeng Hao]

在 2017/6/5 19:08, Dr. David Alan Gilbert 写道:
> * QingFeng Hao (haoqf@linux.vnet.ibm.com) wrote:
>> In load_vmstate, mis->from_src_file is freed twice, the first free is by
>> qemu_fclose, the second is by migration_incoming_state_destroy and
>> it causes Illegal instruction exception. The fix is just to remove the
>> first free.
>>
>> This problem is found by qemu-iotests case 068 since commit
>> "660819b migration: shut src return path unconditionally". The error is:
>> 068 1s ... - output mismatch (see 068.out.bad)
>>      --- tests/qemu-iotests/068.out	2017-05-06 01:00:26.417270437 +0200
>>      +++ 068.out.bad	2017-06-03 13:59:55.360274640 +0200
>>      @@ -6,6 +6,8 @@
>>       QEMU X.Y.Z monitor - type 'help' for more information
>>       (qemu) savevm 0
>>       (qemu) quit
>>      +./common.config: line 107: 242472 Illegal instruction     (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
>>      +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>>      +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>>       QEMU X.Y.Z monitor - type 'help' for more information
>>      -(qemu) quit
>>      -*** done
>>      +(qemu) *** done
>>
>> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
>> ---
>>   migration/savevm.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 9c320f59d0..853e14e34e 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
>>   
>>       aio_context_acquire(aio_context);
>>       ret = qemu_loadvm_state(f);
>> -    qemu_fclose(f);
>>       aio_context_release(aio_context);
> Thanks!
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Thanks David!
>
>>   
>>       migration_incoming_state_destroy();
>> -- 
>> 2.11.2
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

-- 
Regards
QingFeng Hao

Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file

[QingFeng Hao]

在 2017/6/6 11:03, Peter Xu 写道:
> On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
>> In load_vmstate, mis->from_src_file is freed twice, the first free is by
>> qemu_fclose, the second is by migration_incoming_state_destroy and
>> it causes Illegal instruction exception. The fix is just to remove the
>> first free.
>>
>> This problem is found by qemu-iotests case 068 since commit
>> "660819b migration: shut src return path unconditionally". The error is:
>> 068 1s ... - output mismatch (see 068.out.bad)
>>      --- tests/qemu-iotests/068.out	2017-05-06 01:00:26.417270437 +0200
>>      +++ 068.out.bad	2017-06-03 13:59:55.360274640 +0200
>>      @@ -6,6 +6,8 @@
>>       QEMU X.Y.Z monitor - type 'help' for more information
>>       (qemu) savevm 0
>>       (qemu) quit
>>      +./common.config: line 107: 242472 Illegal instruction     (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
>>      +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>>      +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>>       QEMU X.Y.Z monitor - type 'help' for more information
>>      -(qemu) quit
>>      -*** done
>>      +(qemu) *** done
>>
>> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
>> ---
>>   migration/savevm.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 9c320f59d0..853e14e34e 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
>>   
>>       aio_context_acquire(aio_context);
>>       ret = qemu_loadvm_state(f);
>> -    qemu_fclose(f);
>>       aio_context_release(aio_context);
>>   
>>       migration_incoming_state_destroy();
> Thanks QingFeng for the fix!
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Though here instead of removing the fclose(), not sure whether this is
> nicer:
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9c320f5..1feb519 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
>       QEMUFile *f;
>       int ret;
>       AioContext *aio_context;
> -    MigrationIncomingState *mis = migration_incoming_get_current();
>
>       if (!bdrv_all_can_snapshot(&bs)) {
>           error_setg(errp,
> @@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
>       }
>
>       qemu_system_reset(SHUTDOWN_CAUSE_NONE);
> -    mis->from_src_file = f;
>
>       aio_context_acquire(aio_context);
>       ret = qemu_loadvm_state(f);
Thanks Peter. I have a doubt on this change because I see there are 
several places
referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and 
it seems to
get byte from the file and it's called by qemu_loadvm_state.
Anyway, you remind me for the description that is "In load_snapshot" 
rather than
"In load_vmstate". thanks
> Since I see we setup from_src_file but we don't really use it. If we
> remove that line, we can also remove the
> migration_incoming_get_current() call altogether.
>
> Either way work for me. So my r-b stands always. :-)
>

-- 
Regards
QingFeng Hao

Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file

[Peter Xu]

On Tue, Jun 06, 2017 at 11:38:05AM +0800, QingFeng Hao wrote:
> 
> 
> 在 2017/6/6 11:03, Peter Xu 写道:
> >On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
> >>In load_vmstate, mis->from_src_file is freed twice, the first free is by
> >>qemu_fclose, the second is by migration_incoming_state_destroy and
> >>it causes Illegal instruction exception. The fix is just to remove the
> >>first free.
> >>
> >>This problem is found by qemu-iotests case 068 since commit
> >>"660819b migration: shut src return path unconditionally". The error is:
> >>068 1s ... - output mismatch (see 068.out.bad)
> >>     --- tests/qemu-iotests/068.out	2017-05-06 01:00:26.417270437 +0200
> >>     +++ 068.out.bad	2017-06-03 13:59:55.360274640 +0200
> >>     @@ -6,6 +6,8 @@
> >>      QEMU X.Y.Z monitor - type 'help' for more information
> >>      (qemu) savevm 0
> >>      (qemu) quit
> >>     +./common.config: line 107: 242472 Illegal instruction     (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
> >>     +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> >>     +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
> >>      QEMU X.Y.Z monitor - type 'help' for more information
> >>     -(qemu) quit
> >>     -*** done
> >>     +(qemu) *** done
> >>
> >>Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
> >>---
> >>  migration/savevm.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >>diff --git a/migration/savevm.c b/migration/savevm.c
> >>index 9c320f59d0..853e14e34e 100644
> >>--- a/migration/savevm.c
> >>+++ b/migration/savevm.c
> >>@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
> >>      aio_context_acquire(aio_context);
> >>      ret = qemu_loadvm_state(f);
> >>-    qemu_fclose(f);
> >>      aio_context_release(aio_context);
> >>      migration_incoming_state_destroy();
> >Thanks QingFeng for the fix!
> >
> >Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> >Though here instead of removing the fclose(), not sure whether this is
> >nicer:
> >
> >diff --git a/migration/savevm.c b/migration/savevm.c
> >index 9c320f5..1feb519 100644
> >--- a/migration/savevm.c
> >+++ b/migration/savevm.c
> >@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
> >      QEMUFile *f;
> >      int ret;
> >      AioContext *aio_context;
> >-    MigrationIncomingState *mis = migration_incoming_get_current();
> >
> >      if (!bdrv_all_can_snapshot(&bs)) {
> >          error_setg(errp,
> >@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
> >      }
> >
> >      qemu_system_reset(SHUTDOWN_CAUSE_NONE);
> >-    mis->from_src_file = f;
> >
> >      aio_context_acquire(aio_context);
> >      ret = qemu_loadvm_state(f);
> Thanks Peter. I have a doubt on this change because I see there are several
> places
> referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and it
> seems to
> get byte from the file and it's called by qemu_loadvm_state.
> Anyway, you remind me for the description that is "In load_snapshot" rather
> than
> "In load_vmstate". thanks

Oh I didn't really notice that. :)

It shouldn't affect imho since load snapshot won't trigger postcopy
codes. But sure current fix is good enough for me at least.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file

[QingFeng Hao]

在 2017/6/6 11:50, Peter Xu 写道:
> On Tue, Jun 06, 2017 at 11:38:05AM +0800, QingFeng Hao wrote:
>>
>> 在 2017/6/6 11:03, Peter Xu 写道:
>>> On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
>>>> In load_vmstate, mis->from_src_file is freed twice, the first free is by
>>>> qemu_fclose, the second is by migration_incoming_state_destroy and
>>>> it causes Illegal instruction exception. The fix is just to remove the
>>>> first free.
>>>>
>>>> This problem is found by qemu-iotests case 068 since commit
>>>> "660819b migration: shut src return path unconditionally". The error is:
>>>> 068 1s ... - output mismatch (see 068.out.bad)
>>>>      --- tests/qemu-iotests/068.out	2017-05-06 01:00:26.417270437 +0200
>>>>      +++ 068.out.bad	2017-06-03 13:59:55.360274640 +0200
>>>>      @@ -6,6 +6,8 @@
>>>>       QEMU X.Y.Z monitor - type 'help' for more information
>>>>       (qemu) savevm 0
>>>>       (qemu) quit
>>>>      +./common.config: line 107: 242472 Illegal instruction     (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
>>>>      +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>>>>      +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>>>>       QEMU X.Y.Z monitor - type 'help' for more information
>>>>      -(qemu) quit
>>>>      -*** done
>>>>      +(qemu) *** done
>>>>
>>>> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
>>>> ---
>>>>   migration/savevm.c | 1 -
>>>>   1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>> index 9c320f59d0..853e14e34e 100644
>>>> --- a/migration/savevm.c
>>>> +++ b/migration/savevm.c
>>>> @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
>>>>       aio_context_acquire(aio_context);
>>>>       ret = qemu_loadvm_state(f);
>>>> -    qemu_fclose(f);
>>>>       aio_context_release(aio_context);
>>>>       migration_incoming_state_destroy();
>>> Thanks QingFeng for the fix!
>>>
>>> Reviewed-by: Peter Xu <peterx@redhat.com>
>>>
>>> Though here instead of removing the fclose(), not sure whether this is
>>> nicer:
>>>
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index 9c320f5..1feb519 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
>>>       QEMUFile *f;
>>>       int ret;
>>>       AioContext *aio_context;
>>> -    MigrationIncomingState *mis = migration_incoming_get_current();
>>>
>>>       if (!bdrv_all_can_snapshot(&bs)) {
>>>           error_setg(errp,
>>> @@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
>>>       }
>>>
>>>       qemu_system_reset(SHUTDOWN_CAUSE_NONE);
>>> -    mis->from_src_file = f;
>>>
>>>       aio_context_acquire(aio_context);
>>>       ret = qemu_loadvm_state(f);
>> Thanks Peter. I have a doubt on this change because I see there are several
>> places
>> referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and it
>> seems to
>> get byte from the file and it's called by qemu_loadvm_state.
>> Anyway, you remind me for the description that is "In load_snapshot" rather
>> than
>> "In load_vmstate". thanks
> Oh I didn't really notice that. :)
>
> It shouldn't affect imho since load snapshot won't trigger postcopy
> codes. But sure current fix is good enough for me at least.
Ok, thanks!
> Thanks,
>

-- 
Regards
QingFeng Hao