On 09/02/2015 07:50, Bin Wu wrote: > From: Bin Wu <wu.wu...@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.wu...@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) >