From patchwork Fri Aug 11 14:15:08 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 800575 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=nongnu.org (client-ip=2001:4830:134:3::11; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) 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 3xTRry5D74z9sRq for ; Sat, 12 Aug 2017 00:16:50 +1000 (AEST) Received: from localhost ([::1]:43656 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dgAk4-00021a-A1 for incoming@patchwork.ozlabs.org; Fri, 11 Aug 2017 10:16:48 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59796) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dgAig-0001Ii-5Z for qemu-devel@nongnu.org; Fri, 11 Aug 2017 10:15:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dgAie-0007Ci-Ji for qemu-devel@nongnu.org; Fri, 11 Aug 2017 10:15:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39188) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dgAiZ-0007Bx-Ll; Fri, 11 Aug 2017 10:15:15 -0400 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6F4CC76049; Fri, 11 Aug 2017 14:15:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6F4CC76049 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.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 88A22B2417; Fri, 11 Aug 2017 14:15:09 +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> From: Eric Blake Openpgp: url=http://people.redhat.com/eblake/eblake.gpg Organization: Red Hat, Inc. Message-ID: <7d6be617-6907-5213-941b-38fe5d3f0fee@redhat.com> Date: Fri, 11 Aug 2017 09:15:08 -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: <29dc6e29-7bbc-2c81-22cc-61db4e8ac609@virtuozzo.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 11 Aug 2017 14:15:14 +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+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" On 08/11/2017 02:48 AM, Vladimir Sementsov-Ogievskiy wrote: > 11.08.2017 05:37, Eric Blake wrote: >> As soon as the server is sending us garbage, we should quit >> trying to send further messages to the server, and allow all >> pending coroutines for any remaining replies to error out. >> Failure to do so can let a malicious server cause the client >> to hang, for example, if the server sends an invalid magic >> number in its response. >> @@ -107,8 +108,12 @@ static coroutine_fn void >> nbd_read_reply_entry(void *opaque) >> qemu_coroutine_yield(); >> } >> >> + s->reply.handle = 0; >> nbd_recv_coroutines_enter_all(s); >> s->read_reply_co = NULL; >> + if (ret < 0) { >> + nbd_teardown_connection(bs); >> + } > > what if it happens in parallel with nbd_co_send_request? > nbd_teardown_connectin destroys s->ioc, nbd_co_send_requests > checks s->ioc only once and then calls nbd_send_request (which is > finally nbd_rwv and may yield). I think nbd_rwv is not > prepared to sudden destruction of ioc.. The nbd_recv_coroutines_enter_all() call schedules all pending nbd_co_send_request coroutines to fire as soon as the current coroutine reaches a yield point. The next yield point is during the BDRV_POLL_WHILE of nbd_teardown_connection - but this is AFTER we've called qio_channel_shutdown() - so as long as nbd_rwv() is called with a valid ioc, the qio code should recognize that we are shutting down the connection and gracefully give an error on each write attempt. I see your point about the fact that coroutines can change hands in between our two writes for an NBD_CMD_WRITE in nbd_co_send_request() (the first write is nbd_send_request() for the header, the second is nbd_rwv() for the data) - if between those two writes we process a failing read, I see your point about us risking re-reading s->ioc as NULL for the second write call. But maybe this is an appropriate fix - hanging on to the ioc that we learned when grabbing the send_mutex: return rc; But I'm really hoping Paolo will chime in on this thread. diff --git a/block/nbd-client.c b/block/nbd-client.c index 802d50b636..28b10f3fa2 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -122,6 +122,7 @@ static int nbd_co_send_request(BlockDriverState *bs, { NBDClientSession *s = nbd_get_client_session(bs); int rc, ret, i; + QIOChannel *ioc; qemu_co_mutex_lock(&s->send_mutex); while (s->in_flight == MAX_NBD_REQUESTS) { @@ -139,25 +140,26 @@ static int nbd_co_send_request(BlockDriverState *bs, g_assert(qemu_in_coroutine()); assert(i < MAX_NBD_REQUESTS); request->handle = INDEX_TO_HANDLE(s, i); + ioc = s->ioc; - if (!s->ioc) { + if (!ioc) { qemu_co_mutex_unlock(&s->send_mutex); return -EPIPE; } if (qiov) { - qio_channel_set_cork(s->ioc, true); - rc = nbd_send_request(s->ioc, request); + qio_channel_set_cork(ioc, true); + rc = nbd_send_request(ioc, request); if (rc >= 0) { - ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false, + ret = nbd_rwv(ioc, qiov->iov, qiov->niov, request->len, false, NULL); if (ret != request->len) { rc = -EIO; } } - qio_channel_set_cork(s->ioc, false); + qio_channel_set_cork(ioc, false); } else { - rc = nbd_send_request(s->ioc, request); + rc = nbd_send_request(ioc, request); } qemu_co_mutex_unlock(&s->send_mutex);