From nobody Sun Nov 9 14:49:23 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.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 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=fail(p=none dis=none) header.from=virtuozzo.com Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1550949786757394.9134145156271; Sat, 23 Feb 2019 11:23:06 -0800 (PST) Received: from localhost ([127.0.0.1]:41744 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gxcsy-0006pi-F1 for importer@patchew.org; Sat, 23 Feb 2019 14:22:56 -0500 Received: from eggs.gnu.org ([209.51.188.92]:54717) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gxcr1-0005rd-8g for qemu-devel@nongnu.org; Sat, 23 Feb 2019 14:20:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gxcqx-000226-Op for qemu-devel@nongnu.org; Sat, 23 Feb 2019 14:20:53 -0500 Received: from relay.sw.ru ([185.231.240.75]:59142) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gxcqr-0001z3-N8; Sat, 23 Feb 2019 14:20:47 -0500 Received: from [10.28.8.145] (helo=kvm.sw.ru) by relay.sw.ru with esmtp (Exim 4.91) (envelope-from ) id 1gxcqo-0004OX-8t; Sat, 23 Feb 2019 22:20:42 +0300 From: Vladimir Sementsov-Ogievskiy To: qemu-devel@nongnu.org, qemu-block@nongnu.org Date: Sat, 23 Feb 2019 22:20:40 +0300 Message-Id: <20190223192041.289782-3-vsementsov@virtuozzo.com> X-Mailer: git-send-email 2.18.0 In-Reply-To: <20190223192041.289782-1-vsementsov@virtuozzo.com> References: <20190223192041.289782-1-vsementsov@virtuozzo.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 185.231.240.75 Subject: [Qemu-devel] [PATCH 2/3] block: fix bdrv_check_perm for non-tree subgraph 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: kwolf@redhat.com, vsementsov@virtuozzo.com, den@virtuozzo.com, mreitz@redhat.com Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" bdrv_check_perm in it's recursion checks each node in context of new permissions for one parent, because of nature of DFS. It works well, while children subgraph of top-most updated node is a tree, i.e. it doesn't have any kind of loops. But if we have a loop (not oriented, of course), i.e. we have two different ways from top-node to some child-node, then bdrv_check_perm will do wrong thing: top | \ | | v v A B | | v v node It will once check new permissions of node in context of new A permissions and old B permissions and once visa-versa. It's a wrong way and may lead to corruption of permission system. We may start with no-permissions and all-shared for both A->node and B->node relations and finish up with non shared write permission for both ways. The following commit will add a test, which shows this bug. To fix this situation, let's really set BdrvChild permissions during bdrv_check_perm procedure. And we are happy here, as check-perm is already written in transaction manner, so we just need to restore backed-up permissions in _abort. Signed-off-by: Vladimir Sementsov-Ogievskiy --- Thing to discuss: in bdrv_child_abort_perm_update(), should we include "bdrv_abort_perm_update(c->bs);" into "if"? Or not? Anyway, if yes, it'd better be additional patch, as it may be other bug. Or, all things are prepared for abort even if check was not called for this particular child? include/block/block_int.h | 5 +++++ block.c | 27 ++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/include/block/block_int.h b/include/block/block_int.h index 0075bafd10..8437df85a2 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -662,6 +662,11 @@ struct BdrvChild { */ uint64_t shared_perm; =20 + /* backup of permissions during permission update procedure */ + bool has_backup_perm; + uint64_t backup_perm; + uint64_t backup_shared_perm; + QLIST_ENTRY(BdrvChild) next; QLIST_ENTRY(BdrvChild) next_parent; }; diff --git a/block.c b/block.c index d7c2ff655c..ab98e64305 100644 --- a/block.c +++ b/block.c @@ -1954,13 +1954,32 @@ static int bdrv_child_check_perm(BdrvChild *c, Bloc= kReopenQueue *q, ret =3D bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children= , errp); g_slist_free(ignore_children); =20 - return ret; + if (ret < 0) { + return ret; + } + + if (!c->has_backup_perm) { + c->has_backup_perm =3D true; + c->backup_perm =3D c->perm; + c->backup_shared_perm =3D c->shared_perm; + } + /* + * Note: it's OK if c->has_backup_perm was already set, as we can find= the + * same child twice during check_perm procedure + */ + + c->perm =3D perm; + c->shared_perm =3D shared; + + return 0; } =20 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shar= ed) { uint64_t cumulative_perms, cumulative_shared_perms; =20 + c->has_backup_perm =3D false; + c->perm =3D perm; c->shared_perm =3D shared; =20 @@ -1971,6 +1990,12 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64= _t perm, uint64_t shared) =20 static void bdrv_child_abort_perm_update(BdrvChild *c) { + if (c->has_backup_perm) { + c->perm =3D c->backup_perm; + c->shared_perm =3D c->backup_shared_perm; + c->has_backup_perm =3D false; + } + bdrv_abort_perm_update(c->bs); } =20 --=20 2.18.0