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

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

[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
#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 given
in the following patch log.

Bin Wu (1):
  qemu-coroutine: fix qemu_co_queue_run_restart error

 qemu-coroutine.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH] qemu-coroutine: fix qemu_co_queue_run_restart error

[Bin Wu]

From: Bin Wu <wu.wubin@huawei.com>

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 | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/qemu-coroutine.c b/qemu-coroutine.c
index 525247b..9a294c4 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -99,24 +99,25 @@ 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)
@@ -133,6 +134,8 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
     co->caller = self;
     co->entry_arg = opaque;
     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

Re: [Qemu-devel] [PATCH] qemu-coroutine: fix qemu_co_queue_run_restart error

[Bin Wu]

sorry, there is a mistake in this patch: the "ret" variable is not
defined :<

I will send a new patch to fix this problem.

On 2015/2/9 12:09, Bin Wu wrote:
> From: Bin Wu <wu.wubin@huawei.com>
> 
> 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 | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
> index 525247b..9a294c4 100644
> --- a/qemu-coroutine.c
> +++ b/qemu-coroutine.c
> @@ -99,24 +99,25 @@ 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)
> @@ -133,6 +134,8 @@ void qemu_coroutine_enter(Coroutine *co, void *opaque)
>      co->caller = self;
>      co->entry_arg = opaque;
>      coroutine_swap(self, co);
> +    if (ret == COROUTINE_YIELD)
> +        qemu_co_queue_run_restart(co);
>  }
>  
>  void coroutine_fn qemu_coroutine_yield(void)
> 

-- 
Bin Wu