Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
在 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.bad2017-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
---
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
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()) {
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
Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
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.bad2017-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
> >>---
> >> 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
> >
> >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()) {
> > 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
在 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.bad2017-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
---
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
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()) {
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
在 2017/6/5 19:08, Dr. David Alan Gilbert 写道:
* QingFeng Hao (ha...@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.bad2017-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
---
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
Thanks David!
migration_incoming_state_destroy();
--
2.11.2
--
Dr. David Alan Gilbert / dgilb...@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
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.out2017-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
> ---
> 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
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()) {
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 (ha...@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.out2017-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
> ---
> 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
>
> migration_incoming_state_destroy();
> --
> 2.11.2
>
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
