[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_
From: |
Bin Wu |
Subject: |
Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue |
Date: |
Tue, 10 Feb 2015 11:48:21 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 |
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 <address@hidden>
>>>
>>> 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 <address@hidden>
>>> ---
>>> 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