From patchwork Mon Feb 9 14:48:13 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Hajnoczi X-Patchwork-Id: 437956 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 8368E140145 for ; Tue, 10 Feb 2015 01:48:43 +1100 (AEDT) Received: from localhost ([::1]:33279 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKpdg-0005iZ-GR for incoming@patchwork.ozlabs.org; Mon, 09 Feb 2015 09:48:40 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52946) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKpdN-0005OZ-Os for qemu-devel@nongnu.org; Mon, 09 Feb 2015 09:48:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YKpdJ-0002Mx-Nd for qemu-devel@nongnu.org; Mon, 09 Feb 2015 09:48:21 -0500 Received: from mail-we0-x22b.google.com ([2a00:1450:400c:c03::22b]:55775) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKpdJ-0002Lm-An for qemu-devel@nongnu.org; Mon, 09 Feb 2015 09:48:17 -0500 Received: by mail-we0-f171.google.com with SMTP id k11so27406870wes.2 for ; Mon, 09 Feb 2015 06:48:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=phSeInYXAglfQGtHSx+RcV8BGZ7akIJxfu5WIXoRep8=; b=0vFMMFAGE15Tvq24gFDFqEsSE5EmZevxMbYDYPKKW4omoVFuUuammBN4ap/ZvN/g/J Ej91lt7Q/OiiqRVkRo1vHUOnq/hpzxJVCsZipOGAxP4s7/FMzuVdyFweRo1FC8qgEQVD hPX2QD0iiWP9KJbO/oYgk+bYI6Mgd44gogmDmfAG+PI+oHzonoAncEIyAul9GYV161fV lEFcQzIzwD70q65umw+ejXdnYeU9LJylfPwDVfERy3Qz4N8olqML+iFGW+PFHlVPDpHR 4KxFpev7A6s0zi3HYhYC/AtX8G9a+5zBUAVXgG7IHhUlyV10eJqiVl5zke+kWcZBbQYo tULA== X-Received: by 10.180.205.142 with SMTP id lg14mr27148541wic.82.1423493296211; Mon, 09 Feb 2015 06:48:16 -0800 (PST) Received: from localhost (251.138.208.46.dyn.plus.net. [46.208.138.251]) by mx.google.com with ESMTPSA id vh8sm16682084wjc.12.2015.02.09.06.48.14 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Feb 2015 06:48:15 -0800 (PST) Date: Mon, 9 Feb 2015 14:48:13 +0000 From: Stefan Hajnoczi To: Bin Wu Message-ID: <20150209144813.GA2076@stefanha-thinkpad.redhat.com> References: <1423464639-4764-1-git-send-email-wu.wubin@huawei.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1423464639-4764-1-git-send-email-wu.wubin@huawei.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:400c:c03::22b Cc: kwolf@redhat.com, famz@redhat.com, stefanha@redhat.com, subo7@huawei.com, kathy.wangting@huawei.com, bruce.fon@huawei.com, qemu-devel@nongnu.org, arei.gonglei@huawei.com, boby.chen@huawei.com, pbonzini@redhat.com, rudy.zhangmin@huawei.com Subject: Re: [Qemu-devel] [PATCH v2] qemu-coroutine: segfault when restarting co_queue X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Mon, Feb 09, 2015 at 02:50:39PM +0800, Bin Wu wrote: > 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 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: 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. The test crashes since it writes 0xff to C1 after it has terminated. > 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); > } 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 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 #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);