From patchwork Fri Aug 11 22:12:47 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 9896647 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 0003060351 for ; Fri, 11 Aug 2017 22:13:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E647F27F97 for ; Fri, 11 Aug 2017 22:13:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id DADE728475; Fri, 11 Aug 2017 22:13:55 +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.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI 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 6912A27F97 for ; Fri, 11 Aug 2017 22:13:53 +0000 (UTC) Received: from localhost ([::1]:53243 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dgIBk-00057a-Sl for patchwork-qemu-devel@patchwork.kernel.org; Fri, 11 Aug 2017 18:13:52 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38060) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dgIAs-00055O-Ls for qemu-devel@nongnu.org; Fri, 11 Aug 2017 18:13:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dgIAr-0003Ao-1n for qemu-devel@nongnu.org; Fri, 11 Aug 2017 18:12:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48828) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dgIAl-000382-0l; Fri, 11 Aug 2017 18:12:51 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D1D13A24FF; Fri, 11 Aug 2017 22:12:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D1D13A24FF Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=eblake@redhat.com Received: from [10.10.120.43] (ovpn-120-43.rdu2.redhat.com [10.10.120.43]) by smtp.corp.redhat.com (Postfix) with ESMTP id 55B5A1712C; Fri, 11 Aug 2017 22:12:48 +0000 (UTC) To: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org References: <20170811023759.26390-1-eblake@redhat.com> <20170811023759.26390-2-eblake@redhat.com> <29dc6e29-7bbc-2c81-22cc-61db4e8ac609@virtuozzo.com> <7d6be617-6907-5213-941b-38fe5d3f0fee@redhat.com> <325cc2fd-277a-d0d2-53d9-8f46418a6a36@redhat.com> From: Eric Blake Openpgp: url=http://people.redhat.com/eblake/eblake.gpg Organization: Red Hat, Inc. Message-ID: Date: Fri, 11 Aug 2017 17:12:47 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 11 Aug 2017 22:12:49 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [Qemu-devel] [PATCH 1/2] nbd: Drop connection if broken server is detected 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 , Paolo Bonzini , "open list:Network Block Dev..." , Max Reitz Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On 08/11/2017 03:01 PM, Eric Blake wrote: > On 08/11/2017 02:41 PM, Eric Blake wrote: >>> Hmm, was it correct even before your patch? Is it safe to enter a coroutine >>> (which we've scheduled by nbd_recv_coroutines_enter_all()), which is >>> actually >>> yielded inside nbd_rwv (not our yield in nbd_co_receive_reply)? >> >> I'm honestly not sure how to answer the question. In my testing, I was >> unable to catch a coroutine yielding inside of nbd_rwv(); > > Single stepping through nbd_rwv(), I see that I/O is performed by > sendmsg(), which either gets the message sent or, because of nonblocking > mode, fails with EAGAIN, which gets turned into QIO_CHANNEL_ERR_BLOCK > and indeed a call to qemu_channel_yield() within nbd_rwv() - but it's > timing sensitive, so I still haven't been able to provoke this scenario > using gdb. With this compiled into the client: and that got further, only to crash at: (gdb) c Continuing. readv failed: Input/output error aio_write failed: Input/output error qemu-io: block/block-backend.c:1211: blk_aio_write_entry: Assertion `!rwco->qiov || rwco->qiov->size == acb->bytes' failed. where rwco->qiov->size was garbage. At this point, while I still think it might be possible to come up with a less-invasive solution than your v2 patch, I'm also at the point where I want the bug fixed rather than me wallowing around trying to debug coroutine interactions; and thus I'm leaning towards your v2 patch as being more likely to be robust in the face of concurrency (it's not killing ioc while other coroutines still exist, so much as just making sure that every yield point checks if the kill switch has been triggered for a short-circuit exit). So I will probably be taking your version and creating a pull request for -rc3 on Monday. (Before I fully ack your version, though, I _will_ be hammering at it under gdb the same way I hammered at mine) diff --git i/nbd/common.c w/nbd/common.c index a2f28f2eec..f10e991eed 100644 --- i/nbd/common.c +++ w/nbd/common.c @@ -36,6 +36,10 @@ ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov, size_t niov, size_t length, while (nlocal_iov > 0) { ssize_t len; + static int hack; + if (hack) { + len = QIO_CHANNEL_ERR_BLOCK; + } else if (do_read) { len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp); } else { I was able to set a breakpoint in gdb to temporarily manipulate 'hack' at the right moment, in order to trigger what would happen if a nbd_rwv() hit EAGAIN. And sadly, I got a segfault using my patches, because the last reference to ioc had been cleared in the meantime. Program received signal SIGSEGV, Segmentation fault. 0x000055555562ee94 in object_class_dynamic_cast_assert (class=0x555555d9d1b0, typename=0x5555556c856d "qio-channel", file=0x5555556c8560 "io/channel.c", line=75, func=0x5555556c8670 <__func__.21506> "qio_channel_writev_full") at qom/object.c:705 705 trace_object_class_dynamic_cast_assert(class ? class->type->name : "(null)", #0 0x000055555562ee94 in object_class_dynamic_cast_assert ( class=0x555555d9d1b0, typename=0x5555556c856d "qio-channel", file=0x5555556c8560 "io/channel.c", line=75, func=0x5555556c8670 <__func__.21506> "qio_channel_writev_full") at qom/object.c:705 #1 0x000055555562312d in qio_channel_writev_full (ioc=0x555555d9bde0, iov=0x555555d9ec90, niov=1, fds=0x0, nfds=0, errp=0x0) at io/channel.c:75 #2 0x0000555555623233 in qio_channel_writev (ioc=0x555555d9bde0, iov=0x555555d9ec90, niov=1, errp=0x0) at io/channel.c:102 #3 0x0000555555603590 in nbd_rwv (ioc=0x555555d9bde0, iov=0x555555d9ecd0, niov=1, length=1048576, do_read=false, errp=0x0) at nbd/common.c:46 #4 0x00005555555ebcca in nbd_co_send_request (bs=0x555555d94260, request=0x7fffda2819f0, qiov=0x555555d9ee88) at block/nbd-client.c:152 My next test is whether incrementing the ref-count to s->ioc for the duration of nbd_co_send_request() is adequate to protect against this problem: diff --git i/block/nbd-client.c w/block/nbd-client.c index 28b10f3fa2..cb0c4ebedf 100644 --- i/block/nbd-client.c +++ w/block/nbd-client.c @@ -147,6 +147,11 @@ static int nbd_co_send_request(BlockDriverState *bs, return -EPIPE; } + /* + * Make sure ioc stays live, even if another coroutine decides to + * kill the connection because of a server error. + */ + object_ref(OBJECT(ioc)); if (qiov) { qio_channel_set_cork(ioc, true); rc = nbd_send_request(ioc, request); @@ -161,6 +166,7 @@ static int nbd_co_send_request(BlockDriverState *bs, } else { rc = nbd_send_request(ioc, request); } + object_unref(OBJECT(ioc)); qemu_co_mutex_unlock(&s->send_mutex); return rc; }