From patchwork Tue May 30 10:07:36 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roman Pen X-Patchwork-Id: 9754243 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 0422A602F0 for ; Tue, 30 May 2017 10:08:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E987B26E49 for ; Tue, 30 May 2017 10:08:57 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id DE06427569; Tue, 30 May 2017 10:08:57 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 254E827FAD for ; Tue, 30 May 2017 10:08:56 +0000 (UTC) Received: from localhost ([::1]:52630 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dFe5A-0002hf-0S for patchwork-qemu-devel@patchwork.kernel.org; Tue, 30 May 2017 06:08:56 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51560) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dFe4a-0002hO-NW for qemu-devel@nongnu.org; Tue, 30 May 2017 06:08:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dFe4W-0000he-Jx for qemu-devel@nongnu.org; Tue, 30 May 2017 06:08:20 -0400 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]:38479) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dFe4W-0000hN-9Y for qemu-devel@nongnu.org; Tue, 30 May 2017 06:08:16 -0400 Received: by mail-wm0-x231.google.com with SMTP id e127so93214357wmg.1 for ; Tue, 30 May 2017 03:08:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=profitbricks-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id; bh=gqxvPZZ0N1YT9LDQo6feYN6Hy2l4Kaplytazpef95Pw=; b=QenicboeIYvET1n+h5/n2IZ5eZlR1ytTZGApYVsXG7YpGu4oEII3QaKw0PvDcx1Tps xLER/OKOTdCIMmrPMnaCJ8bMYCvXPrPhTa8znuKHmeEJCq/EaAkZRRSy0P4DuD5+j5N7 LA9pOpFJ3+wandrthSaA/YgLqujU0FFXRq3ZY2k3kh/fd+Wy+bD83gC5d6MB8mANFzia HTy3E3DUXnbJmLHqP6pxPTZE/tMzS22McSyBUQFJcsWv5zRgZOs1PbqCe/XjhKZ6WYf0 fag8D3dkUtD7txpaxCXapUba70LCf6VvddO2OC1Bd1woqQ2FuxLIcdBmx8IwptPyQhdR PiFA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=gqxvPZZ0N1YT9LDQo6feYN6Hy2l4Kaplytazpef95Pw=; b=VPweP8UN5PnUapdwPjg4g8WULsG6yVvjUQT/Lb5RiObb3RlBicl4BxAVEF3FiJfu1K HelKjb7VUWFDt7F56bP61e7d2CVirEwm/NkY8tSEgdH/073ASpSQWowYkfQCMu4s30st 8ot6h2kUc+DhEvkyaeRk31uiPvi/oY1aBqy+2Q6ummxHjRz9yAUcvpvAYKYAsgXBjfYN yPP/aWl/xgmHoGTp+EdFzjg2jQd/dGkdBi/hMELs2EXxCI4JeQSMambvPIMyO1CgZfO0 ihpKKsoBuw2zFPT4gSYTL4zT4F0cnqEg2Bi4/R3yZZIIi8EZru3qOr9ieWDG11xHEFrv SFWw== X-Gm-Message-State: AODbwcDMfVWp3vPIFjGE+3YIVuRP8vL065EDotj6XD1mbEEL39aAB1ZH COSDfYQeesVJ2Nkk X-Received: by 10.223.162.219 with SMTP id t27mr13446055wra.28.1496138895238; Tue, 30 May 2017 03:08:15 -0700 (PDT) Received: from pb.pb.local ([62.217.45.26]) by smtp.gmail.com with ESMTPSA id 4sm19538462wrv.33.2017.05.30.03.08.08 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 30 May 2017 03:08:09 -0700 (PDT) From: Roman Pen To: Date: Tue, 30 May 2017 12:07:36 +0200 Message-Id: <20170530100736.3338-1-roman.penyaev@profitbricks.com> X-Mailer: git-send-email 2.11.0 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2a00:1450:400c:c09::231 Subject: [Qemu-devel] [PATCH v2 1/1] coroutine-lock: do not touch coroutine after another one has been entered X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , Fam Zheng , Roman Pen , qemu-devel@nongnu.org, Stefan Hajnoczi , Paolo Bonzini Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP Submission of requests on linux aio is a bit tricky and can lead to requests completions on submission path: 44713c9e8547 ("linux-aio: Handle io_submit() failure gracefully") 0ed93d84edab ("linux-aio: process completions from ioq_submit()") That means that any coroutine which has been yielded in order to wait for completion can be resumed from submission path and be eventually terminated (freed). The following use-after-free crash was observed when IO throttling was enabled: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7f5813dff700 (LWP 56417)] virtqueue_unmap_sg (elem=0x7f5804009a30, len=1, vq=) at virtio.c:252 (gdb) bt #0 virtqueue_unmap_sg (elem=0x7f5804009a30, len=1, vq=) at virtio.c:252 ^^^^^^^^^^^^^^ remember the address #1 virtqueue_fill (vq=0x5598b20d21b0, elem=0x7f5804009a30, len=1, idx=0) at virtio.c:282 #2 virtqueue_push (vq=0x5598b20d21b0, elem=elem@entry=0x7f5804009a30, len=) at virtio.c:308 #3 virtio_blk_req_complete (req=req@entry=0x7f5804009a30, status=status@entry=0 '\000') at virtio-blk.c:61 #4 virtio_blk_rw_complete (opaque=, ret=0) at virtio-blk.c:126 #5 blk_aio_complete (acb=0x7f58040068d0) at block-backend.c:923 #6 coroutine_trampoline (i0=, i1=) at coroutine-ucontext.c:78 (gdb) p * elem $8 = {index = 77, out_num = 2, in_num = 1, in_addr = 0x7f5804009ad8, out_addr = 0x7f5804009ae0, in_sg = 0x0, out_sg = 0x7f5804009a50} ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 'in_sg' and 'out_sg' are invalid. e.g. it is impossible that 'in_sg' is zero, instead its value must be equal to: (gdb) p/x 0x7f5804009ad8 + sizeof(elem->in_addr[0]) + 2 * sizeof(elem->out_addr[0]) $26 = 0x7f5804009af0 Seems 'elem' was corrupted. Meanwhile another thread raised an abort: Thread 12 (Thread 0x7f57f2ffd700 (LWP 56426)): #0 raise () from /lib/x86_64-linux-gnu/libc.so.6 #1 abort () from /lib/x86_64-linux-gnu/libc.so.6 #2 qemu_coroutine_enter (co=0x7f5804009af0) at qemu-coroutine.c:113 #3 qemu_co_queue_run_restart (co=0x7f5804009a30) at qemu-coroutine-lock.c:60 #4 qemu_coroutine_enter (co=0x7f5804009a30) at qemu-coroutine.c:119 ^^^^^^^^^^^^^^^^^^ WTF?? this is equal to elem from crashed thread #5 qemu_co_queue_run_restart (co=0x7f57e7f16ae0) at qemu-coroutine-lock.c:60 #6 qemu_coroutine_enter (co=0x7f57e7f16ae0) at qemu-coroutine.c:119 #7 qemu_co_queue_run_restart (co=0x7f5807e112a0) at qemu-coroutine-lock.c:60 #8 qemu_coroutine_enter (co=0x7f5807e112a0) at qemu-coroutine.c:119 #9 qemu_co_queue_run_restart (co=0x7f5807f17820) at qemu-coroutine-lock.c:60 #10 qemu_coroutine_enter (co=0x7f5807f17820) at qemu-coroutine.c:119 #11 qemu_co_queue_run_restart (co=0x7f57e7f18e10) at qemu-coroutine-lock.c:60 #12 qemu_coroutine_enter (co=0x7f57e7f18e10) at qemu-coroutine.c:119 #13 qemu_co_enter_next (queue=queue@entry=0x5598b1e742d0) at qemu-coroutine-lock.c:106 #14 timer_cb (blk=0x5598b1e74280, is_write=) at throttle-groups.c:419 Crash can be explained by access of 'co' object from the loop inside qemu_co_queue_run_restart(): while ((next = QSIMPLEQ_FIRST(&co->co_queue_wakeup))) { QSIMPLEQ_REMOVE_HEAD(&co->co_queue_wakeup, co_queue_next); ^^^^^^^^^^^^^^^^^^^^ on each iteration 'co' is accessed, but 'co' can be already freed qemu_coroutine_enter(next); } When 'next' coroutine is resumed (entered) it can in its turn resume 'co', and eventually free it. That's why we see 'co' (which was freed) has the same address as 'elem' from the first backtrace. The fix is obvious: use temporary queue and do not touch coroutine after first qemu_coroutine_enter() is invoked. The issue is quite rare and happens every ~12 hours on very high IO and CPU load (building linux kernel with -j512 inside guest) when IO throttling is enabled. With the fix applied guest is running ~35 hours and is still alive so far. Signed-off-by: Roman Pen Cc: Paolo Bonzini Cc: Fam Zheng Cc: Stefan Hajnoczi Cc: Kevin Wolf Cc: qemu-devel@nongnu.org Reviewed-by: Fam Zheng --- v2: Comments tweaks suggested by Paolo. util/qemu-coroutine-lock.c | 14 ++++++++++++-- util/qemu-coroutine.c | 5 +++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c index 6328eed26bc6..d589d8c66d5e 100644 --- a/util/qemu-coroutine-lock.c +++ b/util/qemu-coroutine-lock.c @@ -77,10 +77,20 @@ void coroutine_fn qemu_co_queue_wait(CoQueue *queue, CoMutex *mutex) void qemu_co_queue_run_restart(Coroutine *co) { Coroutine *next; + QSIMPLEQ_HEAD(, Coroutine) tmp_queue_wakeup = + QSIMPLEQ_HEAD_INITIALIZER(tmp_queue_wakeup); trace_qemu_co_queue_run_restart(co); - while ((next = QSIMPLEQ_FIRST(&co->co_queue_wakeup))) { - QSIMPLEQ_REMOVE_HEAD(&co->co_queue_wakeup, co_queue_next); + + /* Because "co" has yielded, any coroutine that we wakeup can resume it. + * If this happens and "co" terminates, co->co_queue_wakeup becomes + * invalid memory. Therefore, use a temporary queue and do not touch + * the "co" coroutine as soon as you enter another one. + */ + QSIMPLEQ_CONCAT(&tmp_queue_wakeup, &co->co_queue_wakeup); + + while ((next = QSIMPLEQ_FIRST(&tmp_queue_wakeup))) { + QSIMPLEQ_REMOVE_HEAD(&tmp_queue_wakeup, co_queue_next); qemu_coroutine_enter(next); } } diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c index 486af9a62275..d6095c1d5aa4 100644 --- a/util/qemu-coroutine.c +++ b/util/qemu-coroutine.c @@ -126,6 +126,11 @@ void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co) qemu_co_queue_run_restart(co); + /* Beware, if ret == COROUTINE_YIELD and qemu_co_queue_run_restart() + * has started any other coroutine, "co" might have been reentered + * and even freed by now! So be careful and do not touch it. + */ + switch (ret) { case COROUTINE_YIELD: return;