[PATCH 0/2] Fix nested permission update

[PATCH 0/2] Fix nested permission update

[Vladimir Sementsov-Ogievskiy]

Hi! With help of some assertions (patch 2) I've found that
bdrv_drop_intermediate() do nested permission update which in my opinion
may lead to unpredictable behavior.

Vladimir Sementsov-Ogievskiy (2):
  block: make bdrv_drop_intermediate() less wrong
  block: assert that permission commit sets same permissions

 block.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

-- 
2.21.3

[PATCH 1/2] block: make bdrv_drop_intermediate() less wrong

[Vladimir Sementsov-Ogievskiy]

First, permission update loop tries to do iterations transactionally,
but the whole update is not transactional: nobody roll-back successful
loop iterations when some iteration fails.

Second, in the iteration we have nested permission update:
c->klass->update_filename may point to bdrv_child_cb_update_filename()
which calls bdrv_backing_update_filename(), which may do node reopen to
RW.

Permission update system is not prepared to nested updates, at least it
has intermediate permission-update state stored in BdrvChild
structures: has_backup_perm, backup_perm and backup_shared_perm.

So, let's first do bdrv_replace_node() (which is more transactional
than open-coded update in bdrv_drop_intermediate()) and then call
update_filename() in separate. We still do not rollback changes in case
of update_filename() failure but it's not much worse than pre-patch
behavior.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index 9538af4884..bd9f4e534b 100644
--- a/block.c
+++ b/block.c
@@ -4958,36 +4958,30 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
         backing_file_str = base->filename;
     }
 
-    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
-        /* 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, NULL, &local_err);
-        g_slist_free(ignore_children);
-        if (ret < 0) {
-            error_report_err(local_err);
-            goto exit;
-        }
+    bdrv_replace_node(top, base, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        goto exit;
+    }
 
-        /* If so, update the backing file path in the image file */
+    QLIST_FOREACH_SAFE(c, &base->parents, next_parent, next) {
         if (c->klass->update_filename) {
             ret = c->klass->update_filename(c, base, backing_file_str,
                                             &local_err);
             if (ret < 0) {
-                bdrv_abort_perm_update(base);
+                /*
+                 * TODO: Actually, we want to rollback all previous iterations
+                 * of this loop, and (which is almost impossible) previous
+                 * bdrv_replace_node()...
+                 *
+                 * Note, that c->klass->update_filename may lead to permission
+                 * update, so it's a bad idea to call it inside permission
+                 * update transaction of bdrv_replace_node.
+                 */
                 error_report_err(local_err);
                 goto exit;
             }
         }
-
-        /*
-         * Do the actual switch in the in-memory graph.
-         * Completes bdrv_check_update_perm() transaction internally.
-         * c->frozen is false, we have checked that above.
-         */
-        bdrv_ref(base);
-        bdrv_replace_child(c, base);
-        bdrv_unref(top);
     }
 
     if (update_inherits_from) {
-- 
2.21.3

Re: [PATCH 1/2] block: make bdrv_drop_intermediate() less wrong

[Max Reitz]

On 31.10.20 13:35, Vladimir Sementsov-Ogievskiy wrote:
> First, permission update loop tries to do iterations transactionally,
> but the whole update is not transactional: nobody roll-back successful
> loop iterations when some iteration fails.

Indeed.  Another thing that should be noted:

bdrv_check_update_perm() doc:
 > Needs to be followed by a call to either bdrv_set_perm()
 > or bdrv_abort_perm_update().

And besides rolling back on error, we don’t commit on success either.

> Second, in the iteration we have nested permission update:
> c->klass->update_filename may point to bdrv_child_cb_update_filename()
> which calls bdrv_backing_update_filename(), which may do node reopen to
> RW.
> 
> Permission update system is not prepared to nested updates, at least it
> has intermediate permission-update state stored in BdrvChild
> structures: has_backup_perm, backup_perm and backup_shared_perm.
> 
> So, let's first do bdrv_replace_node() (which is more transactional
> than open-coded update in bdrv_drop_intermediate()) and then call
> update_filename() in separate. We still do not rollback changes in case
> of update_filename() failure but it's not much worse than pre-patch
> behavior.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block.c | 36 +++++++++++++++---------------------
>   1 file changed, 15 insertions(+), 21 deletions(-)

And it’s also much simpler and clearer now.

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [PATCH 1/2] block: make bdrv_drop_intermediate() less wrong

[Max Reitz]

On 05.11.20 14:22, Max Reitz wrote:
> On 31.10.20 13:35, Vladimir Sementsov-Ogievskiy wrote:
>> First, permission update loop tries to do iterations transactionally,
>> but the whole update is not transactional: nobody roll-back successful
>> loop iterations when some iteration fails.

[...]

> And besides rolling back on error, we don’t commit on success either.

(Oh, we do, through bdrv_replace_child().  Well.)

Max

Re: [PATCH 1/2] block: make bdrv_drop_intermediate() less wrong

[Alberto Garcia]

On Sat 31 Oct 2020 01:35:01 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> @@ -4958,36 +4958,30 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
>          backing_file_str = base->filename;
>      }
>  
> -    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
> -        /* 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, NULL, &local_err);
> -        g_slist_free(ignore_children);
> -        if (ret < 0) {
> -            error_report_err(local_err);
> -            goto exit;
> -        }
> +    bdrv_replace_node(top, base, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        goto exit;
> +    }

At the beginning of this function there's a check for c->frozen. I think
you can remove it safely because you also have it in bdrv_replace_node()

Berto

Re: [PATCH 1/2] block: make bdrv_drop_intermediate() less wrong

[Alberto Garcia]

On Sat 31 Oct 2020 01:35:01 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> -    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
  /* ... */
> +    QLIST_FOREACH_SAFE(c, &base->parents, next_parent, next) {

I also wonder, is top->parents and base->parents guaranteed to be the
same list in this case?

If not you could store the list of top->parents before calling
bdrv_replace_node() and use it afterwards.

    QLIST_FOREACH(c, &top->parents, next_parent) {
        parents = g_slist_prepend(parents, c);
    }

Berto

Re: [PATCH 1/2] block: make bdrv_drop_intermediate() less wrong

[Vladimir Sementsov-Ogievskiy]

05.11.2020 18:14, Alberto Garcia wrote:
> On Sat 31 Oct 2020 01:35:01 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> -    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
>    /* ... */
>> +    QLIST_FOREACH_SAFE(c, &base->parents, next_parent, next) {
> 
> I also wonder, is top->parents and base->parents guaranteed to be the
> same list in this case?
> 
> If not you could store the list of top->parents before calling
> bdrv_replace_node() and use it afterwards.
> 
>      QLIST_FOREACH(c, &top->parents, next_parent) {
>          parents = g_slist_prepend(parents, c);
>      }
> 
> Berto
> 

Hmm... We should not touch other parents of base, which it had prior to bdrv_replace_node().

On the other hand, it should be safe to call update_filename for not-updated parents..

And we should keep in mind that bdrv_replace_node may replace not all children. Still, in this
case we must replace all of them. Seems, we need additional argument for bdrv_replace_node()
to fail, if it want's to skip some children replacement.

So, I'm going to resend to add this parameter to bdrv_replace_node() and will use your
suggestion to be stricter on what we update, thanks!


-- 
Best regards,
Vladimir

[PATCH 2/2] block: assert that permission commit sets same permissions

[Vladimir Sementsov-Ogievskiy]

On permission update commit we must set same permissions as on _check_.
Let's add assertions. Next step may be to drop permission parameters
from _set_.

Note that prior to previous commit, fixing bdrv_drop_intermediate(),
new assertion in bdrv_child_set_perm() crashes on iotests 30 and 40.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index bd9f4e534b..0f4da59a6c 100644
--- a/block.c
+++ b/block.c
@@ -2105,9 +2105,10 @@ static void bdrv_abort_perm_update(BlockDriverState *bs)
     }
 }
 
-static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
-                          uint64_t cumulative_shared_perms)
+static void bdrv_set_perm(BlockDriverState *bs, uint64_t _cumulative_perms,
+                          uint64_t _cumulative_shared_perms)
 {
+    uint64_t cumulative_perms, cumulative_shared_perms;
     BlockDriver *drv = bs->drv;
     BdrvChild *c;
 
@@ -2115,6 +2116,10 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
         return;
     }
 
+    bdrv_get_cumulative_perm(bs, &cumulative_perms, &cumulative_shared_perms);
+    assert(_cumulative_perms == cumulative_perms);
+    assert(_cumulative_shared_perms == cumulative_shared_perms);
+
     /* Update this node */
     if (drv->bdrv_set_perm) {
         drv->bdrv_set_perm(bs, cumulative_perms, cumulative_shared_perms);
@@ -2301,6 +2306,8 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
 
     c->has_backup_perm = false;
 
+    assert(c->perm == perm);
+    assert(c->shared_perm == shared);
     c->perm = perm;
     c->shared_perm = shared;
 
-- 
2.21.3

Re: [PATCH 2/2] block: assert that permission commit sets same permissions

[Vladimir Sementsov-Ogievskiy]

31.10.2020 15:35, Vladimir Sementsov-Ogievskiy wrote:
> On permission update commit we must set same permissions as on _check_.
> Let's add assertions. Next step may be to drop permission parameters
> from _set_.
> 
> Note that prior to previous commit, fixing bdrv_drop_intermediate(),
> new assertion in bdrv_child_set_perm() crashes on iotests 30 and 40.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index bd9f4e534b..0f4da59a6c 100644
> --- a/block.c
> +++ b/block.c
> @@ -2105,9 +2105,10 @@ static void bdrv_abort_perm_update(BlockDriverState *bs)
>       }
>   }
>   
> -static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
> -                          uint64_t cumulative_shared_perms)
> +static void bdrv_set_perm(BlockDriverState *bs, uint64_t _cumulative_perms,
> +                          uint64_t _cumulative_shared_perms)
>   {
> +    uint64_t cumulative_perms, cumulative_shared_perms;
>       BlockDriver *drv = bs->drv;
>       BdrvChild *c;
>   
> @@ -2115,6 +2116,10 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
>           return;
>       }
>   
> +    bdrv_get_cumulative_perm(bs, &cumulative_perms, &cumulative_shared_perms);
> +    assert(_cumulative_perms == cumulative_perms);
> +    assert(_cumulative_shared_perms == cumulative_shared_perms);
> +
>       /* Update this node */
>       if (drv->bdrv_set_perm) {
>           drv->bdrv_set_perm(bs, cumulative_perms, cumulative_shared_perms);
> @@ -2301,6 +2306,8 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
>   
>       c->has_backup_perm = false;
>   
> +    assert(c->perm == perm);
> +    assert(c->shared_perm == shared);
>       c->perm = perm;
>       c->shared_perm = shared;
>   
> 

squash:

@@ -2308,8 +2308,6 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
  
      assert(c->perm == perm);
      assert(c->shared_perm == shared);
-    c->perm = perm;
-    c->shared_perm = shared;
  
      bdrv_get_cumulative_perm(c->bs, &cumulative_perms,
                               &cumulative_shared_perms);



-- 
Best regards,
Vladimir

Re: [PATCH 2/2] block: assert that permission commit sets same permissions

[Max Reitz]

On 31.10.20 13:35, Vladimir Sementsov-Ogievskiy wrote:
> On permission update commit we must set same permissions as on _check_.
> Let's add assertions. Next step may be to drop permission parameters
> from _set_.
> 
> Note that prior to previous commit, fixing bdrv_drop_intermediate(),
> new assertion in bdrv_child_set_perm() crashes on iotests 30 and 40.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index bd9f4e534b..0f4da59a6c 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -2301,6 +2306,8 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
>   
>       c->has_backup_perm = false;
>   
> +    assert(c->perm == perm);
> +    assert(c->shared_perm == shared);
>       c->perm = perm;
>       c->shared_perm = shared;

Then we can drop the assignments, no?

(And, as you write, in the future potentially drop the parameters.)

Anyway:

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH RFC 00/21] block: update graph permissions update

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is a proposal of updating graph changing procedures.

The thing brought me here is a question about "activating" filters after
insertion, which is done in mirror_top and backup_top. The problem is
that we can't simply avoid permission conflict when inserting the
filter: during insertion old permissions of relations to be removed
conflicting with new permissions of new created relations. And current
solution is supporting additional "inactive" mode for the filter when it
doesn't require any permissions.

I suggest to change the order of operations: let's first do all graph
relations modifications and then refresh permissions. Of course we'll
need a way to restore old graph if refresh fails.

Another problem with permission update is that we update permissions in
order of DFS which is not always correct. Better is update node when all
its parents already updated and require correct permissions. This needs
a topological sort of nodes prior to permission update, see more in
patches later.

Key patches here:

01,02 - add failing tests to illustrate conceptual problems of current
permission update system.
[Here is side suggestion: we usually add tests after fix, so careful
 reviewer has to change order of patches to check that test fails before
 fix. I add tests in the way the may be simply executed but not yet take
 part in make check. It seems more native: first show the problem, then
 fix it. And when fixed, make tests available for make check]

08 - toplogical sort implemented for permission update, one of new tests
now pass

13 - improve bdrv_replace_node. second new test now pass

21 - drop .active field and activation procedure for backup-top!

Series marked as RFC as they are non-complete. Some things like
bdrv_replace_node() and bdrv_append() are moved into new paradigm
(update graph first) some not (like bdrv_reopen_multiple()). Because of
it we have to still support old interfaces (like ignore_children).

Still, I'd be very grateful for some feedback before investigating more
time to this thing.

Note, that this series does nothing with another graph-update problem
discussed under "[PATCH RFC 0/5] Fix accidental crash in iotest 30".


The series based on block-next Max's branch and can be found here:

git: https://src.openvz.org/scm/~vsementsov/qemu.git
tag: up-block-topologic-perm-v1

Vladimir Sementsov-Ogievskiy (21):
  tests/test-bdrv-graph-mod: add test_parallel_exclusive_write
  tests/test-bdrv-graph-mod: add test_parallel_perm_update
  util: add transactions.c
  block: bdrv_refresh_perms: check parents compliance
  block: refactor bdrv_child* permission functions
  block: rewrite bdrv_child_try_set_perm() using bdrv_refresh_perms()
  block: inline bdrv_child_*() permission functions calls
  block: use topological sort for permission update
  block: add bdrv_drv_set_perm transaction action
  block: add bdrv_list_* permission update functions
  block: add bdrv_replace_child_safe() transaction action
  block: return value from bdrv_replace_node()
  block: fix bdrv_replace_node_common
  block: add bdrv_attach_child_noperm() transaction action
  block: split out bdrv_replace_node_noperm()
  block: bdrv_append(): don't consume reference
  block: bdrv_append(): return status
  block: adapt bdrv_append() for inserting filters
  block: add bdrv_remove_backing transaction action
  block: introduce bdrv_drop_filter()
  block/backup-top: drop .active

 include/block/block.h       |   9 +-
 include/qemu/transactions.h |  46 +++
 block.c                     | 789 ++++++++++++++++++++++++++++--------
 block/backup-top.c          |  39 +-
 block/commit.c              |   7 +-
 block/mirror.c              |   9 +-
 blockdev.c                  |  10 +-
 tests/test-bdrv-drain.c     |   2 +-
 tests/test-bdrv-graph-mod.c | 122 +++++-
 util/transactions.c         |  81 ++++
 tests/qemu-iotests/283.out  |   2 +-
 util/meson.build            |   1 +
 12 files changed, 872 insertions(+), 245 deletions(-)
 create mode 100644 include/qemu/transactions.h
 create mode 100644 util/transactions.c

-- 
2.21.3

[PATCH 2/2] block: assert that permission commit sets same permissions

[Vladimir Sementsov-Ogievskiy]

On permission update commit we must set same permissions as on _check_.
Let's add assertions. Next step may be to drop permission parameters
from _set_.

Note that prior to previous commit, fixing bdrv_drop_intermediate(),
new assertion in bdrv_child_set_perm() crashes on iotests 30 and 40.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index bd9f4e534b..0f4da59a6c 100644
--- a/block.c
+++ b/block.c
@@ -2105,9 +2105,10 @@ static void bdrv_abort_perm_update(BlockDriverState *bs)
     }
 }
 
-static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
-                          uint64_t cumulative_shared_perms)
+static void bdrv_set_perm(BlockDriverState *bs, uint64_t _cumulative_perms,
+                          uint64_t _cumulative_shared_perms)
 {
+    uint64_t cumulative_perms, cumulative_shared_perms;
     BlockDriver *drv = bs->drv;
     BdrvChild *c;
 
@@ -2115,6 +2116,10 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
         return;
     }
 
+    bdrv_get_cumulative_perm(bs, &cumulative_perms, &cumulative_shared_perms);
+    assert(_cumulative_perms == cumulative_perms);
+    assert(_cumulative_shared_perms == cumulative_shared_perms);
+
     /* Update this node */
     if (drv->bdrv_set_perm) {
         drv->bdrv_set_perm(bs, cumulative_perms, cumulative_shared_perms);
@@ -2301,6 +2306,8 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
 
     c->has_backup_perm = false;
 
+    assert(c->perm == perm);
+    assert(c->shared_perm == shared);
     c->perm = perm;
     c->shared_perm = shared;
 
-- 
2.21.3

Re: [PATCH 2/2] block: assert that permission commit sets same permissions

[Vladimir Sementsov-Ogievskiy]

23.11.2020 23:12, Vladimir Sementsov-Ogievskiy wrote:
> On permission update commit we must set same permissions as on_check_.
> Let's add assertions. Next step may be to drop permission parameters
> from_set_.
> 
> Note that prior to previous commit, fixing bdrv_drop_intermediate(),
> new assertion in bdrv_child_set_perm() crashes on iotests 30 and 40.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>

this is accidental patch, ignore it

-- 
Best regards,
Vladimir