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

[Qemu-devel] [PATCH 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.


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 *loosen_restrictions to *check*_perm()
  block: Ignore loosening perm restrictions failures

 include/block/block_int.h |  15 ++++
 block.c                   | 143 ++++++++++++++++++++++++++++++++------
 block/commit.c            |   2 -
 block/file-posix.c        |   3 +
 block/mirror.c            |  32 ++++++---
 5 files changed, 161 insertions(+), 34 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 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>
---
 block/file-posix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 1cf4ee49eb..66b46ec0eb 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
@@ -2762,6 +2763,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 +2802,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

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

[Kevin Wolf]

Am 06.05.2019 um 21:47 hat Max Reitz geschrieben:
> 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>
> ---
>  block/file-posix.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 1cf4ee49eb..66b46ec0eb 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
> @@ -2762,6 +2763,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,

Adding some context before this hunk:

    if (s->reopen_state) {
        /* We already have a new file descriptor to set permissions for */
        assert(s->reopen_state->perm == perm);
        assert(s->reopen_state->shared_perm == shared);
        rs = s->reopen_state->opaque;
        s->perm_change_fd = rs->fd;
    } 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,
                                    false, errp);
        if (ret < 0) {
>              return ret;
>          } else if (ret != s->fd) {
>              s->perm_change_fd = ret;
> +            s->perm_change_flags = open_flags;
>          }
>      }

s->perm_change_flags is set in the else branch. According to the comment
in raw_set_perm(), not setting it in the then branch is actually correct
because .bdrv_reopen_commit will run first, so s->perm_change_flags
isn't accessed, but wouldn't it be nicer to have a valid value in it
anyway? Who knows where we'll add accesses later.

Kevin

> @@ -2800,6 +2802,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
> 

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

[Max Reitz]

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

On 08.05.19 15:06, Kevin Wolf wrote:
> Am 06.05.2019 um 21:47 hat Max Reitz geschrieben:
>> 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>
>> ---
>>  block/file-posix.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 1cf4ee49eb..66b46ec0eb 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
>> @@ -2762,6 +2763,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
> 
> Adding some context before this hunk:
> 
>     if (s->reopen_state) {
>         /* We already have a new file descriptor to set permissions for */
>         assert(s->reopen_state->perm == perm);
>         assert(s->reopen_state->shared_perm == shared);
>         rs = s->reopen_state->opaque;
>         s->perm_change_fd = rs->fd;
>     } 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,
>                                     false, errp);
>         if (ret < 0) {
>>              return ret;
>>          } else if (ret != s->fd) {
>>              s->perm_change_fd = ret;
>> +            s->perm_change_flags = open_flags;
>>          }
>>      }
> 
> s->perm_change_flags is set in the else branch. According to the comment
> in raw_set_perm(), not setting it in the then branch is actually correct
> because .bdrv_reopen_commit will run first, so s->perm_change_flags
> isn't accessed, but wouldn't it be nicer to have a valid value in it
> anyway? Who knows where we'll add accesses later.

Why not, it can’t hurt.

Max


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

[Qemu-devel] [PATCH 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>
---
 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 7dc8fe289a..fb6f0c48ae 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 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>
---
 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 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>
---
 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 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>
---
 block.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index fb6f0c48ae..21e4514426 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 6/7] block: Add *loosen_restrictions to *check*_perm()

[Max Reitz]

This patch makes three functions report whether the necessary permission
change purely loosens 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 | 81 +++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 65 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 21e4514426..105866d15a 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 *loosen_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,37 @@ 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 *loosen_restrictions to true if and only if no new permissions have
+ * to be taken and no existing shared permissions are to be unshared.  In this
+ * case, 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.
+ *
  * 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 *loosen_restrictions, Error **errp)
 {
     BlockDriver *drv = bs->drv;
     BdrvChild *c;
     int ret;
 
+    if (loosen_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;
+
+        *loosen_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 +1820,16 @@ 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_loosen_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);
+                                    ignore_children, &child_loosen_restr, errp);
+        if (loosen_restrictions) {
+            *loosen_restrictions &= child_loosen_restr;
+        }
         if (ret < 0) {
             return ret;
         }
@@ -1926,12 +1953,20 @@ 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.
  *
+ * Will set *loosen_restrictions to true if and only if no new permissions have
+ * to be taken and no existing shared permissions are to be unshared.  In this
+ * case, 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.
+ *
  * 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 *loosen_restrictions,
+                                  Error **errp)
 {
     BdrvChild *c;
     uint64_t cumulative_perms = new_used_perm;
@@ -1948,6 +1983,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 (loosen_restrictions) {
+                *loosen_restrictions = false;
+            }
+
             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 +1999,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 (loosen_restrictions) {
+                *loosen_restrictions = false;
+            }
+
             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 +2017,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, loosen_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 *loosen_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,
+                                 loosen_restrictions, errp);
     g_slist_free(ignore_children);
 
     if (ret < 0) {
@@ -2037,7 +2084,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 +2270,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 +2285,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 +3341,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 +3353,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 +4080,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 +4427,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 +5202,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 +5352,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

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

[Kevin Wolf]

Am 06.05.2019 um 21:47 hat Max Reitz geschrieben:
> This patch makes three functions report whether the necessary permission
> change purely loosens 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 | 81 +++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 21e4514426..105866d15a 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 *loosen_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,37 @@ 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 *loosen_restrictions to true if and only if no new permissions have
> + * to be taken and no existing shared permissions are to be unshared.  In this
> + * case, 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.
> + *
>   * 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 *loosen_restrictions, Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
>      BdrvChild *c;
>      int ret;
>  
> +    if (loosen_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;
> +
> +        *loosen_restrictions = !added_perms && !removed_shared_perms;
> +    }

(loosen_restrictions is a misnomer, just not changing permissions will
make it true, too)

>      /* 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))
    {
        error_setg(errp, "Block node is read-only");
        return -EPERM;
    }

This is an interesting case in the context of reopen. It could happen
that we're actually not taking any new permissions, but the node becomes
read-only in reopen, so we fail here while maintaining the old set of
options.

If this happens, we want the reopen operation to fail, so should we set
*loosen_restrictions = false here even though we're not literally taking
new permissions?

Hm, or actually, loosen_restrictions should always be NULL during
reopen, so it will never make a different. Maybe what we want then is
assert(!q || !loosen_restrictions) at the start of the function?

Kevin

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

[Max Reitz]

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

On 08.05.19 16:28, Kevin Wolf wrote:
> Am 06.05.2019 um 21:47 hat Max Reitz geschrieben:
>> This patch makes three functions report whether the necessary permission
>> change purely loosens 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 | 81 +++++++++++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 65 insertions(+), 16 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 21e4514426..105866d15a 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 *loosen_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,37 @@ 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 *loosen_restrictions to true if and only if no new permissions have
>> + * to be taken and no existing shared permissions are to be unshared.  In this
>> + * case, 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.
>> + *
>>   * 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 *loosen_restrictions, Error **errp)
>>  {
>>      BlockDriver *drv = bs->drv;
>>      BdrvChild *c;
>>      int ret;
>>  
>> +    if (loosen_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;
>> +
>> +        *loosen_restrictions = !added_perms && !removed_shared_perms;
>> +    }
> 
> (loosen_restrictions is a misnomer, just not changing permissions will
> make it true, too)

Naming things is hard. :-)

Should I name it tighten_restrictions and invert its value?

>>      /* 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))
>     {
>         error_setg(errp, "Block node is read-only");
>         return -EPERM;
>     }
> 
> This is an interesting case in the context of reopen. It could happen
> that we're actually not taking any new permissions, but the node becomes
> read-only in reopen, so we fail here while maintaining the old set of
> options.
> 
> If this happens, we want the reopen operation to fail, so should we set
> *loosen_restrictions = false here even though we're not literally taking
> new permissions?

Well, I had that at one point, yes.  I decided this case would always
imply that the restrictions are tightened somewhere, so I dropped it --
but I forgot about reopen, for some reason (even though it says “reopen”
right there).

> Hm, or actually, loosen_restrictions should always be NULL during
> reopen, so it will never make a different. Maybe what we want then is
> assert(!q || !loosen_restrictions) at the start of the function?

And add a note that we cannot return this information when reopening?
That sounds good to me.

Max


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

[Qemu-devel] [PATCH 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 105866d15a..42a0f3d305 100644
--- a/block.c
+++ b/block.c
@@ -2082,11 +2082,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 loosen_restrictions;
 
-    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, NULL, errp);
+    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL,
+                                &loosen_restrictions, &local_err);
     if (ret < 0) {
         bdrv_child_abort_perm_update(c);
+        if (loosen_restrictions) {
+            warn_reportf_err(local_err, "Failed to loosen restrictions: ");
+            ret = 0;
+        } else {
+            error_propagate(errp, local_err);
+        }
         return ret;
     }
 
@@ -2269,10 +2278,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 loosen_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,
+                              &loosen_restrictions, &local_err);
+        assert(loosen_restrictions == true);
+        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);
+        }
     }
 }
 
@@ -5316,7 +5335,9 @@ static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
 static int bdrv_inactivate_recurse(BlockDriverState *bs)
 {
     BdrvChild *child, *parent;
+    bool loosen_restrictions;
     uint64_t perm, shared_perm;
+    Error *local_err = NULL;
     int ret;
 
     if (!bs->drv) {
@@ -5352,8 +5373,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,
+                          &loosen_restrictions, &local_err);
+    assert(loosen_restrictions == true);
+    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 0/7] block: Ignore loosening perm restrictions failures

[Kevin Wolf]

Am 06.05.2019 um 21:47 hat Max Reitz geschrieben:
> 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.

There are some places in this series that were a bit confusing (just
because the whole permission mechanism is rather confusing). I suggested
improvements for two patches, and deleted the half-written mails for the
rest. After all, it does look correct to me, so whether you want to
address my comments or not:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>