From nobody Thu May 2 19:34:14 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) by mx.zohomail.com with SMTPS id 1502746550403670.8667848476981; Mon, 14 Aug 2017 14:35:50 -0700 (PDT) Received: from localhost ([::1]:56709 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dhN1Y-0006Y6-0o for importer@patchew.org; Mon, 14 Aug 2017 17:35:48 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38366) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dhN0Q-0005u7-V8 for qemu-devel@nongnu.org; Mon, 14 Aug 2017 17:34:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dhN0P-0006Ra-Os for qemu-devel@nongnu.org; Mon, 14 Aug 2017 17:34:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58894) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dhN0J-0006K9-S4; Mon, 14 Aug 2017 17:34:32 -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 A6172CA36B; Mon, 14 Aug 2017 21:34:30 +0000 (UTC) Received: from red.redhat.com (ovpn-120-199.rdu2.redhat.com [10.10.120.199]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1339D60BED; Mon, 14 Aug 2017 21:34:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A6172CA36B Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=eblake@redhat.com From: Eric Blake To: qemu-devel@nongnu.org Date: Mon, 14 Aug 2017 16:34:26 -0500 Message-Id: <20170814213426.24681-1-eblake@redhat.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.38]); Mon, 14 Aug 2017 21:34:30 +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 Subject: [Qemu-devel] [PATCH for-2.10 v2] nbd-client: Fix regression when server sends garbage 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+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" When we switched NBD to use coroutines for qemu 2.9 (in particular, commit a12a712a), we introduced a regression: if a server sends us garbage (such as a corrupted magic number), we quit the read loop but do not stop sending further queued commands, resulting in the client hanging when it never reads the response to those additional commands. In qemu 2.8, we properly detected that the server is no longer reliable, and cancelled all existing pending commands with EIO, then tore down the socket so that all further command attempts get EPIPE. Restore the proper behavior of quitting (almost) all communication with a broken server: Once we know we are out of sync or otherwise can't trust the server, we must assume that any further incoming data is unreliable and therefore end all pending commands with EIO, and quit trying to send any further commands. As an exception, we still (try to) send NBD_CMD_DISC to let the server know we are going away (in part, because it is easier to do that than to further refactor nbd_teardown_connection, and in part because it is the only command where we do not have to wait for a reply). Based on a patch by Vladimir Sementsov-Ogievskiy. A malicious server can be created with the following hack, followed by setting NBD_SERVER_DEBUG to a non-zero value in the environment when running qemu-nbd: | --- a/nbd/server.c | +++ b/nbd/server.c | @@ -919,6 +919,17 @@ static int nbd_send_reply(QIOChannel *ioc, NBDReply = *reply, Error **errp) | stl_be_p(buf + 4, reply->error); | stq_be_p(buf + 8, reply->handle); | | + static int debug; | + static int count; | + if (!count++) { | + const char *str =3D getenv("NBD_SERVER_DEBUG"); | + if (str) { | + debug =3D atoi(str); | + } | + } | + if (debug && !(count % debug)) { | + buf[0] =3D 0; | + } | return nbd_write(ioc, buf, sizeof(buf), errp); | } Reported-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Eric Blake Reviewed-by: Stefan Hajnoczi --- Supercedes both Vladimir and my earlier attempts: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02131.html https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01501.html block/nbd-client.h | 1 + block/nbd-client.c | 14 ++++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/block/nbd-client.h b/block/nbd-client.h index df80771357..1935ffbcaa 100644 --- a/block/nbd-client.h +++ b/block/nbd-client.h @@ -29,6 +29,7 @@ typedef struct NBDClientSession { Coroutine *recv_coroutine[MAX_NBD_REQUESTS]; NBDReply reply; + bool quit; } NBDClientSession; NBDClientSession *nbd_get_client_session(BlockDriverState *bs); diff --git a/block/nbd-client.c b/block/nbd-client.c index 25dd28406b..bb17e3da45 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -73,7 +73,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaqu= e) int ret; Error *local_err =3D NULL; - for (;;) { + while (!s->quit) { assert(s->reply.handle =3D=3D 0); ret =3D nbd_receive_reply(s->ioc, &s->reply, &local_err); if (ret < 0) { @@ -107,6 +107,9 @@ static coroutine_fn void nbd_read_reply_entry(void *opa= que) qemu_coroutine_yield(); } + if (ret < 0) { + s->quit =3D true; + } nbd_recv_coroutines_enter_all(s); s->read_reply_co =3D NULL; } @@ -135,6 +138,10 @@ static int nbd_co_send_request(BlockDriverState *bs, assert(i < MAX_NBD_REQUESTS); request->handle =3D INDEX_TO_HANDLE(s, i); + if (s->quit) { + qemu_co_mutex_unlock(&s->send_mutex); + return -EIO; + } if (!s->ioc) { qemu_co_mutex_unlock(&s->send_mutex); return -EPIPE; @@ -143,7 +150,7 @@ static int nbd_co_send_request(BlockDriverState *bs, if (qiov) { qio_channel_set_cork(s->ioc, true); rc =3D nbd_send_request(s->ioc, request); - if (rc >=3D 0) { + if (rc >=3D 0 && !s->quit) { ret =3D nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, f= alse, NULL); if (ret !=3D request->len) { @@ -168,8 +175,7 @@ static void nbd_co_receive_reply(NBDClientSession *s, /* Wait until we're woken up by nbd_read_reply_entry. */ qemu_coroutine_yield(); *reply =3D s->reply; - if (reply->handle !=3D request->handle || - !s->ioc) { + if (reply->handle !=3D request->handle || !s->ioc || s->quit) { reply->error =3D EIO; } else { if (qiov && reply->error =3D=3D 0) { --=20 2.13.5