Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue

[Wen Congyang]

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 <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).

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 <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
>>>
>>
>>
>> .
>>
>