[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

[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 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 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>

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