[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-block] [PULL 12/14] block: Ignore loosening perm restrictions fail
From: |
Kevin Wolf |
Subject: |
[Qemu-block] [PULL 12/14] block: Ignore loosening perm restrictions failures |
Date: |
Tue, 18 Jun 2019 17:23:16 +0200 |
From: Max Reitz <address@hidden>
We generally assume that loosening permission restrictions can never
fail. We have seen in the past that this assumption is wrong. This has
led to crashes because we generally pass &error_abort when loosening
permissions.
However, a failure in such a case should actually be handled in quite
the opposite way: It is very much not fatal, so qemu may report it, but
still consider the operation successful. The only realistic problem is
that qemu may then retain permissions and thus locks on images it
actually does not require. But again, that is not fatal.
To implement this behavior, we make all functions that change
permissions and that pass &error_abort to the initiating function
(bdrv_check_perm() or bdrv_child_check_perm()) evaluate the
@loosen_restrictions value introduced in the previous patch. If it is
true and an error did occur, we abort the permission update, discard the
error, and instead report success to the caller.
bdrv_child_try_set_perm() itself does not pass &error_abort, but it is
the only public function to change permissions. As such, callers may
pass &error_abort to it, expecting dropping permission restrictions to
never fail.
Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>
---
block.c | 44 ++++++++++++++++++++++++++++++++++++++------
1 file changed, 38 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index 1b10a5ce35..c139540f2b 100644
--- a/block.c
+++ b/block.c
@@ -2121,11 +2121,26 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
Error **errp)
{
+ Error *local_err = NULL;
int ret;
+ bool tighten_restrictions;
- ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, NULL, errp);
+ ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL,
+ &tighten_restrictions, &local_err);
if (ret < 0) {
bdrv_child_abort_perm_update(c);
+ if (tighten_restrictions) {
+ error_propagate(errp, local_err);
+ } else {
+ /*
+ * Our caller may intend to only loosen restrictions and
+ * does not expect this function to fail. Errors are not
+ * fatal in such a case, so we can just hide them from our
+ * caller.
+ */
+ error_free(local_err);
+ ret = 0;
+ }
return ret;
}
@@ -2308,10 +2323,19 @@ static void bdrv_replace_child(BdrvChild *child,
BlockDriverState *new_bs)
/* Update permissions for old node. This is guaranteed to succeed
* because we're just taking a parent away, so we're loosening
* restrictions. */
+ bool tighten_restrictions;
+ int ret;
+
bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
- bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
- NULL, &error_abort);
- bdrv_set_perm(old_bs, perm, shared_perm);
+ ret = bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
+ &tighten_restrictions, NULL);
+ assert(tighten_restrictions == false);
+ if (ret < 0) {
+ /* We only tried to loosen restrictions, so errors are not fatal */
+ bdrv_abort_perm_update(old_bs);
+ } else {
+ bdrv_set_perm(old_bs, perm, shared_perm);
+ }
/* When the parent requiring a non-default AioContext is removed, the
* node moves back to the main AioContext */
@@ -5386,6 +5410,7 @@ static bool bdrv_has_bds_parent(BlockDriverState *bs,
bool only_active)
static int bdrv_inactivate_recurse(BlockDriverState *bs)
{
BdrvChild *child, *parent;
+ bool tighten_restrictions;
uint64_t perm, shared_perm;
int ret;
@@ -5422,8 +5447,15 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
/* Update permissions, they may differ for inactive nodes */
bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
- bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &error_abort);
- bdrv_set_perm(bs, perm, shared_perm);
+ ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL,
+ &tighten_restrictions, NULL);
+ assert(tighten_restrictions == false);
+ if (ret < 0) {
+ /* We only tried to loosen restrictions, so errors are not fatal */
+ bdrv_abort_perm_update(bs);
+ } else {
+ bdrv_set_perm(bs, perm, shared_perm);
+ }
/* Recursively inactivate children */
--
2.20.1
- [Qemu-block] [PULL 03/14] block/block-backend: blk_iostatus_reset: drop usage of bs->job, (continued)
- [Qemu-block] [PULL 03/14] block/block-backend: blk_iostatus_reset: drop usage of bs->job, Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 02/14] block/replication: drop usage of bs->job, Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 09/14] block/commit: Drop bdrv_child_try_set_perm(), Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 08/14] block/mirror: Fix child permissions, Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 05/14] block: drop bs->job, Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 06/14] file-posix: Update open_flags in raw_set_perm(), Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 07/14] block: Add bdrv_child_refresh_perms(), Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 11/14] block: Add *tighten_restrictions to *check*_perm(), Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 04/14] blockdev: blockdev_mark_auto_del: drop usage of bs->job, Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 13/14] iotests: Test failure to loosen restrictions, Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 12/14] block: Ignore loosening perm restrictions failures,
Kevin Wolf <=
- [Qemu-block] [PULL 10/14] block: Fix order in bdrv_replace_child(), Kevin Wolf, 2019/06/18
- [Qemu-block] [PULL 14/14] block/null: Expose read-zeroes option in QAPI schema, Kevin Wolf, 2019/06/18
- Re: [Qemu-block] [Qemu-devel] [PULL 00/14] Block layer patches, Peter Maydell, 2019/06/18