From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39356) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKiBp-0002Ca-PL for qemu-devel@nongnu.org; Mon, 09 Feb 2015 01:51:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YKiBm-0000O7-F7 for qemu-devel@nongnu.org; Mon, 09 Feb 2015 01:51:25 -0500 Received: from szxga01-in.huawei.com ([119.145.14.64]:30891) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKiBl-0000Np-QN for qemu-devel@nongnu.org; Mon, 09 Feb 2015 01:51:22 -0500 From: Bin Wu Date: Mon, 9 Feb 2015 14:50:39 +0800 Message-ID: <1423464639-4764-1-git-send-email-wu.wubin@huawei.com> MIME-Version: 1.0 Content-Type: text/plain Subject: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, boby.chen@huawei.com, subo7@huawei.com, kathy.wangting@huawei.com, rudy.zhangmin@huawei.com, arei.gonglei@huawei.com, stefanha@redhat.com, pbonzini@redhat.com, bruce.fon@huawei.com, Bin Wu From: Bin Wu 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 --- 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