* [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue
@ 2015-02-09 6:50 Bin Wu
2015-02-09 9:09 ` Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Bin Wu @ 2015-02-09 6:50 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, famz, boby.chen, subo7, kathy.wangting, rudy.zhangmin,
arei.gonglei, stefanha, pbonzini, bruce.fon, Bin Wu
From: Bin Wu <wu.wubin@huawei.com>
We tested VMs migration with their disk images by drive_mirror. With
migration, two VMs copyed large files between each other. During the
test, a segfault occured. The stack was as follow:
(gdb) bt
qemu-coroutine-lock.c:66
to=0x7fa5a1798648) at qemu-coroutine.c:97
request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
block/nbd-client.c:165
sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
block/nbd-client.c:262
sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
block/nbd-client.c:296
nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
flags=0) at block.c:3321
offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
block.c:3447
sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
block.c:3471
nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
flags=0) at block.c:3321
offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
block.c:3447
sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
block.c:3471
coroutine-ucontext.c:121
After analyzing the stack and reviewing the code, we find the
qemu_co_queue_run_restart should not be put in the coroutine_swap function which
can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
qemu_coroutine_enter needs to restart the co_queue.
The error scenario is as follow: coroutine C1 enters C2, C2 yields
back to C1, then C1 ternimates and the related coroutine memory
becomes invalid. After a while, the C2 coroutine is entered again.
At this point, C1 is used as a parameter passed to
qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
accesses an invalid memory and a segfault error ocurrs.
The qemu_co_queue_run_restart function re-enters coroutines waiting
in the co_queue. However, this function should be only used int the
qemu_coroutine_enter context. Only in this context, when the current
coroutine gets execution control again(after the execution of
qemu_coroutine_switch), we can restart the target coutine because the
target coutine has yielded back to the current coroutine or it has
terminated.
First we want to put qemu_co_queue_run_restart in qemu_coroutine_enter,
but we find we can not access the target coroutine if it terminates.
Signed-off-by: Bin Wu <wu.wubin@huawei.com>
---
qemu-coroutine.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/qemu-coroutine.c b/qemu-coroutine.c
index 525247b..cc0bdfa 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -99,29 +99,31 @@ static void coroutine_delete(Coroutine *co)
qemu_coroutine_delete(co);
}
-static void coroutine_swap(Coroutine *from, Coroutine *to)
+static CoroutineAction coroutine_swap(Coroutine *from, Coroutine *to)
{
CoroutineAction ret;
ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
- qemu_co_queue_run_restart(to);
-
switch (ret) {
case COROUTINE_YIELD:
- return;
+ break;
case COROUTINE_TERMINATE:
trace_qemu_coroutine_terminate(to);
+ qemu_co_queue_run_restart(to);
coroutine_delete(to);
- return;
+ break;
default:
abort();
}
+
+ return ret;
}
void qemu_coroutine_enter(Coroutine *co, void *opaque)
{
Coroutine *self = qemu_coroutine_self();
+ CoroutineAction ret;
trace_qemu_coroutine_enter(self, co, opaque);
@@ -132,7 +134,9 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
co->caller = self;
co->entry_arg = opaque;
- coroutine_swap(self, co);
+ ret = coroutine_swap(self, co);
+ if (ret == COROUTINE_YIELD)
+ qemu_co_queue_run_restart(co);
}
void coroutine_fn qemu_coroutine_yield(void)
--
1.7.12.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue
2015-02-09 6:50 [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue Bin Wu
@ 2015-02-09 9:09 ` Paolo Bonzini
2015-02-10 0:55 ` Bin Wu
2015-02-09 9:42 ` Kevin Wolf
2015-02-09 14:48 ` Stefan Hajnoczi
2 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2015-02-09 9:09 UTC (permalink / raw)
To: Bin Wu, qemu-devel
Cc: kwolf, famz, stefanha, subo7, kathy.wangting, bruce.fon,
arei.gonglei, boby.chen, rudy.zhangmin
On 09/02/2015 07:50, Bin Wu wrote:
> From: Bin Wu <wu.wubin@huawei.com>
>
> We tested VMs migration with their disk images by drive_mirror. With
> migration, two VMs copyed large files between each other. During the
> test, a segfault occured. The stack was as follow:
>
> (gdb) bt
> qemu-coroutine-lock.c:66
> to=0x7fa5a1798648) at qemu-coroutine.c:97
> request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
> block/nbd-client.c:165
> sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
> block/nbd-client.c:262
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
> block/nbd-client.c:296
> nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
> req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
> flags=0) at block.c:3321
> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3447
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3471
> nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
> nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
> req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
> flags=0) at block.c:3321
> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3447
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3471
> coroutine-ucontext.c:121
>
> After analyzing the stack and reviewing the code, we find the
> qemu_co_queue_run_restart should not be put in the coroutine_swap function which
> can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
> qemu_coroutine_enter needs to restart the co_queue.
>
> The error scenario is as follow: coroutine C1 enters C2, C2 yields
> back to C1, then C1 ternimates and the related coroutine memory
> becomes invalid. After a while, the C2 coroutine is entered again.
> At this point, C1 is used as a parameter passed to
> qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
> accesses an invalid memory and a segfault error ocurrs.
>
> The qemu_co_queue_run_restart function re-enters coroutines waiting
> in the co_queue. However, this function should be only used int the
> qemu_coroutine_enter context. Only in this context, when the current
> coroutine gets execution control again(after the execution of
> qemu_coroutine_switch), we can restart the target coutine because the
> target coutine has yielded back to the current coroutine or it has
> terminated.
qemu_coroutine_yield can be executed for other reasons than locks. In
those cases, it is correct to call qemu_co_queue_run_restart. I think
it's an NBD bug.
Paolo
> First we want to put qemu_co_queue_run_restart in qemu_coroutine_enter,
> but we find we can not access the target coroutine if it terminates.
>
> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
> ---
> qemu-coroutine.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
> index 525247b..cc0bdfa 100644
> --- a/qemu-coroutine.c
> +++ b/qemu-coroutine.c
> @@ -99,29 +99,31 @@ static void coroutine_delete(Coroutine *co)
> qemu_coroutine_delete(co);
> }
>
> -static void coroutine_swap(Coroutine *from, Coroutine *to)
> +static CoroutineAction coroutine_swap(Coroutine *from, Coroutine *to)
> {
> CoroutineAction ret;
>
> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
>
> - qemu_co_queue_run_restart(to);
> -
> switch (ret) {
> case COROUTINE_YIELD:
> - return;
> + break;
> case COROUTINE_TERMINATE:
> trace_qemu_coroutine_terminate(to);
> + qemu_co_queue_run_restart(to);
> coroutine_delete(to);
> - return;
> + break;
> default:
> abort();
> }
> +
> + return ret;
> }
>
> void qemu_coroutine_enter(Coroutine *co, void *opaque)
> {
> Coroutine *self = qemu_coroutine_self();
> + CoroutineAction ret;
>
> trace_qemu_coroutine_enter(self, co, opaque);
>
> @@ -132,7 +134,9 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
>
> co->caller = self;
> co->entry_arg = opaque;
> - coroutine_swap(self, co);
> + ret = coroutine_swap(self, co);
> + if (ret == COROUTINE_YIELD)
> + qemu_co_queue_run_restart(co);
> }
>
> void coroutine_fn qemu_coroutine_yield(void)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue
2015-02-09 6:50 [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue Bin Wu
2015-02-09 9:09 ` Paolo Bonzini
@ 2015-02-09 9:42 ` Kevin Wolf
2015-02-09 14:48 ` Stefan Hajnoczi
2 siblings, 0 replies; 10+ messages in thread
From: Kevin Wolf @ 2015-02-09 9:42 UTC (permalink / raw)
To: Bin Wu
Cc: famz, boby.chen, subo7, kathy.wangting, rudy.zhangmin,
qemu-devel, arei.gonglei, stefanha, pbonzini, bruce.fon
Am 09.02.2015 um 07:50 hat Bin Wu geschrieben:
> From: Bin Wu <wu.wubin@huawei.com>
>
> We tested VMs migration with their disk images by drive_mirror. With
> migration, two VMs copyed large files between each other. During the
> test, a segfault occured. The stack was as follow:
>
> (gdb) bt
> qemu-coroutine-lock.c:66
> to=0x7fa5a1798648) at qemu-coroutine.c:97
> request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
> block/nbd-client.c:165
> sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
> block/nbd-client.c:262
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
> block/nbd-client.c:296
> nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
> req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
> flags=0) at block.c:3321
> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3447
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3471
> nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
> nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
> req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
> flags=0) at block.c:3321
> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3447
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3471
> coroutine-ucontext.c:121
>
> After analyzing the stack and reviewing the code, we find the
> qemu_co_queue_run_restart should not be put in the coroutine_swap function which
> can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
> qemu_coroutine_enter needs to restart the co_queue.
>
> The error scenario is as follow: coroutine C1 enters C2, C2 yields
> back to C1, then C1 ternimates and the related coroutine memory
> becomes invalid. After a while, the C2 coroutine is entered again.
> At this point, C1 is used as a parameter passed to
> qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
> accesses an invalid memory and a segfault error ocurrs.
Why would C1 be used for the qemu_co_queue_run_restart() call? There is
only one caller, coroutine_swap(). It calls the function for the
coroutine that has just yielded or terminated. In the case of
termination, the coroutine is deleted only afterwards, in the switch
block in coroutine_swap().
> The qemu_co_queue_run_restart function re-enters coroutines waiting
> in the co_queue. However, this function should be only used int the
> qemu_coroutine_enter context.
It shouldn't make a difference. Any coroutine waiting in a CoQueue
should be ready to be restarted at either point.
Kevin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue
2015-02-09 6:50 [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue Bin Wu
2015-02-09 9:09 ` Paolo Bonzini
2015-02-09 9:42 ` Kevin Wolf
@ 2015-02-09 14:48 ` Stefan Hajnoczi
2015-02-10 0:51 ` Bin Wu
` (2 more replies)
2 siblings, 3 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2015-02-09 14:48 UTC (permalink / raw)
To: Bin Wu
Cc: kwolf, famz, stefanha, subo7, kathy.wangting, bruce.fon,
qemu-devel, arei.gonglei, boby.chen, pbonzini, rudy.zhangmin
[-- Attachment #1: Type: text/plain, Size: 7835 bytes --]
On Mon, Feb 09, 2015 at 02:50:39PM +0800, Bin Wu wrote:
> From: Bin Wu <wu.wubin@huawei.com>
>
> We tested VMs migration with their disk images by drive_mirror. With
> migration, two VMs copyed large files between each other. During the
> test, a segfault occured. The stack was as follow:
>
> (gdb) bt
> qemu-coroutine-lock.c:66
> to=0x7fa5a1798648) at qemu-coroutine.c:97
> request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
> block/nbd-client.c:165
> sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
> block/nbd-client.c:262
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
> block/nbd-client.c:296
> nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
> req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
> flags=0) at block.c:3321
> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3447
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3471
> nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
> nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
> req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
> flags=0) at block.c:3321
> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3447
> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> block.c:3471
> coroutine-ucontext.c:121
This backtrace is incomplete. Where are the function names? The
parameter lists appear incomplete too.
> After analyzing the stack and reviewing the code, we find the
> qemu_co_queue_run_restart should not be put in the coroutine_swap function which
> can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
> qemu_coroutine_enter needs to restart the co_queue.
>
> The error scenario is as follow: coroutine C1 enters C2, C2 yields
> back to C1, then C1 ternimates and the related coroutine memory
> becomes invalid. After a while, the C2 coroutine is entered again.
> At this point, C1 is used as a parameter passed to
> qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
> accesses an invalid memory and a segfault error ocurrs.
>
> The qemu_co_queue_run_restart function re-enters coroutines waiting
> in the co_queue. However, this function should be only used int the
> qemu_coroutine_enter context. Only in this context, when the current
> coroutine gets execution control again(after the execution of
> qemu_coroutine_switch), we can restart the target coutine because the
> target coutine has yielded back to the current coroutine or it has
> terminated.
>
> First we want to put qemu_co_queue_run_restart in qemu_coroutine_enter,
> but we find we can not access the target coroutine if it terminates.
This example captures the scenario you describe:
diff --git a/qemu-coroutine.c b/qemu-coroutine.c
index 525247b..883cbf5 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -103,7 +103,10 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
{
CoroutineAction ret;
+ fprintf(stderr, "> %s from %p to %p\n", __func__, from, to);
ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
+ fprintf(stderr, "< %s from %p to %p switch %s\n", __func__, from, to,
+ ret == COROUTINE_YIELD ? "yield" : "terminate");
qemu_co_queue_run_restart(to);
@@ -111,6 +114,7 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
case COROUTINE_YIELD:
return;
case COROUTINE_TERMINATE:
+ fprintf(stderr, "coroutine_delete %p\n", to);
trace_qemu_coroutine_terminate(to);
coroutine_delete(to);
return;
diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
index 27d1b6f..d44c428 100644
--- a/tests/test-coroutine.c
+++ b/tests/test-coroutine.c
@@ -13,6 +13,7 @@
#include <glib.h>
#include "block/coroutine.h"
+#include "block/coroutine_int.h"
/*
* Check that qemu_in_coroutine() works
@@ -122,6 +123,35 @@ static void test_yield(void)
g_assert_cmpint(i, ==, 5); /* coroutine must yield 5 times */
}
+static void coroutine_fn c2_fn(void *opaque)
+{
+ fprintf(stderr, "c2 Part 1\n");
+ qemu_coroutine_yield();
+ fprintf(stderr, "c2 Part 2\n");
+}
+
+static void coroutine_fn c1_fn(void *opaque)
+{
+ Coroutine *c2 = opaque;
+
+ fprintf(stderr, "c1 Part 1\n");
+ qemu_coroutine_enter(c2, NULL);
+ fprintf(stderr, "c1 Part 2\n");
+}
+
+static void test_co_queue(void)
+{
+ Coroutine *c1;
+ Coroutine *c2;
+
+ c1 = qemu_coroutine_create(c1_fn);
+ c2 = qemu_coroutine_create(c2_fn);
+
+ qemu_coroutine_enter(c1, c2);
+ memset(c1, 0xff, sizeof(Coroutine));
+ qemu_coroutine_enter(c2, NULL);
+}
+
/*
* Check that creation, enter, and return work
*/
@@ -343,6 +373,7 @@ static void perf_cost(void)
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
+ g_test_add_func("/basic/co_queue", test_co_queue);
g_test_add_func("/basic/lifecycle", test_lifecycle);
g_test_add_func("/basic/yield", test_yield);
g_test_add_func("/basic/nesting", test_nesting);
Here is the output (with printfs in coroutine_swap):
-> coroutine_swap from MAIN to C1
c1 Part 1
-> coroutine_swap from C1 to C2
c2 Part 1
-> coroutine_swap from C2 to C1
<- coroutine_swap from C1 to C2 switch yield
c1 Part 2
<- coroutine_swap from MAIN to C1 switch terminate
coroutine_delete C1
-> coroutine_swap from MAIN to C2
<- coroutine_swap from C2 to C1 switch yield !!!
c2 Part 2
<- coroutine_swap from MAIN to C2 switch terminate
coroutine_delete C2
I have marked the problematic line with "!!!". The to=C1 variable is
used after C1 has been deleted.
The test crashes since it writes 0xff to C1 after it has terminated.
> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
> ---
> qemu-coroutine.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
> index 525247b..cc0bdfa 100644
> --- a/qemu-coroutine.c
> +++ b/qemu-coroutine.c
> @@ -99,29 +99,31 @@ static void coroutine_delete(Coroutine *co)
> qemu_coroutine_delete(co);
> }
>
> -static void coroutine_swap(Coroutine *from, Coroutine *to)
> +static CoroutineAction coroutine_swap(Coroutine *from, Coroutine *to)
> {
> CoroutineAction ret;
>
> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
>
> - qemu_co_queue_run_restart(to);
> -
> switch (ret) {
> case COROUTINE_YIELD:
> - return;
> + break;
> case COROUTINE_TERMINATE:
> trace_qemu_coroutine_terminate(to);
> + qemu_co_queue_run_restart(to);
> coroutine_delete(to);
> - return;
> + break;
> default:
> abort();
> }
> +
> + return ret;
> }
>
> void qemu_coroutine_enter(Coroutine *co, void *opaque)
> {
> Coroutine *self = qemu_coroutine_self();
> + CoroutineAction ret;
>
> trace_qemu_coroutine_enter(self, co, opaque);
>
> @@ -132,7 +134,9 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
>
> co->caller = self;
> co->entry_arg = opaque;
> - coroutine_swap(self, co);
> + ret = coroutine_swap(self, co);
> + if (ret == COROUTINE_YIELD)
> + qemu_co_queue_run_restart(co);
> }
Your fix looks correct although QEMU coding style requires {}.
I tried to think of a simpler solution that keeps a single
qemu_co_queue_run_restart() call but was unable to find one.
Please send another revision with a test-coroutine.c test case so we can
protect against regressions.
Thanks,
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue
2015-02-09 14:48 ` Stefan Hajnoczi
@ 2015-02-10 0:51 ` Bin Wu
2015-02-10 3:16 ` Wen Congyang
2015-02-10 10:13 ` Kevin Wolf
2 siblings, 0 replies; 10+ messages in thread
From: Bin Wu @ 2015-02-10 0:51 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, famz, stefanha, subo7, kathy.wangting, bruce.fon,
qemu-devel, arei.gonglei, boby.chen, pbonzini, rudy.zhangmin
On 2015/2/9 22:48, Stefan Hajnoczi wrote:
> On Mon, Feb 09, 2015 at 02:50:39PM +0800, Bin Wu wrote:
>> From: Bin Wu <wu.wubin@huawei.com>
>>
>> We tested VMs migration with their disk images by drive_mirror. With
>> migration, two VMs copyed large files between each other. During the
>> test, a segfault occured. The stack was as follow:
>>
>> (gdb) bt
>> qemu-coroutine-lock.c:66
>> to=0x7fa5a1798648) at qemu-coroutine.c:97
>> request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
>> block/nbd-client.c:165
>> sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
>> block/nbd-client.c:262
>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
>> block/nbd-client.c:296
>> nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
>> req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
>> flags=0) at block.c:3321
>> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3447
>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3471
>> nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
>> nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
>> req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
>> flags=0) at block.c:3321
>> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3447
>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3471
>> coroutine-ucontext.c:121
>
> This backtrace is incomplete. Where are the function names? The
> parameter lists appear incomplete too.
>
I put the stack in the git log, so some lines are missed:
(gdb) bt
#0 0x00007fa5a0c63fc5 in qemu_co_queue_run_restart (co=0x7fa5a1798648) at
qemu-coroutine-lock.c:66
#1 0x00007fa5a0c63bed in coroutine_swap (from=0x7fa5a178f160,
to=0x7fa5a1798648) at qemu-coroutine.c:97
#2 0x00007fa5a0c63dbf in qemu_coroutine_yield () at qemu-coroutine.c:140
#3 0x00007fa5a0c9e474 in nbd_co_receive_reply (s=0x7fa5a1a3cfd0,
request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
block/nbd-client.c:165
#4 0x00007fa5a0c9e8b5 in nbd_co_writev_1 (client=0x7fa5a1a3cfd0,
sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
block/nbd-client.c:262
#5 0x00007fa5a0c9e9dd in nbd_client_session_co_writev (client=0x7fa5a1a3cfd0,
sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
block/nbd-client.c:296
#6 0x00007fa5a0c9dda1 in nbd_co_writev (bs=0x7fa5a198fcb0, sector_num=8552704,
nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
#7 0x00007fa5a0c509a4 in bdrv_aligned_pwritev (bs=0x7fa5a198fcb0,
req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
flags=0) at block.c:3321
#8 0x00007fa5a0c50f3f in bdrv_co_do_pwritev (bs=0x7fa5a198fcb0,
offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
block.c:3447
#9 0x00007fa5a0c51007 in bdrv_co_do_writev (bs=0x7fa5a198fcb0,
sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
block.c:3471
#10 0x00007fa5a0c51074 in bdrv_co_writev (bs=0x7fa5a198fcb0, sector_num=8552704,
nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
#11 0x00007fa5a0c652ec in raw_co_writev (bs=0x7fa5a198c110, sector_num=8552704,
nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
#12 0x00007fa5a0c509a4 in bdrv_aligned_pwritev (bs=0x7fa5a198c110,
req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
flags=0) at block.c:3321
#13 0x00007fa5a0c50f3f in bdrv_co_do_pwritev (bs=0x7fa5a198c110,
offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
block.c:3447
#14 0x00007fa5a0c51007 in bdrv_co_do_writev (bs=0x7fa5a198c110,
sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
block.c:3471
#15 0x00007fa5a0c542b3 in bdrv_co_do_rw (opaque=0x7fa5a17a0000) at block.c:4706
#16 0x00007fa5a0c64e6e in coroutine_trampoline (i0=-1585909408, i1=32677) at
coroutine-ucontext.c:121
#17 0x00007fa59dc5aa50 in __correctly_grouped_prefixwc () from /lib64/libc.so.6
#18 0x0000000000000000 in ?? ()
>> After analyzing the stack and reviewing the code, we find the
>> qemu_co_queue_run_restart should not be put in the coroutine_swap function which
>> can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
>> qemu_coroutine_enter needs to restart the co_queue.
>>
>> The error scenario is as follow: coroutine C1 enters C2, C2 yields
>> back to C1, then C1 ternimates and the related coroutine memory
>> becomes invalid. After a while, the C2 coroutine is entered again.
>> At this point, C1 is used as a parameter passed to
>> qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
>> accesses an invalid memory and a segfault error ocurrs.
>>
>> The qemu_co_queue_run_restart function re-enters coroutines waiting
>> in the co_queue. However, this function should be only used int the
>> qemu_coroutine_enter context. Only in this context, when the current
>> coroutine gets execution control again(after the execution of
>> qemu_coroutine_switch), we can restart the target coutine because the
>> target coutine has yielded back to the current coroutine or it has
>> terminated.
>>
>> First we want to put qemu_co_queue_run_restart in qemu_coroutine_enter,
>> but we find we can not access the target coroutine if it terminates.
>
> This example captures the scenario you describe:
>
> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
> index 525247b..883cbf5 100644
> --- a/qemu-coroutine.c
> +++ b/qemu-coroutine.c
> @@ -103,7 +103,10 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
> {
> CoroutineAction ret;
>
> + fprintf(stderr, "> %s from %p to %p\n", __func__, from, to);
> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
> + fprintf(stderr, "< %s from %p to %p switch %s\n", __func__, from, to,
> + ret == COROUTINE_YIELD ? "yield" : "terminate");
>
> qemu_co_queue_run_restart(to);
>
> @@ -111,6 +114,7 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
> case COROUTINE_YIELD:
> return;
> case COROUTINE_TERMINATE:
> + fprintf(stderr, "coroutine_delete %p\n", to);
> trace_qemu_coroutine_terminate(to);
> coroutine_delete(to);
> return;
> diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
> index 27d1b6f..d44c428 100644
> --- a/tests/test-coroutine.c
> +++ b/tests/test-coroutine.c
> @@ -13,6 +13,7 @@
>
> #include <glib.h>
> #include "block/coroutine.h"
> +#include "block/coroutine_int.h"
>
> /*
> * Check that qemu_in_coroutine() works
> @@ -122,6 +123,35 @@ static void test_yield(void)
> g_assert_cmpint(i, ==, 5); /* coroutine must yield 5 times */
> }
>
> +static void coroutine_fn c2_fn(void *opaque)
> +{
> + fprintf(stderr, "c2 Part 1\n");
> + qemu_coroutine_yield();
> + fprintf(stderr, "c2 Part 2\n");
> +}
> +
> +static void coroutine_fn c1_fn(void *opaque)
> +{
> + Coroutine *c2 = opaque;
> +
> + fprintf(stderr, "c1 Part 1\n");
> + qemu_coroutine_enter(c2, NULL);
> + fprintf(stderr, "c1 Part 2\n");
> +}
> +
> +static void test_co_queue(void)
> +{
> + Coroutine *c1;
> + Coroutine *c2;
> +
> + c1 = qemu_coroutine_create(c1_fn);
> + c2 = qemu_coroutine_create(c2_fn);
> +
> + qemu_coroutine_enter(c1, c2);
> + memset(c1, 0xff, sizeof(Coroutine));
> + qemu_coroutine_enter(c2, NULL);
> +}
> +
> /*
> * Check that creation, enter, and return work
> */
> @@ -343,6 +373,7 @@ static void perf_cost(void)
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
> + g_test_add_func("/basic/co_queue", test_co_queue);
> g_test_add_func("/basic/lifecycle", test_lifecycle);
> g_test_add_func("/basic/yield", test_yield);
> g_test_add_func("/basic/nesting", test_nesting);
>
> Here is the output (with printfs in coroutine_swap):
>
> -> coroutine_swap from MAIN to C1
> c1 Part 1
> -> coroutine_swap from C1 to C2
> c2 Part 1
> -> coroutine_swap from C2 to C1
> <- coroutine_swap from C1 to C2 switch yield
> c1 Part 2
> <- coroutine_swap from MAIN to C1 switch terminate
> coroutine_delete C1
> -> coroutine_swap from MAIN to C2
> <- coroutine_swap from C2 to C1 switch yield !!!
> c2 Part 2
> <- coroutine_swap from MAIN to C2 switch terminate
> coroutine_delete C2
>
> I have marked the problematic line with "!!!". The to=C1 variable is
> used after C1 has been deleted.
>
> The test crashes since it writes 0xff to C1 after it has terminated.
yes, this is exactly the error scenario I described. Thanks, stefan.
>
>> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
>> ---
>> qemu-coroutine.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
>> index 525247b..cc0bdfa 100644
>> --- a/qemu-coroutine.c
>> +++ b/qemu-coroutine.c
>> @@ -99,29 +99,31 @@ static void coroutine_delete(Coroutine *co)
>> qemu_coroutine_delete(co);
>> }
>>
>> -static void coroutine_swap(Coroutine *from, Coroutine *to)
>> +static CoroutineAction coroutine_swap(Coroutine *from, Coroutine *to)
>> {
>> CoroutineAction ret;
>>
>> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
>>
>> - qemu_co_queue_run_restart(to);
>> -
>> switch (ret) {
>> case COROUTINE_YIELD:
>> - return;
>> + break;
>> case COROUTINE_TERMINATE:
>> trace_qemu_coroutine_terminate(to);
>> + qemu_co_queue_run_restart(to);
>> coroutine_delete(to);
>> - return;
>> + break;
>> default:
>> abort();
>> }
>> +
>> + return ret;
>> }
>>
>> void qemu_coroutine_enter(Coroutine *co, void *opaque)
>> {
>> Coroutine *self = qemu_coroutine_self();
>> + CoroutineAction ret;
>>
>> trace_qemu_coroutine_enter(self, co, opaque);
>>
>> @@ -132,7 +134,9 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
>>
>> co->caller = self;
>> co->entry_arg = opaque;
>> - coroutine_swap(self, co);
>> + ret = coroutine_swap(self, co);
>> + if (ret == COROUTINE_YIELD)
>> + qemu_co_queue_run_restart(co);
>> }
>
> Your fix looks correct although QEMU coding style requires {}.
>
> I tried to think of a simpler solution that keeps a single
> qemu_co_queue_run_restart() call but was unable to find one.
>
> Please send another revision with a test-coroutine.c test case so we can
> protect against regressions.
OK, I will send another version latter.
>
> Thanks,
> Stefan
>
--
Bin Wu
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue
2015-02-09 9:09 ` Paolo Bonzini
@ 2015-02-10 0:55 ` Bin Wu
0 siblings, 0 replies; 10+ messages in thread
From: Bin Wu @ 2015-02-10 0:55 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: kwolf, famz, boby.chen, subo7, kathy.wangting, rudy.zhangmin,
arei.gonglei, stefanha, bruce.fon
On 2015/2/9 17:09, Paolo Bonzini wrote:
>
>
> On 09/02/2015 07:50, Bin Wu wrote:
>> From: Bin Wu <wu.wubin@huawei.com>
>>
>> We tested VMs migration with their disk images by drive_mirror. With
>> migration, two VMs copyed large files between each other. During the
>> test, a segfault occured. The stack was as follow:
>>
>> (gdb) bt
>> qemu-coroutine-lock.c:66
>> to=0x7fa5a1798648) at qemu-coroutine.c:97
>> request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
>> block/nbd-client.c:165
>> sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
>> block/nbd-client.c:262
>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
>> block/nbd-client.c:296
>> nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
>> req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
>> flags=0) at block.c:3321
>> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3447
>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3471
>> nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
>> nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
>> req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
>> flags=0) at block.c:3321
>> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3447
>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3471
>> coroutine-ucontext.c:121
>>
>> After analyzing the stack and reviewing the code, we find the
>> qemu_co_queue_run_restart should not be put in the coroutine_swap function which
>> can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
>> qemu_coroutine_enter needs to restart the co_queue.
>>
>> The error scenario is as follow: coroutine C1 enters C2, C2 yields
>> back to C1, then C1 ternimates and the related coroutine memory
>> becomes invalid. After a while, the C2 coroutine is entered again.
>> At this point, C1 is used as a parameter passed to
>> qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
>> accesses an invalid memory and a segfault error ocurrs.
>>
>> The qemu_co_queue_run_restart function re-enters coroutines waiting
>> in the co_queue. However, this function should be only used int the
>> qemu_coroutine_enter context. Only in this context, when the current
>> coroutine gets execution control again(after the execution of
>> qemu_coroutine_switch), we can restart the target coutine because the
>> target coutine has yielded back to the current coroutine or it has
>> terminated.
>
> qemu_coroutine_yield can be executed for other reasons than locks. In
> those cases, it is correct to call qemu_co_queue_run_restart. I think
> it's an NBD bug.
>
> Paolo
>
Maybe I didn't describe the error scenario clearly, but it's a normal coroutine
using case, not a NBD bug. Please reference stefan's reply, thanks.
>> First we want to put qemu_co_queue_run_restart in qemu_coroutine_enter,
>> but we find we can not access the target coroutine if it terminates.
>>
>> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
>> ---
>> qemu-coroutine.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
>> index 525247b..cc0bdfa 100644
>> --- a/qemu-coroutine.c
>> +++ b/qemu-coroutine.c
>> @@ -99,29 +99,31 @@ static void coroutine_delete(Coroutine *co)
>> qemu_coroutine_delete(co);
>> }
>>
>> -static void coroutine_swap(Coroutine *from, Coroutine *to)
>> +static CoroutineAction coroutine_swap(Coroutine *from, Coroutine *to)
>> {
>> CoroutineAction ret;
>>
>> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
>>
>> - qemu_co_queue_run_restart(to);
>> -
>> switch (ret) {
>> case COROUTINE_YIELD:
>> - return;
>> + break;
>> case COROUTINE_TERMINATE:
>> trace_qemu_coroutine_terminate(to);
>> + qemu_co_queue_run_restart(to);
>> coroutine_delete(to);
>> - return;
>> + break;
>> default:
>> abort();
>> }
>> +
>> + return ret;
>> }
>>
>> void qemu_coroutine_enter(Coroutine *co, void *opaque)
>> {
>> Coroutine *self = qemu_coroutine_self();
>> + CoroutineAction ret;
>>
>> trace_qemu_coroutine_enter(self, co, opaque);
>>
>> @@ -132,7 +134,9 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
>>
>> co->caller = self;
>> co->entry_arg = opaque;
>> - coroutine_swap(self, co);
>> + ret = coroutine_swap(self, co);
>> + if (ret == COROUTINE_YIELD)
>> + qemu_co_queue_run_restart(co);
>> }
>>
>> void coroutine_fn qemu_coroutine_yield(void)
>>
>
>
> .
>
--
Bin Wu
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue
2015-02-09 14:48 ` Stefan Hajnoczi
2015-02-10 0:51 ` Bin Wu
@ 2015-02-10 3:16 ` Wen Congyang
2015-02-10 3:48 ` Bin Wu
2015-02-10 10:13 ` Kevin Wolf
2 siblings, 1 reply; 10+ messages in thread
From: Wen Congyang @ 2015-02-10 3:16 UTC (permalink / raw)
To: Stefan Hajnoczi, Bin Wu
Cc: kwolf, famz, boby.chen, subo7, kathy.wangting, rudy.zhangmin,
qemu-devel, arei.gonglei, stefanha, pbonzini, bruce.fon
On 02/09/2015 10:48 PM, Stefan Hajnoczi wrote:
> On Mon, Feb 09, 2015 at 02:50:39PM +0800, Bin Wu wrote:
>> From: Bin Wu <wu.wubin@huawei.com>
>>
>> We tested VMs migration with their disk images by drive_mirror. With
>> migration, two VMs copyed large files between each other. During the
>> test, a segfault occured. The stack was as follow:
>>
>> (gdb) bt
>> qemu-coroutine-lock.c:66
>> to=0x7fa5a1798648) at qemu-coroutine.c:97
>> request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
>> block/nbd-client.c:165
>> sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
>> block/nbd-client.c:262
>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
>> block/nbd-client.c:296
>> nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
>> req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
>> flags=0) at block.c:3321
>> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3447
>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3471
>> nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
>> nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
>> req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
>> flags=0) at block.c:3321
>> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3447
>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>> block.c:3471
>> coroutine-ucontext.c:121
>
> This backtrace is incomplete. Where are the function names? The
> parameter lists appear incomplete too.
>
>> After analyzing the stack and reviewing the code, we find the
>> qemu_co_queue_run_restart should not be put in the coroutine_swap function which
>> can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
>> qemu_coroutine_enter needs to restart the co_queue.
>>
>> The error scenario is as follow: coroutine C1 enters C2, C2 yields
>> back to C1, then C1 ternimates and the related coroutine memory
>> becomes invalid. After a while, the C2 coroutine is entered again.
>> At this point, C1 is used as a parameter passed to
>> qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
>> accesses an invalid memory and a segfault error ocurrs.
>>
>> The qemu_co_queue_run_restart function re-enters coroutines waiting
>> in the co_queue. However, this function should be only used int the
>> qemu_coroutine_enter context. Only in this context, when the current
>> coroutine gets execution control again(after the execution of
>> qemu_coroutine_switch), we can restart the target coutine because the
>> target coutine has yielded back to the current coroutine or it has
>> terminated.
>>
>> First we want to put qemu_co_queue_run_restart in qemu_coroutine_enter,
>> but we find we can not access the target coroutine if it terminates.
>
> This example captures the scenario you describe:
>
> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
> index 525247b..883cbf5 100644
> --- a/qemu-coroutine.c
> +++ b/qemu-coroutine.c
> @@ -103,7 +103,10 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
> {
> CoroutineAction ret;
>
> + fprintf(stderr, "> %s from %p to %p\n", __func__, from, to);
> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
> + fprintf(stderr, "< %s from %p to %p switch %s\n", __func__, from, to,
> + ret == COROUTINE_YIELD ? "yield" : "terminate");
>
> qemu_co_queue_run_restart(to);
>
> @@ -111,6 +114,7 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
> case COROUTINE_YIELD:
> return;
> case COROUTINE_TERMINATE:
> + fprintf(stderr, "coroutine_delete %p\n", to);
> trace_qemu_coroutine_terminate(to);
> coroutine_delete(to);
> return;
> diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
> index 27d1b6f..d44c428 100644
> --- a/tests/test-coroutine.c
> +++ b/tests/test-coroutine.c
> @@ -13,6 +13,7 @@
>
> #include <glib.h>
> #include "block/coroutine.h"
> +#include "block/coroutine_int.h"
>
> /*
> * Check that qemu_in_coroutine() works
> @@ -122,6 +123,35 @@ static void test_yield(void)
> g_assert_cmpint(i, ==, 5); /* coroutine must yield 5 times */
> }
>
> +static void coroutine_fn c2_fn(void *opaque)
> +{
> + fprintf(stderr, "c2 Part 1\n");
> + qemu_coroutine_yield();
> + fprintf(stderr, "c2 Part 2\n");
> +}
> +
> +static void coroutine_fn c1_fn(void *opaque)
> +{
> + Coroutine *c2 = opaque;
> +
> + fprintf(stderr, "c1 Part 1\n");
> + qemu_coroutine_enter(c2, NULL);
> + fprintf(stderr, "c1 Part 2\n");
> +}
> +
> +static void test_co_queue(void)
> +{
> + Coroutine *c1;
> + Coroutine *c2;
> +
> + c1 = qemu_coroutine_create(c1_fn);
> + c2 = qemu_coroutine_create(c2_fn);
> +
> + qemu_coroutine_enter(c1, c2);
> + memset(c1, 0xff, sizeof(Coroutine));
> + qemu_coroutine_enter(c2, NULL);
> +}
> +
> /*
> * Check that creation, enter, and return work
> */
> @@ -343,6 +373,7 @@ static void perf_cost(void)
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
> + g_test_add_func("/basic/co_queue", test_co_queue);
> g_test_add_func("/basic/lifecycle", test_lifecycle);
> g_test_add_func("/basic/yield", test_yield);
> g_test_add_func("/basic/nesting", test_nesting);
>
> Here is the output (with printfs in coroutine_swap):
>
> -> coroutine_swap from MAIN to C1
> c1 Part 1
> -> coroutine_swap from C1 to C2
> c2 Part 1
> -> coroutine_swap from C2 to C1
> <- coroutine_swap from C1 to C2 switch yield
> c1 Part 2
> <- coroutine_swap from MAIN to C1 switch terminate
> coroutine_delete C1
> -> coroutine_swap from MAIN to C2
> <- coroutine_swap from C2 to C1 switch yield !!!
> c2 Part 2
> <- coroutine_swap from MAIN to C2 switch terminate
> coroutine_delete C2
>
> I have marked the problematic line with "!!!". The to=C1 variable is
> used after C1 has been deleted.
How to build the test program? make test? I meet the following errors:
make test
make -C tests/tcg test
make[1]: Entering directory '/work/src/qemu/tests/tcg'
CC test_path.o
In file included from /work/src/qemu/include/qemu-common.h:43:0,
from /work/src/qemu/util/cutils.c:24,
from test_path.c:3:
/work/src/qemu/include/glib-compat.h:19:18: fatal error: glib.h: No such file or directory
#include <glib.h>
^
compilation terminated.
/work/src/qemu/rules.mak:57: recipe for target 'test_path.o' failed
make[1]: *** [test_path.o] Error 1
make[1]: Leaving directory '/work/src/qemu/tests/tcg'
Makefile:428: recipe for target 'test' failed
make: *** [test] Error 2
Do I miss something?
Thanks
Wen Congyang
>
> The test crashes since it writes 0xff to C1 after it has terminated.
>
>> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
>> ---
>> qemu-coroutine.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
>> index 525247b..cc0bdfa 100644
>> --- a/qemu-coroutine.c
>> +++ b/qemu-coroutine.c
>> @@ -99,29 +99,31 @@ static void coroutine_delete(Coroutine *co)
>> qemu_coroutine_delete(co);
>> }
>>
>> -static void coroutine_swap(Coroutine *from, Coroutine *to)
>> +static CoroutineAction coroutine_swap(Coroutine *from, Coroutine *to)
>> {
>> CoroutineAction ret;
>>
>> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
>>
>> - qemu_co_queue_run_restart(to);
>> -
>> switch (ret) {
>> case COROUTINE_YIELD:
>> - return;
>> + break;
>> case COROUTINE_TERMINATE:
>> trace_qemu_coroutine_terminate(to);
>> + qemu_co_queue_run_restart(to);
>> coroutine_delete(to);
>> - return;
>> + break;
>> default:
>> abort();
>> }
>> +
>> + return ret;
>> }
>>
>> void qemu_coroutine_enter(Coroutine *co, void *opaque)
>> {
>> Coroutine *self = qemu_coroutine_self();
>> + CoroutineAction ret;
>>
>> trace_qemu_coroutine_enter(self, co, opaque);
>>
>> @@ -132,7 +134,9 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
>>
>> co->caller = self;
>> co->entry_arg = opaque;
>> - coroutine_swap(self, co);
>> + ret = coroutine_swap(self, co);
>> + if (ret == COROUTINE_YIELD)
>> + qemu_co_queue_run_restart(co);
>> }
>
> Your fix looks correct although QEMU coding style requires {}.
>
> I tried to think of a simpler solution that keeps a single
> qemu_co_queue_run_restart() call but was unable to find one.
>
> Please send another revision with a test-coroutine.c test case so we can
> protect against regressions.
>
> Thanks,
> Stefan
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue
2015-02-10 3:16 ` Wen Congyang
@ 2015-02-10 3:48 ` Bin Wu
2015-02-10 4:49 ` Wen Congyang
0 siblings, 1 reply; 10+ messages in thread
From: Bin Wu @ 2015-02-10 3:48 UTC (permalink / raw)
To: Wen Congyang, Stefan Hajnoczi
Cc: kwolf, famz, boby.chen, subo7, kathy.wangting, rudy.zhangmin,
qemu-devel, arei.gonglei, stefanha, pbonzini, bruce.fon
On 2015/2/10 11:16, Wen Congyang wrote:
> On 02/09/2015 10:48 PM, Stefan Hajnoczi wrote:
>> On Mon, Feb 09, 2015 at 02:50:39PM +0800, Bin Wu wrote:
>>> From: Bin Wu <wu.wubin@huawei.com>
>>>
>>> We tested VMs migration with their disk images by drive_mirror. With
>>> migration, two VMs copyed large files between each other. During the
>>> test, a segfault occured. The stack was as follow:
>>>
>>> (gdb) bt
>>> qemu-coroutine-lock.c:66
>>> to=0x7fa5a1798648) at qemu-coroutine.c:97
>>> request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
>>> block/nbd-client.c:165
>>> sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
>>> block/nbd-client.c:262
>>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
>>> block/nbd-client.c:296
>>> nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
>>> req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
>>> flags=0) at block.c:3321
>>> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>>> block.c:3447
>>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>>> block.c:3471
>>> nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
>>> nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
>>> req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
>>> flags=0) at block.c:3321
>>> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>>> block.c:3447
>>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>>> block.c:3471
>>> coroutine-ucontext.c:121
>>
>> This backtrace is incomplete. Where are the function names? The
>> parameter lists appear incomplete too.
>>
>>> After analyzing the stack and reviewing the code, we find the
>>> qemu_co_queue_run_restart should not be put in the coroutine_swap function which
>>> can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
>>> qemu_coroutine_enter needs to restart the co_queue.
>>>
>>> The error scenario is as follow: coroutine C1 enters C2, C2 yields
>>> back to C1, then C1 ternimates and the related coroutine memory
>>> becomes invalid. After a while, the C2 coroutine is entered again.
>>> At this point, C1 is used as a parameter passed to
>>> qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
>>> accesses an invalid memory and a segfault error ocurrs.
>>>
>>> The qemu_co_queue_run_restart function re-enters coroutines waiting
>>> in the co_queue. However, this function should be only used int the
>>> qemu_coroutine_enter context. Only in this context, when the current
>>> coroutine gets execution control again(after the execution of
>>> qemu_coroutine_switch), we can restart the target coutine because the
>>> target coutine has yielded back to the current coroutine or it has
>>> terminated.
>>>
>>> First we want to put qemu_co_queue_run_restart in qemu_coroutine_enter,
>>> but we find we can not access the target coroutine if it terminates.
>>
>> This example captures the scenario you describe:
>>
>> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
>> index 525247b..883cbf5 100644
>> --- a/qemu-coroutine.c
>> +++ b/qemu-coroutine.c
>> @@ -103,7 +103,10 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
>> {
>> CoroutineAction ret;
>>
>> + fprintf(stderr, "> %s from %p to %p\n", __func__, from, to);
>> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
>> + fprintf(stderr, "< %s from %p to %p switch %s\n", __func__, from, to,
>> + ret == COROUTINE_YIELD ? "yield" : "terminate");
>>
>> qemu_co_queue_run_restart(to);
>>
>> @@ -111,6 +114,7 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
>> case COROUTINE_YIELD:
>> return;
>> case COROUTINE_TERMINATE:
>> + fprintf(stderr, "coroutine_delete %p\n", to);
>> trace_qemu_coroutine_terminate(to);
>> coroutine_delete(to);
>> return;
>> diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
>> index 27d1b6f..d44c428 100644
>> --- a/tests/test-coroutine.c
>> +++ b/tests/test-coroutine.c
>> @@ -13,6 +13,7 @@
>>
>> #include <glib.h>
>> #include "block/coroutine.h"
>> +#include "block/coroutine_int.h"
>>
>> /*
>> * Check that qemu_in_coroutine() works
>> @@ -122,6 +123,35 @@ static void test_yield(void)
>> g_assert_cmpint(i, ==, 5); /* coroutine must yield 5 times */
>> }
>>
>> +static void coroutine_fn c2_fn(void *opaque)
>> +{
>> + fprintf(stderr, "c2 Part 1\n");
>> + qemu_coroutine_yield();
>> + fprintf(stderr, "c2 Part 2\n");
>> +}
>> +
>> +static void coroutine_fn c1_fn(void *opaque)
>> +{
>> + Coroutine *c2 = opaque;
>> +
>> + fprintf(stderr, "c1 Part 1\n");
>> + qemu_coroutine_enter(c2, NULL);
>> + fprintf(stderr, "c1 Part 2\n");
>> +}
>> +
>> +static void test_co_queue(void)
>> +{
>> + Coroutine *c1;
>> + Coroutine *c2;
>> +
>> + c1 = qemu_coroutine_create(c1_fn);
>> + c2 = qemu_coroutine_create(c2_fn);
>> +
>> + qemu_coroutine_enter(c1, c2);
>> + memset(c1, 0xff, sizeof(Coroutine));
>> + qemu_coroutine_enter(c2, NULL);
>> +}
>> +
>> /*
>> * Check that creation, enter, and return work
>> */
>> @@ -343,6 +373,7 @@ static void perf_cost(void)
>> int main(int argc, char **argv)
>> {
>> g_test_init(&argc, &argv, NULL);
>> + g_test_add_func("/basic/co_queue", test_co_queue);
>> g_test_add_func("/basic/lifecycle", test_lifecycle);
>> g_test_add_func("/basic/yield", test_yield);
>> g_test_add_func("/basic/nesting", test_nesting);
>>
>> Here is the output (with printfs in coroutine_swap):
>>
>> -> coroutine_swap from MAIN to C1
>> c1 Part 1
>> -> coroutine_swap from C1 to C2
>> c2 Part 1
>> -> coroutine_swap from C2 to C1
>> <- coroutine_swap from C1 to C2 switch yield
>> c1 Part 2
>> <- coroutine_swap from MAIN to C1 switch terminate
>> coroutine_delete C1
>> -> coroutine_swap from MAIN to C2
>> <- coroutine_swap from C2 to C1 switch yield !!!
>> c2 Part 2
>> <- coroutine_swap from MAIN to C2 switch terminate
>> coroutine_delete C2
>>
>> I have marked the problematic line with "!!!". The to=C1 variable is
>> used after C1 has been deleted.
>
> How to build the test program? make test? I meet the following errors:
> make test
> make -C tests/tcg test
> make[1]: Entering directory '/work/src/qemu/tests/tcg'
> CC test_path.o
> In file included from /work/src/qemu/include/qemu-common.h:43:0,
> from /work/src/qemu/util/cutils.c:24,
> from test_path.c:3:
> /work/src/qemu/include/glib-compat.h:19:18: fatal error: glib.h: No such file or directory
> #include <glib.h>
> ^
> compilation terminated.
> /work/src/qemu/rules.mak:57: recipe for target 'test_path.o' failed
> make[1]: *** [test_path.o] Error 1
> make[1]: Leaving directory '/work/src/qemu/tests/tcg'
> Makefile:428: recipe for target 'test' failed
> make: *** [test] Error 2
>
> Do I miss something?
>
> Thanks
> Wen Congyang
>
First, you need to install glib2-devel package which includes the glib.h. Then
you can use "make check" in the source tree to compile and run all tests (or use
"make check-help" to get some helps).
>>
>> The test crashes since it writes 0xff to C1 after it has terminated.
>>
>>> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
>>> ---
>>> qemu-coroutine.c | 16 ++++++++++------
>>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
>>> index 525247b..cc0bdfa 100644
>>> --- a/qemu-coroutine.c
>>> +++ b/qemu-coroutine.c
>>> @@ -99,29 +99,31 @@ static void coroutine_delete(Coroutine *co)
>>> qemu_coroutine_delete(co);
>>> }
>>>
>>> -static void coroutine_swap(Coroutine *from, Coroutine *to)
>>> +static CoroutineAction coroutine_swap(Coroutine *from, Coroutine *to)
>>> {
>>> CoroutineAction ret;
>>>
>>> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
>>>
>>> - qemu_co_queue_run_restart(to);
>>> -
>>> switch (ret) {
>>> case COROUTINE_YIELD:
>>> - return;
>>> + break;
>>> case COROUTINE_TERMINATE:
>>> trace_qemu_coroutine_terminate(to);
>>> + qemu_co_queue_run_restart(to);
>>> coroutine_delete(to);
>>> - return;
>>> + break;
>>> default:
>>> abort();
>>> }
>>> +
>>> + return ret;
>>> }
>>>
>>> void qemu_coroutine_enter(Coroutine *co, void *opaque)
>>> {
>>> Coroutine *self = qemu_coroutine_self();
>>> + CoroutineAction ret;
>>>
>>> trace_qemu_coroutine_enter(self, co, opaque);
>>>
>>> @@ -132,7 +134,9 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
>>>
>>> co->caller = self;
>>> co->entry_arg = opaque;
>>> - coroutine_swap(self, co);
>>> + ret = coroutine_swap(self, co);
>>> + if (ret == COROUTINE_YIELD)
>>> + qemu_co_queue_run_restart(co);
>>> }
>>
>> Your fix looks correct although QEMU coding style requires {}.
>>
>> I tried to think of a simpler solution that keeps a single
>> qemu_co_queue_run_restart() call but was unable to find one.
>>
>> Please send another revision with a test-coroutine.c test case so we can
>> protect against regressions.
>>
>> Thanks,
>> Stefan
>>
>
>
> .
>
--
Bin Wu
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue
2015-02-10 3:48 ` Bin Wu
@ 2015-02-10 4:49 ` Wen Congyang
0 siblings, 0 replies; 10+ messages in thread
From: Wen Congyang @ 2015-02-10 4:49 UTC (permalink / raw)
To: Bin Wu, Stefan Hajnoczi
Cc: kwolf, famz, boby.chen, subo7, kathy.wangting, rudy.zhangmin,
qemu-devel, arei.gonglei, stefanha, pbonzini, bruce.fon
On 02/10/2015 11:48 AM, Bin Wu wrote:
> On 2015/2/10 11:16, Wen Congyang wrote:
>> On 02/09/2015 10:48 PM, Stefan Hajnoczi wrote:
>>> On Mon, Feb 09, 2015 at 02:50:39PM +0800, Bin Wu wrote:
>>>> From: Bin Wu <wu.wubin@huawei.com>
>>>>
>>>> We tested VMs migration with their disk images by drive_mirror. With
>>>> migration, two VMs copyed large files between each other. During the
>>>> test, a segfault occured. The stack was as follow:
>>>>
>>>> (gdb) bt
>>>> qemu-coroutine-lock.c:66
>>>> to=0x7fa5a1798648) at qemu-coroutine.c:97
>>>> request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
>>>> block/nbd-client.c:165
>>>> sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
>>>> block/nbd-client.c:262
>>>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
>>>> block/nbd-client.c:296
>>>> nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
>>>> req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
>>>> flags=0) at block.c:3321
>>>> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>>>> block.c:3447
>>>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>>>> block.c:3471
>>>> nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
>>>> nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
>>>> req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
>>>> flags=0) at block.c:3321
>>>> offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>>>> block.c:3447
>>>> sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
>>>> block.c:3471
>>>> coroutine-ucontext.c:121
>>>
>>> This backtrace is incomplete. Where are the function names? The
>>> parameter lists appear incomplete too.
>>>
>>>> After analyzing the stack and reviewing the code, we find the
>>>> qemu_co_queue_run_restart should not be put in the coroutine_swap function which
>>>> can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
>>>> qemu_coroutine_enter needs to restart the co_queue.
>>>>
>>>> The error scenario is as follow: coroutine C1 enters C2, C2 yields
>>>> back to C1, then C1 ternimates and the related coroutine memory
>>>> becomes invalid. After a while, the C2 coroutine is entered again.
>>>> At this point, C1 is used as a parameter passed to
>>>> qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
>>>> accesses an invalid memory and a segfault error ocurrs.
>>>>
>>>> The qemu_co_queue_run_restart function re-enters coroutines waiting
>>>> in the co_queue. However, this function should be only used int the
>>>> qemu_coroutine_enter context. Only in this context, when the current
>>>> coroutine gets execution control again(after the execution of
>>>> qemu_coroutine_switch), we can restart the target coutine because the
>>>> target coutine has yielded back to the current coroutine or it has
>>>> terminated.
>>>>
>>>> First we want to put qemu_co_queue_run_restart in qemu_coroutine_enter,
>>>> but we find we can not access the target coroutine if it terminates.
>>>
>>> This example captures the scenario you describe:
>>>
>>> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
>>> index 525247b..883cbf5 100644
>>> --- a/qemu-coroutine.c
>>> +++ b/qemu-coroutine.c
>>> @@ -103,7 +103,10 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
>>> {
>>> CoroutineAction ret;
>>>
>>> + fprintf(stderr, "> %s from %p to %p\n", __func__, from, to);
>>> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
>>> + fprintf(stderr, "< %s from %p to %p switch %s\n", __func__, from, to,
>>> + ret == COROUTINE_YIELD ? "yield" : "terminate");
>>>
>>> qemu_co_queue_run_restart(to);
>>>
>>> @@ -111,6 +114,7 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
>>> case COROUTINE_YIELD:
>>> return;
>>> case COROUTINE_TERMINATE:
>>> + fprintf(stderr, "coroutine_delete %p\n", to);
>>> trace_qemu_coroutine_terminate(to);
>>> coroutine_delete(to);
>>> return;
>>> diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
>>> index 27d1b6f..d44c428 100644
>>> --- a/tests/test-coroutine.c
>>> +++ b/tests/test-coroutine.c
>>> @@ -13,6 +13,7 @@
>>>
>>> #include <glib.h>
>>> #include "block/coroutine.h"
>>> +#include "block/coroutine_int.h"
>>>
>>> /*
>>> * Check that qemu_in_coroutine() works
>>> @@ -122,6 +123,35 @@ static void test_yield(void)
>>> g_assert_cmpint(i, ==, 5); /* coroutine must yield 5 times */
>>> }
>>>
>>> +static void coroutine_fn c2_fn(void *opaque)
>>> +{
>>> + fprintf(stderr, "c2 Part 1\n");
>>> + qemu_coroutine_yield();
>>> + fprintf(stderr, "c2 Part 2\n");
>>> +}
>>> +
>>> +static void coroutine_fn c1_fn(void *opaque)
>>> +{
>>> + Coroutine *c2 = opaque;
>>> +
>>> + fprintf(stderr, "c1 Part 1\n");
>>> + qemu_coroutine_enter(c2, NULL);
>>> + fprintf(stderr, "c1 Part 2\n");
>>> +}
>>> +
>>> +static void test_co_queue(void)
>>> +{
>>> + Coroutine *c1;
>>> + Coroutine *c2;
>>> +
>>> + c1 = qemu_coroutine_create(c1_fn);
>>> + c2 = qemu_coroutine_create(c2_fn);
>>> +
>>> + qemu_coroutine_enter(c1, c2);
>>> + memset(c1, 0xff, sizeof(Coroutine));
>>> + qemu_coroutine_enter(c2, NULL);
>>> +}
>>> +
>>> /*
>>> * Check that creation, enter, and return work
>>> */
>>> @@ -343,6 +373,7 @@ static void perf_cost(void)
>>> int main(int argc, char **argv)
>>> {
>>> g_test_init(&argc, &argv, NULL);
>>> + g_test_add_func("/basic/co_queue", test_co_queue);
>>> g_test_add_func("/basic/lifecycle", test_lifecycle);
>>> g_test_add_func("/basic/yield", test_yield);
>>> g_test_add_func("/basic/nesting", test_nesting);
>>>
>>> Here is the output (with printfs in coroutine_swap):
>>>
>>> -> coroutine_swap from MAIN to C1
>>> c1 Part 1
>>> -> coroutine_swap from C1 to C2
>>> c2 Part 1
>>> -> coroutine_swap from C2 to C1
>>> <- coroutine_swap from C1 to C2 switch yield
>>> c1 Part 2
>>> <- coroutine_swap from MAIN to C1 switch terminate
>>> coroutine_delete C1
>>> -> coroutine_swap from MAIN to C2
>>> <- coroutine_swap from C2 to C1 switch yield !!!
>>> c2 Part 2
>>> <- coroutine_swap from MAIN to C2 switch terminate
>>> coroutine_delete C2
>>>
>>> I have marked the problematic line with "!!!". The to=C1 variable is
>>> used after C1 has been deleted.
>>
>> How to build the test program? make test? I meet the following errors:
>> make test
>> make -C tests/tcg test
>> make[1]: Entering directory '/work/src/qemu/tests/tcg'
>> CC test_path.o
>> In file included from /work/src/qemu/include/qemu-common.h:43:0,
>> from /work/src/qemu/util/cutils.c:24,
>> from test_path.c:3:
>> /work/src/qemu/include/glib-compat.h:19:18: fatal error: glib.h: No such file or directory
>> #include <glib.h>
>> ^
>> compilation terminated.
>> /work/src/qemu/rules.mak:57: recipe for target 'test_path.o' failed
>> make[1]: *** [test_path.o] Error 1
>> make[1]: Leaving directory '/work/src/qemu/tests/tcg'
>> Makefile:428: recipe for target 'test' failed
>> make: *** [test] Error 2
>>
>> Do I miss something?
>>
>> Thanks
>> Wen Congyang
>>
>
> First, you need to install glib2-devel package which includes the glib.h. Then
> you can use "make check" in the source tree to compile and run all tests (or use
> "make check-help" to get some helps).
I have installed glib2-devel, and "make check" can work, but "make test" cannot work...
Thanks
Wen Congyang
>
>>>
>>> The test crashes since it writes 0xff to C1 after it has terminated.
>>>
>>>> Signed-off-by: Bin Wu <wu.wubin@huawei.com>
>>>> ---
>>>> qemu-coroutine.c | 16 ++++++++++------
>>>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
>>>> index 525247b..cc0bdfa 100644
>>>> --- a/qemu-coroutine.c
>>>> +++ b/qemu-coroutine.c
>>>> @@ -99,29 +99,31 @@ static void coroutine_delete(Coroutine *co)
>>>> qemu_coroutine_delete(co);
>>>> }
>>>>
>>>> -static void coroutine_swap(Coroutine *from, Coroutine *to)
>>>> +static CoroutineAction coroutine_swap(Coroutine *from, Coroutine *to)
>>>> {
>>>> CoroutineAction ret;
>>>>
>>>> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
>>>>
>>>> - qemu_co_queue_run_restart(to);
>>>> -
>>>> switch (ret) {
>>>> case COROUTINE_YIELD:
>>>> - return;
>>>> + break;
>>>> case COROUTINE_TERMINATE:
>>>> trace_qemu_coroutine_terminate(to);
>>>> + qemu_co_queue_run_restart(to);
>>>> coroutine_delete(to);
>>>> - return;
>>>> + break;
>>>> default:
>>>> abort();
>>>> }
>>>> +
>>>> + return ret;
>>>> }
>>>>
>>>> void qemu_coroutine_enter(Coroutine *co, void *opaque)
>>>> {
>>>> Coroutine *self = qemu_coroutine_self();
>>>> + CoroutineAction ret;
>>>>
>>>> trace_qemu_coroutine_enter(self, co, opaque);
>>>>
>>>> @@ -132,7 +134,9 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
>>>>
>>>> co->caller = self;
>>>> co->entry_arg = opaque;
>>>> - coroutine_swap(self, co);
>>>> + ret = coroutine_swap(self, co);
>>>> + if (ret == COROUTINE_YIELD)
>>>> + qemu_co_queue_run_restart(co);
>>>> }
>>>
>>> Your fix looks correct although QEMU coding style requires {}.
>>>
>>> I tried to think of a simpler solution that keeps a single
>>> qemu_co_queue_run_restart() call but was unable to find one.
>>>
>>> Please send another revision with a test-coroutine.c test case so we can
>>> protect against regressions.
>>>
>>> Thanks,
>>> Stefan
>>>
>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue
2015-02-09 14:48 ` Stefan Hajnoczi
2015-02-10 0:51 ` Bin Wu
2015-02-10 3:16 ` Wen Congyang
@ 2015-02-10 10:13 ` Kevin Wolf
2 siblings, 0 replies; 10+ messages in thread
From: Kevin Wolf @ 2015-02-10 10:13 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: famz, stefanha, subo7, kathy.wangting, bruce.fon, qemu-devel,
arei.gonglei, boby.chen, pbonzini, rudy.zhangmin, Bin Wu
[-- Attachment #1: Type: text/plain, Size: 8980 bytes --]
Am 09.02.2015 um 15:48 hat Stefan Hajnoczi geschrieben:
> On Mon, Feb 09, 2015 at 02:50:39PM +0800, Bin Wu wrote:
> > From: Bin Wu <wu.wubin@huawei.com>
> >
> > We tested VMs migration with their disk images by drive_mirror. With
> > migration, two VMs copyed large files between each other. During the
> > test, a segfault occured. The stack was as follow:
> >
> > (gdb) bt
> > qemu-coroutine-lock.c:66
> > to=0x7fa5a1798648) at qemu-coroutine.c:97
> > request=0x7fa28c2ffa10, reply=0x7fa28c2ffa30, qiov=0x0, offset=0) at
> > block/nbd-client.c:165
> > sector_num=8552704, nb_sectors=2040, qiov=0x7fa5a1757468, offset=0) at
> > block/nbd-client.c:262
> > sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468) at
> > block/nbd-client.c:296
> > nb_sectors=2048, qiov=0x7fa5a1757468) at block/nbd.c:291
> > req=0x7fa28c2ffbb0, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
> > flags=0) at block.c:3321
> > offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> > block.c:3447
> > sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> > block.c:3471
> > nb_sectors=2048, qiov=0x7fa5a1757468) at block.c:3480
> > nb_sectors=2048, qiov=0x7fa5a1757468) at block/raw_bsd.c:62
> > req=0x7fa28c2ffe30, offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468,
> > flags=0) at block.c:3321
> > offset=4378984448, bytes=1048576, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> > block.c:3447
> > sector_num=8552704, nb_sectors=2048, qiov=0x7fa5a1757468, flags=(unknown: 0)) at
> > block.c:3471
> > coroutine-ucontext.c:121
>
> This backtrace is incomplete. Where are the function names? The
> parameter lists appear incomplete too.
>
> > After analyzing the stack and reviewing the code, we find the
> > qemu_co_queue_run_restart should not be put in the coroutine_swap function which
> > can be invoked by qemu_coroutine_enter or qemu_coroutine_yield. Only
> > qemu_coroutine_enter needs to restart the co_queue.
> >
> > The error scenario is as follow: coroutine C1 enters C2, C2 yields
> > back to C1, then C1 ternimates and the related coroutine memory
> > becomes invalid. After a while, the C2 coroutine is entered again.
> > At this point, C1 is used as a parameter passed to
> > qemu_co_queue_run_restart. Therefore, qemu_co_queue_run_restart
> > accesses an invalid memory and a segfault error ocurrs.
> >
> > The qemu_co_queue_run_restart function re-enters coroutines waiting
> > in the co_queue. However, this function should be only used int the
> > qemu_coroutine_enter context. Only in this context, when the current
> > coroutine gets execution control again(after the execution of
> > qemu_coroutine_switch), we can restart the target coutine because the
> > target coutine has yielded back to the current coroutine or it has
> > terminated.
> >
> > First we want to put qemu_co_queue_run_restart in qemu_coroutine_enter,
> > but we find we can not access the target coroutine if it terminates.
>
> This example captures the scenario you describe:
>
> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
> index 525247b..883cbf5 100644
> --- a/qemu-coroutine.c
> +++ b/qemu-coroutine.c
> @@ -103,7 +103,10 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
> {
> CoroutineAction ret;
>
> + fprintf(stderr, "> %s from %p to %p\n", __func__, from, to);
> ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
> + fprintf(stderr, "< %s from %p to %p switch %s\n", __func__, from, to,
> + ret == COROUTINE_YIELD ? "yield" : "terminate");
>
> qemu_co_queue_run_restart(to);
>
> @@ -111,6 +114,7 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
> case COROUTINE_YIELD:
> return;
> case COROUTINE_TERMINATE:
> + fprintf(stderr, "coroutine_delete %p\n", to);
> trace_qemu_coroutine_terminate(to);
> coroutine_delete(to);
> return;
> diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
> index 27d1b6f..d44c428 100644
> --- a/tests/test-coroutine.c
> +++ b/tests/test-coroutine.c
> @@ -13,6 +13,7 @@
>
> #include <glib.h>
> #include "block/coroutine.h"
> +#include "block/coroutine_int.h"
>
> /*
> * Check that qemu_in_coroutine() works
> @@ -122,6 +123,35 @@ static void test_yield(void)
> g_assert_cmpint(i, ==, 5); /* coroutine must yield 5 times */
> }
>
> +static void coroutine_fn c2_fn(void *opaque)
> +{
> + fprintf(stderr, "c2 Part 1\n");
> + qemu_coroutine_yield();
> + fprintf(stderr, "c2 Part 2\n");
> +}
> +
> +static void coroutine_fn c1_fn(void *opaque)
> +{
> + Coroutine *c2 = opaque;
> +
> + fprintf(stderr, "c1 Part 1\n");
> + qemu_coroutine_enter(c2, NULL);
> + fprintf(stderr, "c1 Part 2\n");
> +}
> +
> +static void test_co_queue(void)
> +{
> + Coroutine *c1;
> + Coroutine *c2;
> +
> + c1 = qemu_coroutine_create(c1_fn);
> + c2 = qemu_coroutine_create(c2_fn);
> +
> + qemu_coroutine_enter(c1, c2);
> + memset(c1, 0xff, sizeof(Coroutine));
> + qemu_coroutine_enter(c2, NULL);
> +}
> +
> /*
> * Check that creation, enter, and return work
> */
> @@ -343,6 +373,7 @@ static void perf_cost(void)
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
> + g_test_add_func("/basic/co_queue", test_co_queue);
> g_test_add_func("/basic/lifecycle", test_lifecycle);
> g_test_add_func("/basic/yield", test_yield);
> g_test_add_func("/basic/nesting", test_nesting);
>
> Here is the output (with printfs in coroutine_swap):
>
> -> coroutine_swap from MAIN to C1
> c1 Part 1
> -> coroutine_swap from C1 to C2
> c2 Part 1
> -> coroutine_swap from C2 to C1
> <- coroutine_swap from C1 to C2 switch yield
> c1 Part 2
> <- coroutine_swap from MAIN to C1 switch terminate
> coroutine_delete C1
> -> coroutine_swap from MAIN to C2
> <- coroutine_swap from C2 to C1 switch yield !!!
> c2 Part 2
> <- coroutine_swap from MAIN to C2 switch terminate
> coroutine_delete C2
>
> I have marked the problematic line with "!!!". The to=C1 variable is
> used after C1 has been deleted.
>
> The test crashes since it writes 0xff to C1 after it has terminated.
>
> > Signed-off-by: Bin Wu <wu.wubin@huawei.com>
> > ---
> > qemu-coroutine.c | 16 ++++++++++------
> > 1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/qemu-coroutine.c b/qemu-coroutine.c
> > index 525247b..cc0bdfa 100644
> > --- a/qemu-coroutine.c
> > +++ b/qemu-coroutine.c
> > @@ -99,29 +99,31 @@ static void coroutine_delete(Coroutine *co)
> > qemu_coroutine_delete(co);
> > }
> >
> > -static void coroutine_swap(Coroutine *from, Coroutine *to)
> > +static CoroutineAction coroutine_swap(Coroutine *from, Coroutine *to)
> > {
> > CoroutineAction ret;
> >
> > ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
> >
> > - qemu_co_queue_run_restart(to);
> > -
> > switch (ret) {
> > case COROUTINE_YIELD:
> > - return;
> > + break;
> > case COROUTINE_TERMINATE:
> > trace_qemu_coroutine_terminate(to);
> > + qemu_co_queue_run_restart(to);
> > coroutine_delete(to);
> > - return;
> > + break;
> > default:
> > abort();
> > }
> > +
> > + return ret;
> > }
> >
> > void qemu_coroutine_enter(Coroutine *co, void *opaque)
> > {
> > Coroutine *self = qemu_coroutine_self();
> > + CoroutineAction ret;
> >
> > trace_qemu_coroutine_enter(self, co, opaque);
> >
> > @@ -132,7 +134,9 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
> >
> > co->caller = self;
> > co->entry_arg = opaque;
> > - coroutine_swap(self, co);
> > + ret = coroutine_swap(self, co);
> > + if (ret == COROUTINE_YIELD)
> > + qemu_co_queue_run_restart(co);
> > }
>
> Your fix looks correct although QEMU coding style requires {}.
>
> I tried to think of a simpler solution that keeps a single
> qemu_co_queue_run_restart() call but was unable to find one.
This one-liner is the core of the real fix:
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -148,5 +148,5 @@ void coroutine_fn qemu_coroutine_yield(void)
}
self->caller = NULL;
- coroutine_swap(self, to);
+ qemu_coroutine_switch(self, to, COROUTINE_YIELD);
}
There is no reason at all for qemu_coroutine_yield() to go through
coroutine_swap(). The cleanups that can be made on top is to inline
coroutine_swap() in qemu_coroutine_enter(), and to change its use of
COROUTINE_YIELD into a new COROUTINE_ENTER (which is never looked at
anyway, but we're not yielding here).
All the other solutions take a unnecessarily complicated design as
granted and further complicate it to distinguish two unrelated code
paths.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-02-10 10:13 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-09 6:50 [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue Bin Wu
2015-02-09 9:09 ` Paolo Bonzini
2015-02-10 0:55 ` Bin Wu
2015-02-09 9:42 ` Kevin Wolf
2015-02-09 14:48 ` Stefan Hajnoczi
2015-02-10 0:51 ` Bin Wu
2015-02-10 3:16 ` Wen Congyang
2015-02-10 3:48 ` Bin Wu
2015-02-10 4:49 ` Wen Congyang
2015-02-10 10:13 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.