block: Fix permissions after ro/rw reopen

qemu-io provides a 'reopen' command that allows switching from writable to read-only access. We need to make sure that we don't try to keep write permissions to a BlockBackend that becomes read-only, otherwise things are going to fail.

command() already makes sure to request any additional permissions that a qemu-io command requires, so just resetting the permissions to values that are safe for read-only images is enough to fix this.

As a side effect, this makes the output of qemu-iotests case 187 more consistent.

Signed-off-by Kevin Wolf <[email protected]>

---
 qemu-io.c                  | 13 +++++++++++++
 tests/qemu-iotests/187.out |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

qemu-io provides a 'reopen' command that allows switching from writable to read-only access. We need to make sure that we don't try to keep write permissions to a BlockBackend that becomes read-only, otherwise things are going to fail.

command() already makes sure to request any additional permissions that a qemu-io command requires, so just resetting the permissions to values that are safe for read-only images is enough to fix this.

As a side effect, this makes the output of qemu-iotests case 187 more consistent.

Signed-off-by Kevin Wolf <[email protected]>

---
 qemu-io.c                  | 13 +++++++++++++
 tests/qemu-iotests/187.out |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

qemu-io.c +13 -0

index 265445ad89..9e031f0f8e 100644
@@ -366,6 +366,12 @@ static void command_loop(void)
     char *input;
 
     for (i = 0; !done && i < ncmdline; i++) {
+        /* Make sure that we start each command with clean permissions and only
+         * add whatever the specific cmdinfo_t describes */
+        if (qemuio_blk) {
+            blk_set_perm(qemuio_blk, BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL,
+                         &error_abort);
+        }
         done = qemuio_command(qemuio_blk, cmdline[i]);
     }
     if (cmdline) {

@@ -391,6 +397,13 @@ static void command_loop(void)
         if (input == NULL) {
             break;
         }
+
+        /* Make sure that we start each command with clean permissions and only
+         * add whatever the specific cmdinfo_t describes */
+        if (qemuio_blk) {
+            blk_set_perm(qemuio_blk, BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL,
+                         &error_abort);
+        }
         done = qemuio_command(qemuio_blk, input);
         g_free(input);
 

tests/qemu-iotests/187.out +1 -1

index 68fb944cd5..30b987f71f 100644
@@ -12,7 +12,7 @@ Start from read-write
 
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-write failed: Operation not permitted
+Block node is read-only
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done

Signature · 1 line

2.13.5

qemu-io provides a 'reopen' command that allows switching from writable to read-only access. We need to make sure that we don't try to keep write permissions to a BlockBackend that becomes read-only, otherwise things are going to fail.

command() already makes sure to request any additional permissions that a qemu-io command requires, so just resetting the permissions to values that are safe for read-only images is enough to fix this.

As a side effect, this makes the output of qemu-iotests case 187 more consistent.

Signed-off-by Kevin Wolf <[email protected]>

Am 15.09.2017 um 12:10 hat Kevin Wolf geschrieben:

Quoted 9 lines

qemu-io provides a 'reopen' command that allows switching from writable to read-only access. We need to make sure that we don't try to keep write permissions to a BlockBackend that becomes read-only, otherwise things are going to fail.

command() already makes sure to request any additional permissions that a qemu-io command requires, so just resetting the permissions to values that are safe for read-only images is enough to fix this.

Quoted 4 lines

As a side effect, this makes the output of qemu-iotests case 187 more consistent.

Signed-off-by Kevin Wolf <[email protected]>

This seems to break qemu-iotests 077 and 081 for raw. I'll look into it tomorrow.

qemu-io provides a 'reopen' command that allows switching from writable to read-only access. We need to make sure that we don't try to keep write permissions to a BlockBackend that becomes read-only, otherwise things are going to fail.

This requires a bdrv_drain() call because otherwise in-flight AIO write requests could issue new internal requests while the permission has already gone away, which would cause assertion failures. Draining the queue doesn't break AIO requests in any new way, bdrv_reopen() would drain it anyway only a few lines later.

Signed-off-by Kevin Wolf <[email protected]>

---
 qemu-io-cmds.c             | 12 ++++++++++++
 tests/qemu-iotests/187.out |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

qemu-io-cmds.c +12 -0

index 2811a89099..3727fb43f3 100644
@@ -2010,6 +2010,18 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
         return 0;
     }
 
+    if (!(flags & BDRV_O_RDWR)) {
+        uint64_t orig_perm, orig_shared_perm;
+
+        bdrv_drain(bs);
+
+        blk_get_perm(blk, &orig_perm, &orig_shared_perm);
+        blk_set_perm(blk,
+                     orig_perm & ~(BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED),
+                     orig_shared_perm,
+                     &error_abort);
+    }
+
     qopts = qemu_opts_find(&reopen_opts, NULL);
     opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
     qemu_opts_reset(&reopen_opts);

tests/qemu-iotests/187.out +1 -1

index 68fb944cd5..30b987f71f 100644
@@ -12,7 +12,7 @@ Start from read-write
 
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-write failed: Operation not permitted
+Block node is read-only
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done

Signature · 1 line

2.13.5

In the context of bdrv_reopen(), we'll have to look at the state of the graph as it will be after the reopen. This interface addition is in preparation for the change.

Signed-off-by Kevin Wolf <[email protected]>

---
 include/block/block_int.h |  7 +++++++
 block.c                   | 19 ++++++++++++-------
 block/commit.c            |  1 +
 block/mirror.c            |  1 +
 block/replication.c       |  1 +
 block/vvfat.c             |  1 +
 6 files changed, 23 insertions(+), 7 deletions(-)

In the context of bdrv_reopen(), we'll have to look at the state of the graph as it will be after the reopen. This interface addition is in preparation for the change.

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

When new permissions are calculated during bdrv_reopen(), they need to be based on the state of the graph as it will be after the reopen has completed, not on the current state of the involved nodes.

This patch makes bdrv_is_writable() optionally accept a BlockReopenQueue from which the new flags are taken. This is then used for determining the new bs->file permissions of format drivers as soon as we add the code to actually pass a non-NULL reopen queue to the .bdrv_child_perm callbacks.

While moving bdrv_is_writable(), make it static. It isn't used outside block.c.

Signed-off-by Kevin Wolf <[email protected]>

---
 include/block/block.h |  1 -
 block.c               | 52 ++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 37 insertions(+), 16 deletions(-)

+ * Return the flags that @bs will have after the reopens in @q have
+ * successfully completed. If @q is NULL (or @bs is not contained in @q),
+ * return the current flags.
+ */

We will calculate the required new permissions in the prepare stage of a reopen. Required permissions of children can be influenced by the changes made to their parents, but parents are independent from their children. This means that permissions need to be calculated top-down. In order to achieve this, queue parents before their children rather than queuing the children first.

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

If we switch between read-only and read-write, the permissions that image format drivers need on bs->file change, too. Make sure to update the permissions during bdrv_reopen().

Signed-off-by Kevin Wolf <[email protected]>

---
 include/block/block.h |  1 +
 block.c               | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

block.c +2 -0

+static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
+                                                          BdrvChild *c)

Quoted 10 lines

If we switch between read-only and read-write, the permissions that image format drivers need on bs->file change, too. Make sure to update the permissions during bdrv_reopen().

Signed-off-by Kevin Wolf <[email protected]>

---
 include/block/block.h |  1 +
 block.c               | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

block.c int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp) · line 2905

Quoted 12 lines

block.c +12 -0

+static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
+                                                          BdrvChild *c)
+{
+    BlockReopenQueueEntry *entry;
+
+    QSIMPLEQ_FOREACH(entry, q, entry) {
+        BlockDriverState *bs = entry->state.bs;
+        BdrvChild *child;
+
+        QLIST_FOREACH(child, &bs->children, next) {
+            if (child == c) {
+                return entry;

An O(n^2) loop. Is it going to bite us at any point in the future, or are we generally dealing with a small enough queue size and BDS graph to not worry about it?

If we switch between read-only and read-write, the permissions that image format drivers need on bs->file change, too. Make sure to update the permissions during bdrv_reopen().

Signed-off-by Kevin Wolf <[email protected]>

---
 include/block/block.h |  1 +
 block.c               | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

include/block/block.h +1 -0

index 082eb2cd9c..3c3af462e4 100644
@@ -166,6 +166,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
 typedef struct BDRVReopenState {
     BlockDriverState *bs;
     int flags;
+    uint64_t perm, shared_perm;
     QDict *options;
     QDict *explicit_options;
     void *opaque;

block.c +0 -0

index 204cbb46c7..5c65fac672 100644

block.c +62 -0

+    bs_entry->state.shared_perm = 0;
+
     QLIST_FOREACH(child, &bs->children, next) {
         QDict *new_child_options;
         char *child_key_dot;

@@ -2887,6 +2891,52 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
     return ret;
 }
 
+static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
+                                                          BdrvChild *c)
+{
+    BlockReopenQueueEntry *entry;
+
+    QSIMPLEQ_FOREACH(entry, q, entry) {
+        BlockDriverState *bs = entry->state.bs;
+        BdrvChild *child;
+
+        QLIST_FOREACH(child, &bs->children, next) {
+            if (child == c) {
+                return entry;
+            }
+        }
+    }
+
+    return NULL;
+}
+
+static void bdrv_reopen_perm(BlockReopenQueue *q, BlockDriverState *bs,
+                             uint64_t *perm, uint64_t *shared)
+{
+    BdrvChild *c;
+    BlockReopenQueueEntry *parent;
+    uint64_t cumulative_perms = 0;
+    uint64_t cumulative_shared_perms = BLK_PERM_ALL;
+
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        parent = find_parent_in_reopen_queue(q, c);
+        if (!parent) {
+            cumulative_perms |= c->perm;
+            cumulative_shared_perms &= c->shared_perm;
+        } else {
+            uint64_t nperm, nshared;
+
+            bdrv_child_perm(parent->state.bs, bs, c, c->role, q,
+                            parent->state.perm, parent->state.shared_perm,
+                            &nperm, &nshared);
+
+            cumulative_perms |= nperm;
+            cumulative_shared_perms &= nshared;
+        }
+    }
+    *perm = cumulative_perms;
+    *shared = cumulative_shared_perms;
+}
 
 /*
  * Prepares a BlockDriverState for reopen. All changes are staged in the

@@ -2952,6 +3002,9 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         goto error;
     }
 
+    /* Calculate required permissions after reopening */
+    bdrv_reopen_perm(queue, reopen_state->bs,
+                     &reopen_state->perm, &reopen_state->shared_perm);
 
     ret = bdrv_flush(reopen_state->bs);
     if (ret) {

@@ -3007,6 +3060,12 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         } while ((entry = qdict_next(reopen_state->options, entry)));
     }
 
+    ret = bdrv_check_perm(reopen_state->bs, queue, reopen_state->perm,
+                          reopen_state->shared_perm, NULL, errp);
+    if (ret < 0) {
+        goto error;
+    }
+
     ret = 0;
 
 error:

@@ -3047,6 +3106,9 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 
     bdrv_refresh_limits(bs, NULL);
 
+    bdrv_set_perm(reopen_state->bs, reopen_state->perm,
+                  reopen_state->shared_perm);
+
     new_can_write =
         !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
     if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {

@@ -3080,6 +3142,8 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
     }
 
     QDECREF(reopen_state->explicit_options);
+
+    bdrv_abort_perm_update(reopen_state->bs);
 }
 
 

Signature · 1 line

2.13.5

Quoted 30 lines

If we switch between read-only and read-write, the permissions that image format drivers need on bs->file change, too. Make sure to update the permissions during bdrv_reopen().

Signed-off-by Kevin Wolf <[email protected]>

---
 include/block/block.h |  1 +
 block.c               | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

include/block/block.h +1 -0

index 082eb2cd9c..3c3af462e4 100644
@@ -166,6 +166,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
 typedef struct BDRVReopenState {
     BlockDriverState *bs;
     int flags;
+    uint64_t perm, shared_perm;
     QDict *options;
     QDict *explicit_options;
     void *opaque;

block.c +0 -0

index 204cbb46c7..5c65fac672 100644
@@ -2781,6 +2781,10 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     bs_entry->state.explicit_options = explicit_options;
     bs_entry->state.flags = flags;
 

block.c static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, · line 2785

Quoted 2 lines

block.c +2 -0

+    /* This needs to be overwritten in bdrv_reopen_prepare() */
+    bs_entry->state.perm = UINT64_MAX;

Probably doesn't matter because as the comment says it will be overwritten soon, but is BLK_PERM_ALL more appropriate?

This involves a temporary read-write reopen if the backing file link in the middle of a backing file chain should be changed and is therefore a good test for the latest bdrv_reopen() vs. op blockers fixes.

Signed-off-by Kevin Wolf <[email protected]>

---

I actually managed to find a simple case that reproduces the bug that is fixed in this series, but outside of my commit job improvements that originally led me to this work.

 tests/qemu-iotests/195     | 92 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/195.out | 78 +++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +

tests/qemu-iotests/195 new file +5 -0

+{"execute":"quit"}
+EOF
+
+TEST_IMG="$TEST_IMG.mid" _img_info
+

bdrv_reopen() can switch nodes between read-only and read-write modes. This has implications for the required permissions on their child nodes. For example, a qcow2 node requests write permissions on bs->file only if it is writable itself.

This means that during bdrv_reopen(), the permissions need to be recalculated in order to prevent failures where the bs->file permissions don't match its actual read-only state (e.g. bs->file is a read-write node, but the permission still enforces read-only access).

Kevin Wolf (6):
qemu-io: Reset qemuio_blk permissions before each command
block: Add reopen_queue to bdrv_child_perm()
block: Add reopen queue to bdrv_check_perm()
block: Base permissions on rw state after reopen
block: reopen: Queue children after their parents
block: Fix permissions after bdrv_reopen()

 include/block/block.h      |   2 +-
 include/block/block_int.h  |   7 ++
 block.c                    | 191 +++++++++++++++++++++++++++++++++------------
 block/commit.c             |   1 +
 block/mirror.c             |   1 +
 block/replication.c        |   1 +
 block/vvfat.c              |   1 +
 qemu-io.c                  |  13 +++
 tests/qemu-iotests/187.out |   2 +-
 9 files changed, 169 insertions(+), 50 deletions(-)

Signature · 1 line

2.13.5

Quoted 5 lines

bdrv_reopen() can switch nodes between read-only and read-write modes. This has implications for the required permissions on their child nodes. For example, a qcow2 node requests write permissions on bs->file only if it is writable itself.

Quoted 4 lines

This means that during bdrv_reopen(), the permissions need to be recalculated in order to prevent failures where the bs->file permissions don't match its actual read-only state (e.g. bs->file is a read-write node, but the permission still enforces read-only access).

Passing reopen_queue around makes the interface and implementations complicated.

I wonder if any of the alternatives make sense:

1) Don't pass reopen_queue as a parameter, just pass the one interesting BDRVReopenState pointer. So that callees don't need to call bdrv_reopen_get_flags().

2) Don't change the prototypes at all, just change .bdrv_reopen_prepare contract so that after it returns, .bdrv_child_perm/.bdrv_check_perm should comply to the new state that would be commited once .bdrv_reopen_commit() is called, or reverted if .bdrv_reopen_abort().

3) Don't change the prototypes at all, track the reopen progress in block.c generically, (e.g. ignore conflicts and voilations) and update the permissions only after bdrv_reopen_commit().

Quoted 4 lines

2) Don't change the prototypes at all, just change .bdrv_reopen_prepare contract so that after it returns, .bdrv_child_perm/.bdrv_check_perm should comply to the new state that would be commited once .bdrv_reopen_commit() is called, or reverted if .bdrv_reopen_abort().

Hm, .bdrv_reopen_prepare already gets the whole queue passed, so I guess this could technically work. I'm not sure if it is a good idea, though.

Such a change would still make .bdrv_child_perm depend on the reopen queue, just without actually passing it as a parameter. I like such hidden data flows even less than adding an explicit one.

It would also mean that each block driver would have to save the queue in its local bs->opaque structure so that .bdrv_child_perm can access it later. Alternatively, bdrv_reopen_prepare could already store the new cumulative parent permissions, but it would still involve two new fields in bs->opaque for storing something of a rather temporary nature.

Quoted 12 lines

Quoted 4 lines

2) Don't change the prototypes at all, just change .bdrv_reopen_prepare contract so that after it returns, .bdrv_child_perm/.bdrv_check_perm should comply to the new state that would be commited once .bdrv_reopen_commit() is called, or reverted if .bdrv_reopen_abort().

Hm, .bdrv_reopen_prepare already gets the whole queue passed, so I guess this could technically work. I'm not sure if it is a good idea, though.

Such a change would still make .bdrv_child_perm depend on the reopen queue, just without actually passing it as a parameter. I like such hidden data flows even less than adding an explicit one.

Quoted 5 lines

It would also mean that each block driver would have to save the queue in its local bs->opaque structure so that .bdrv_child_perm can access it later. Alternatively, bdrv_reopen_prepare could already store the new cumulative parent permissions, but it would still involve two new fields in bs->opaque for storing something of a rather temporary nature.

What about this?

1) drv->bdrv_reopen_prepare() saves the desired new perms in BDRVReopenState.

2) bdrv_reopen_prepare() checks the new perms after drv->bdrv_reopen_prepare() returns.
3) bdrv_reopen_commit() updates the bs to new perms.

But how does it determine the desired new perms? Either you duplicate the logic of the .bdrv_child_perm implementation into a new set of functions that does the same thing, but based on the new state; or you extend the existing function with a BlockReopenQueue parameter. The latter is basically this series, except with an additional unnecessary detour through the driver code instead of doing it in common code.

Also note that storing it in BDRVReopenState would have to involve a new list of an additional data structure because permissions are per BdrvChild, not per BlockDriverState.

bdrv_reopen() can switch nodes between read-only and read-write modes. This has implications for the required permissions on their child nodes. For example, a qcow2 node requests write permissions on bs->file only if it is writable itself.

This means that during bdrv_reopen(), the permissions need to be recalculated in order to prevent failures where the bs->file permissions don't match its actual read-only state (e.g. bs->file is a read-write node, but the permission still enforces read-only access).