[Qemu-devel] [PATCH v2 0/7] block: Ignore loosening perm restrictions failures

[Qemu-devel] [PATCH v2 0/7] block: Ignore loosening perm restrictions failures

[Max Reitz]

Hi,

This series is mainly a fix for
https://bugzilla.redhat.com/show_bug.cgi?id=1703793.  The problem
described there is that mirroring to a gluster volume, then switching
off the volume makes qemu crash.  There are two problems here:

(1) file-posix reopens the FD all the time because it thinks the FD it
    has is RDONLY.  It actually isn’t after the first reopen, we just
    forgot to change the internal flags.  That’s what patch 1 is for.

(2) Even then, when mirror completes, it drops its write permission on
    the FD.  This requires a reopen, which will fail if the volume is
    down.  Mirror doesn’t expect that.  Nobody ever expects that
    dropping permissions can fail, and rightfully so because that’s what
    I think we have generally agreed on.
    Therefore, the block layer should hide this error.  This is what the
    last two patches are for.

The last patch adds two assertions: bdrv_replace_child() (for the old
BDS) and bdrv_inactivate_recurse() assume they only ever drop
assertions.  This is now substantiated by these new assertions.
It turns out that this assumption was just plain wrong.  Patches 3 to 5
make it right.


v2:
- Patch 1: Set s->perm_change_flags for reopen, too [Kevin]
- Patch 6:
  - Rename loosen_restrictions to tighten_restrictions and kind of
    invert its meaning [Kevin]
  - Assert and document that we cannot return useful information about
    whether restrictions are loosened or tightened if the caller wants
    to reopen the node [Kevin]
- Patch 7: Handle loosen_restrictions -> tighten_restrictions fallout


git backport-diff output against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/7:[0001] [FC] 'file-posix: Update open_flags in raw_set_perm()'
002/7:[----] [--] 'block: Add bdrv_child_refresh_perms()'
003/7:[----] [--] 'block/mirror: Fix child permissions'
004/7:[----] [--] 'block/commit: Drop bdrv_child_try_set_perm()'
005/7:[0018] [FC] 'block: Fix order in bdrv_replace_child()'
       ^^^^ Confuses my v1 patch with 8aecf1d1bd250a, should be:
      [----] [--]
006/7:[down] 'block: Add *tighten_restrictions to *check*_perm()'
       ^^^^ Commit title has changed, but should be something like:
      [0061] [FC]
007/7:[0022] [FC] 'block: Ignore loosening perm restrictions failures'


Max Reitz (7):
  file-posix: Update open_flags in raw_set_perm()
  block: Add bdrv_child_refresh_perms()
  block/mirror: Fix child permissions
  block/commit: Drop bdrv_child_try_set_perm()
  block: Fix order in bdrv_replace_child()
  block: Add *tighten_restrictions to *check*_perm()
  block: Ignore loosening perm restrictions failures

 include/block/block_int.h |  15 ++++
 block.c                   | 151 ++++++++++++++++++++++++++++++++------
 block/commit.c            |   2 -
 block/file-posix.c        |   4 +
 block/mirror.c            |  32 +++++---
 5 files changed, 169 insertions(+), 35 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/7] file-posix: Update open_flags in raw_set_perm()

[Max Reitz]

raw_check_perm() + raw_set_perm() can change the flags associated with
the current FD.  If so, we have to update BDRVRawState.open_flags
accordingly.  Otherwise, we may keep reopening the FD even though the
current one already has the correct flags.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 1cf4ee49eb..3e859c4b9f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -145,6 +145,7 @@ typedef struct BDRVRawState {
     uint64_t locked_shared_perm;
 
     int perm_change_fd;
+    int perm_change_flags;
     BDRVReopenState *reopen_state;
 
 #ifdef CONFIG_XFS
@@ -2754,6 +2755,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
         assert(s->reopen_state->shared_perm == shared);
         rs = s->reopen_state->opaque;
         s->perm_change_fd = rs->fd;
+        s->perm_change_flags = rs->open_flags;
     } else {
         /* We may need a new fd if auto-read-only switches the mode */
         ret = raw_reconfigure_getfd(bs, bs->open_flags, &open_flags, perm,
@@ -2762,6 +2764,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
             return ret;
         } else if (ret != s->fd) {
             s->perm_change_fd = ret;
+            s->perm_change_flags = open_flags;
         }
     }
 
@@ -2800,6 +2803,7 @@ static void raw_set_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared)
     if (s->perm_change_fd && s->fd != s->perm_change_fd) {
         qemu_close(s->fd);
         s->fd = s->perm_change_fd;
+        s->open_flags = s->perm_change_flags;
     }
     s->perm_change_fd = 0;
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/7] block: Add bdrv_child_refresh_perms()

[Max Reitz]

If a block node uses bdrv_child_try_set_perm() to change the permission
it takes on its child, the result may be very short-lived.  If anything
makes the block layer recalculate the permissions internally, it will
invoke the node driver's .bdrv_child_perm() implementation.  The
permission/shared permissions masks that returns will then override the
values previously passed to bdrv_child_try_set_perm().

If drivers want a child edge to have specific values for the
permissions/shared permissions mask, it must return them in
.bdrv_child_perm().  Consequentially, there is no need for them to pass
the same values to bdrv_child_try_set_perm() then: It is better to have
a function that invokes .bdrv_child_perm() and calls
bdrv_child_try_set_perm() with the result.  This patch adds such a
function under the name of bdrv_child_refresh_perms().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h | 15 +++++++++++++++
 block.c                   | 12 ++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 94d45c9708..5522e58201 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1184,9 +1184,24 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   void *opaque, Error **errp);
 void bdrv_root_unref_child(BdrvChild *child);
 
+/**
+ * Sets a BdrvChild's permissions.  Avoid if the parent is a BDS; use
+ * bdrv_child_refresh_perms() instead and make the parent's
+ * .bdrv_child_perm() implementation return the correct values.
+ */
 int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
                             Error **errp);
 
+/**
+ * Calls bs->drv->bdrv_child_perm() and updates the child's permission
+ * masks with the result.
+ * Drivers should invoke this function whenever an event occurs that
+ * makes their .bdrv_child_perm() implementation return different
+ * values than before, but which will not result in the block layer
+ * automatically refreshing the permissions.
+ */
+int bdrv_child_refresh_perms(BlockDriverState *bs, BdrvChild *c, Error **errp);
+
 /* Default implementation for BlockDriver.bdrv_child_perm() that can be used by
  * block filters: Forward CONSISTENT_READ, WRITE, WRITE_UNCHANGED and RESIZE to
  * all children */
diff --git a/block.c b/block.c
index 7778e0dd89..be11504940 100644
--- a/block.c
+++ b/block.c
@@ -2048,6 +2048,18 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
     return 0;
 }
 
+int bdrv_child_refresh_perms(BlockDriverState *bs, BdrvChild *c, Error **errp)
+{
+    uint64_t parent_perms, parent_shared;
+    uint64_t perms, shared;
+
+    bdrv_get_cumulative_perm(bs, &parent_perms, &parent_shared);
+    bdrv_child_perm(bs, c->bs, c, c->role, NULL, parent_perms, parent_shared,
+                    &perms, &shared);
+
+    return bdrv_child_try_set_perm(c, perms, shared, errp);
+}
+
 void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c,
                                const BdrvChildRole *role,
                                BlockReopenQueue *reopen_queue,
-- 
2.20.1

[Qemu-devel] [PATCH v2 3/7] block/mirror: Fix child permissions

[Max Reitz]

We cannot use bdrv_child_try_set_perm() to give up all restrictions on
the child edge, and still have bdrv_mirror_top_child_perm() request
BLK_PERM_WRITE.  Fix this by making bdrv_mirror_top_child_perm() return
0/BLK_PERM_ALL when we want to give up all permissions, and replacing
bdrv_child_try_set_perm() by bdrv_child_refresh_perms().

The bdrv_child_try_set_perm() before removing the node with
bdrv_replace_node() is then unnecessary.  No permissions have changed
since the previous invocation of bdrv_child_try_set_perm().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index ff15cfb197..e15adce98e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -85,6 +85,7 @@ typedef struct MirrorBlockJob {
 
 typedef struct MirrorBDSOpaque {
     MirrorBlockJob *job;
+    bool stop;
 } MirrorBDSOpaque;
 
 struct MirrorOp {
@@ -656,8 +657,9 @@ static int mirror_exit_common(Job *job)
 
     /* We don't access the source any more. Dropping any WRITE/RESIZE is
      * required before it could become a backing file of target_bs. */
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
+    bs_opaque->stop = true;
+    bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
+                             &error_abort);
     if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
         BlockDriverState *backing = s->is_none_mode ? src : s->base;
         if (backing_bs(target_bs) != backing) {
@@ -704,13 +706,12 @@ static int mirror_exit_common(Job *job)
     g_free(s->replaces);
     bdrv_unref(target_bs);
 
-    /* Remove the mirror filter driver from the graph. Before this, get rid of
+    /*
+     * Remove the mirror filter driver from the graph. Before this, get rid of
      * the blockers on the intermediate nodes so that the resulting state is
-     * valid. Also give up permissions on mirror_top_bs->backing, which might
-     * block the removal. */
+     * valid.
+     */
     block_job_remove_all_bdrv(bjob);
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     /* We just changed the BDS the job BB refers to (with either or both of the
@@ -1468,6 +1469,18 @@ static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c,
                                        uint64_t perm, uint64_t shared,
                                        uint64_t *nperm, uint64_t *nshared)
 {
+    MirrorBDSOpaque *s = bs->opaque;
+
+    if (s->stop) {
+        /*
+         * If the job is to be stopped, we do not need to forward
+         * anything to the real image.
+         */
+        *nperm = 0;
+        *nshared = BLK_PERM_ALL;
+        return;
+    }
+
     /* Must be able to forward guest writes to the real image */
     *nperm = 0;
     if (perm & BLK_PERM_WRITE) {
@@ -1687,8 +1700,9 @@ fail:
         job_early_fail(&s->common.job);
     }
 
-    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
+    bs_opaque->stop = true;
+    bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
+                             &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     bdrv_unref(mirror_top_bs);
-- 
2.20.1

[Qemu-devel] [PATCH v2 4/7] block/commit: Drop bdrv_child_try_set_perm()

[Max Reitz]

commit_top_bs never requests or unshares any permissions.  There is no
reason to make this so explicit here.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 14e5bb394c..44b3083b84 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -110,8 +110,6 @@ static void commit_abort(Job *job)
      * XXX Can (or should) we somehow keep 'consistent read' blocked even
      * after the failed/cancelled commit job is gone? If we already wrote
      * something to base, the intermediate images aren't valid any more. */
-    bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
-                            &error_abort);
     bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
                       &error_abort);
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 5/7] block: Fix order in bdrv_replace_child()

[Max Reitz]

We have to start by applying the permission restrictions to new_bs
before we can loosen them on old_bs.  See the comment for the
explanation.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index be11504940..da7da8cc6c 100644
--- a/block.c
+++ b/block.c
@@ -2205,6 +2205,19 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
 
     bdrv_replace_child_noperm(child, new_bs);
 
+    /*
+     * Start with the new node's permissions.  If @new_bs is a (direct
+     * or indirect) child of @old_bs, we must complete the permission
+     * update on @new_bs before we loosen the restrictions on @old_bs.
+     * Otherwise, bdrv_check_perm() on @old_bs would re-initiate
+     * updating the permissions of @new_bs, and thus not purely loosen
+     * restrictions.
+     */
+    if (new_bs) {
+        bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
+        bdrv_set_perm(new_bs, perm, shared_perm);
+    }
+
     if (old_bs) {
         /* Update permissions for old node. This is guaranteed to succeed
          * because we're just taking a parent away, so we're loosening
@@ -2213,11 +2226,6 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
         bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL, &error_abort);
         bdrv_set_perm(old_bs, perm, shared_perm);
     }
-
-    if (new_bs) {
-        bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
-        bdrv_set_perm(new_bs, perm, shared_perm);
-    }
 }
 
 BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
-- 
2.20.1

[Qemu-devel] [PATCH v2 6/7] block: Add *tighten_restrictions to *check*_perm()

[Max Reitz]

This patch makes three functions report whether the necessary permission
change tightens restrictions or not.  These functions are:
- bdrv_check_perm()
- bdrv_check_update_perm()
- bdrv_child_check_perm()

Callers can use this result to decide whether a failure is fatal or not
(see the next patch).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 72 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index da7da8cc6c..8f517be5b4 100644
--- a/block.c
+++ b/block.c
@@ -1686,9 +1686,12 @@ static int bdrv_fill_options(QDict **options, const char *filename,
 
 static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
                                  uint64_t perm, uint64_t shared,
-                                 GSList *ignore_children, Error **errp);
+                                 GSList *ignore_children,
+                                 bool *tighten_restrictions, Error **errp);
 static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
+static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
+                                     uint64_t *shared_perm);
 
 typedef struct BlockReopenQueueEntry {
      bool prepared;
@@ -1759,18 +1762,43 @@ static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
  * permissions of all its parents. This involves checking whether all necessary
  * permission changes to child nodes can be performed.
  *
+ * Will set *tighten_restrictions to true if and only if new permissions have to
+ * be taken or currently shared permissions are to be unshared.  Otherwise,
+ * errors are not fatal as long as the caller accepts that the restrictions
+ * remain tighter than they need to be.  The caller still has to abort the
+ * transaction.
+ * @tighten_restrictions cannot be used together with @q: When reopening, we may
+ * encounter fatal errors even though no restrictions are to be tightened.  For
+ * example, changing a node from RW to RO will fail if the WRITE permission is
+ * to be kept.
+ *
  * A call to this function must always be followed by a call to bdrv_set_perm()
  * or bdrv_abort_perm_update().
  */
 static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
                            uint64_t cumulative_perms,
                            uint64_t cumulative_shared_perms,
-                           GSList *ignore_children, Error **errp)
+                           GSList *ignore_children,
+                           bool *tighten_restrictions, Error **errp)
 {
     BlockDriver *drv = bs->drv;
     BdrvChild *c;
     int ret;
 
+    assert(!q || !tighten_restrictions);
+
+    if (tighten_restrictions) {
+        uint64_t current_perms, current_shared;
+        uint64_t added_perms, removed_shared_perms;
+
+        bdrv_get_cumulative_perm(bs, &current_perms, &current_shared);
+
+        added_perms = cumulative_perms & ~current_perms;
+        removed_shared_perms = current_shared & ~cumulative_shared_perms;
+
+        *tighten_restrictions = added_perms || removed_shared_perms;
+    }
+
     /* Write permissions never work with read-only images */
     if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
         !bdrv_is_writable_after_reopen(bs, q))
@@ -1798,11 +1826,18 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
     /* Check all children */
     QLIST_FOREACH(c, &bs->children, next) {
         uint64_t cur_perm, cur_shared;
+        bool child_tighten_restr;
+
         bdrv_child_perm(bs, c->bs, c, c->role, q,
                         cumulative_perms, cumulative_shared_perms,
                         &cur_perm, &cur_shared);
-        ret = bdrv_child_check_perm(c, q, cur_perm, cur_shared,
-                                    ignore_children, errp);
+        ret = bdrv_child_check_perm(c, q, cur_perm, cur_shared, ignore_children,
+                                    tighten_restrictions ? &child_tighten_restr
+                                                         : NULL,
+                                    errp);
+        if (tighten_restrictions) {
+            *tighten_restrictions |= child_tighten_restr;
+        }
         if (ret < 0) {
             return ret;
         }
@@ -1926,17 +1961,23 @@ char *bdrv_perm_names(uint64_t perm)
  * set, the BdrvChild objects in this list are ignored in the calculations;
  * this allows checking permission updates for an existing reference.
  *
+ * See bdrv_check_perm() for the semantics of @tighten_restrictions.
+ *
  * Needs to be followed by a call to either bdrv_set_perm() or
  * bdrv_abort_perm_update(). */
 static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
                                   uint64_t new_used_perm,
                                   uint64_t new_shared_perm,
-                                  GSList *ignore_children, Error **errp)
+                                  GSList *ignore_children,
+                                  bool *tighten_restrictions,
+                                  Error **errp)
 {
     BdrvChild *c;
     uint64_t cumulative_perms = new_used_perm;
     uint64_t cumulative_shared_perms = new_shared_perm;
 
+    assert(!q || !tighten_restrictions);
+
     /* There is no reason why anyone couldn't tolerate write_unchanged */
     assert(new_shared_perm & BLK_PERM_WRITE_UNCHANGED);
 
@@ -1948,6 +1989,11 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
         if ((new_used_perm & c->shared_perm) != new_used_perm) {
             char *user = bdrv_child_user_desc(c);
             char *perm_names = bdrv_perm_names(new_used_perm & ~c->shared_perm);
+
+            if (tighten_restrictions) {
+                *tighten_restrictions = true;
+            }
+
             error_setg(errp, "Conflicts with use by %s as '%s', which does not "
                              "allow '%s' on %s",
                        user, c->name, perm_names, bdrv_get_node_name(c->bs));
@@ -1959,6 +2005,11 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
         if ((c->perm & new_shared_perm) != c->perm) {
             char *user = bdrv_child_user_desc(c);
             char *perm_names = bdrv_perm_names(c->perm & ~new_shared_perm);
+
+            if (tighten_restrictions) {
+                *tighten_restrictions = true;
+            }
+
             error_setg(errp, "Conflicts with use by %s as '%s', which uses "
                              "'%s' on %s",
                        user, c->name, perm_names, bdrv_get_node_name(c->bs));
@@ -1972,19 +2023,21 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
     }
 
     return bdrv_check_perm(bs, q, cumulative_perms, cumulative_shared_perms,
-                           ignore_children, errp);
+                           ignore_children, tighten_restrictions, errp);
 }
 
 /* Needs to be followed by a call to either bdrv_child_set_perm() or
  * bdrv_child_abort_perm_update(). */
 static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
                                  uint64_t perm, uint64_t shared,
-                                 GSList *ignore_children, Error **errp)
+                                 GSList *ignore_children,
+                                 bool *tighten_restrictions, Error **errp)
 {
     int ret;
 
     ignore_children = g_slist_prepend(g_slist_copy(ignore_children), c);
-    ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children, errp);
+    ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children,
+                                 tighten_restrictions, errp);
     g_slist_free(ignore_children);
 
     if (ret < 0) {
@@ -2037,7 +2090,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
 {
     int ret;
 
-    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, errp);
+    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, NULL, errp);
     if (ret < 0) {
         bdrv_child_abort_perm_update(c);
         return ret;
@@ -2223,7 +2276,8 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
          * because we're just taking a parent away, so we're loosening
          * restrictions. */
         bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
-        bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL, &error_abort);
+        bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
+                        NULL, &error_abort);
         bdrv_set_perm(old_bs, perm, shared_perm);
     }
 }
@@ -2237,7 +2291,8 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
     BdrvChild *child;
     int ret;
 
-    ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp);
+    ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, NULL,
+                                 errp);
     if (ret < 0) {
         bdrv_abort_perm_update(child_bs);
         return NULL;
@@ -3292,7 +3347,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
     QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
         BDRVReopenState *state = &bs_entry->state;
         ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
-                              state->shared_perm, NULL, errp);
+                              state->shared_perm, NULL, NULL, errp);
         if (ret < 0) {
             goto cleanup_perm;
         }
@@ -3304,7 +3359,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
                             state->perm, state->shared_perm,
                             &nperm, &nshared);
             ret = bdrv_check_update_perm(state->new_backing_bs, NULL,
-                                         nperm, nshared, NULL, errp);
+                                         nperm, nshared, NULL, NULL, errp);
             if (ret < 0) {
                 goto cleanup_perm;
             }
@@ -4031,7 +4086,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
 
     /* Check whether the required permissions can be granted on @to, ignoring
      * all BdrvChild in @list so that they can't block themselves. */
-    ret = bdrv_check_update_perm(to, NULL, perm, shared, list, errp);
+    ret = bdrv_check_update_perm(to, NULL, perm, shared, list, NULL, errp);
     if (ret < 0) {
         bdrv_abort_perm_update(to);
         goto out;
@@ -4378,7 +4433,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
         /* Check whether we are allowed to switch c from top to base */
         GSList *ignore_children = g_slist_prepend(NULL, c);
         ret = bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
-                                     ignore_children, &local_err);
+                                     ignore_children, NULL, &local_err);
         g_slist_free(ignore_children);
         if (ret < 0) {
             error_report_err(local_err);
@@ -5153,7 +5208,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
      */
     bs->open_flags &= ~BDRV_O_INACTIVE;
     bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &local_err);
+    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &local_err);
     if (ret < 0) {
         bs->open_flags |= BDRV_O_INACTIVE;
         error_propagate(errp, local_err);
@@ -5303,7 +5358,7 @@ 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, &error_abort);
+    bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &error_abort);
     bdrv_set_perm(bs, perm, shared_perm);
 
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 7/7] block: Ignore loosening perm restrictions failures

[Max Reitz]

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, report
the error, and 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 <mreitz@redhat.com>
---
 block.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 8f517be5b4..a5a03906d0 100644
--- a/block.c
+++ b/block.c
@@ -2088,11 +2088,20 @@ 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 {
+            warn_reportf_err(local_err, "Failed to loosen restrictions: ");
+            ret = 0;
+        }
         return ret;
     }
 
@@ -2275,10 +2284,20 @@ 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;
+        Error *local_err = NULL;
+        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, &local_err);
+        assert(tighten_restrictions == false);
+        if (ret < 0) {
+            warn_reportf_err(local_err, "Failed to loosen restrictions: ");
+            bdrv_abort_perm_update(old_bs);
+        } else {
+            bdrv_set_perm(old_bs, perm, shared_perm);
+        }
     }
 }
 
@@ -5322,7 +5341,9 @@ 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;
+    Error *local_err = NULL;
     int ret;
 
     if (!bs->drv) {
@@ -5358,8 +5379,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, &local_err);
+    assert(tighten_restrictions == false);
+    if (ret < 0) {
+        warn_reportf_err(local_err, "Failed to loosen restrictions: ");
+        bdrv_abort_perm_update(bs);
+    } else {
+        bdrv_set_perm(bs, perm, shared_perm);
+    }
 
 
     /* Recursively inactivate children */
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 7/7] block: Ignore loosening perm restrictions failures

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 6050 bytes --]

On 08.05.19 20:25, Max Reitz wrote:
> 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, report
> the error, and report success to the caller.

Hm, I just noticed that while make check passes, it emits two of these
warnings:

TEST: tests/i440fx-test... (pid=23021)
qemu-system-x86_64: warning: Failed to loosen restrictions: Could not
reopen file: No such file or directory
qemu-system-x86_64: warning: Failed to loosen restrictions: Could not
reopen file: No such file or directory
PASS: tests/i440fx-test

This is because the test creates temporary files which it unlinks after
qemu has opened them.  The bdrv_close_all() then attempts to drop the
WRITE permission, which requires reopening the file, which fails.

(Reproducer:

$ touch /tmp/foo; x86_64-softmmu/qemu-system-x86_64 -hda /tmp/foo &; \
  sleep 1; rm /tmp/foo; kill %1
[1] 23236
WARNING: Image format was not specified for '/tmp/foo' and probing
guessed raw.
         Automatically detecting the format is dangerous for raw images,
write operations on block 0 will be restricted.
         Specify the 'raw' format explicitly to remove the restrictions.
qemu-system-x86_64: terminating on signal 15 from pid 7922 (-zsh)



qemu-system-x86_64: warning: Failed to loosen restrictions: Could not
reopen file: No such file or directory
qemu-system-x86_64: warning: Failed to loosen restrictions: Could not
reopen file: No such file or directory
[1]  + 23236 done       x86_64-softmmu/qemu-system-x86_64 -hda /tmp/foo

)

Should I just drop the warning?

Max

> 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 <mreitz@redhat.com>
> ---
>  block.c | 40 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 8f517be5b4..a5a03906d0 100644
> --- a/block.c
> +++ b/block.c
> @@ -2088,11 +2088,20 @@ 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 {
> +            warn_reportf_err(local_err, "Failed to loosen restrictions: ");
> +            ret = 0;
> +        }
>          return ret;
>      }
>  
> @@ -2275,10 +2284,20 @@ 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;
> +        Error *local_err = NULL;
> +        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, &local_err);
> +        assert(tighten_restrictions == false);
> +        if (ret < 0) {
> +            warn_reportf_err(local_err, "Failed to loosen restrictions: ");
> +            bdrv_abort_perm_update(old_bs);
> +        } else {
> +            bdrv_set_perm(old_bs, perm, shared_perm);
> +        }
>      }
>  }
>  
> @@ -5322,7 +5341,9 @@ 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;
> +    Error *local_err = NULL;
>      int ret;
>  
>      if (!bs->drv) {
> @@ -5358,8 +5379,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, &local_err);
> +    assert(tighten_restrictions == false);
> +    if (ret < 0) {
> +        warn_reportf_err(local_err, "Failed to loosen restrictions: ");
> +        bdrv_abort_perm_update(bs);
> +    } else {
> +        bdrv_set_perm(bs, perm, shared_perm);
> +    }
>  
>  
>      /* Recursively inactivate children */
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]