[v2,7/7] block: Ignore loosening perm restrictions failures

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(-)

On 08.05.19 20:25, Max Reitz wrote:
> We generally assume that loosening permission restrictions can never
> fail.  We have seen in the past that this assumption is wrong.  This has
> led to crashes because we generally pass &error_abort when loosening
> permissions.
> 
> However, a failure in such a case should actually be handled in quite
> the opposite way: It is very much not fatal, so qemu may report it, but
> still consider the operation successful.  The only realistic problem is
> that qemu may then retain permissions and thus locks on images it
> actually does not require.  But again, that is not fatal.
> 
> To implement this behavior, we make all functions that change
> permissions and that pass &error_abort to the initiating function
> (bdrv_check_perm() or bdrv_child_check_perm()) evaluate the
> @loosen_restrictions value introduced in the previous patch.  If it is
> true and an error did occur, we abort the permission update, report
> the error, and report success to the caller.

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

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

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

(Reproducer:

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



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

)

Should I just drop the warning?

Max

> bdrv_child_try_set_perm() itself does not pass &error_abort, but it is
> the only public function to change permissions.  As such, callers may
> pass &error_abort to it, expecting dropping permission restrictions to
> never fail.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 40 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 8f517be5b4..a5a03906d0 100644
> --- a/block.c
> +++ b/block.c
> @@ -2088,11 +2088,20 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
>  int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
>                              Error **errp)
>  {
> +    Error *local_err = NULL;
>      int ret;
> +    bool tighten_restrictions;
>  
> -    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, NULL, errp);
> +    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL,
> +                                &tighten_restrictions, &local_err);
>      if (ret < 0) {
>          bdrv_child_abort_perm_update(c);
> +        if (tighten_restrictions) {
> +            error_propagate(errp, local_err);
> +        } else {
> +            warn_reportf_err(local_err, "Failed to loosen restrictions: ");
> +            ret = 0;
> +        }
>          return ret;
>      }
>  
> @@ -2275,10 +2284,20 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>          /* Update permissions for old node. This is guaranteed to succeed
>           * because we're just taking a parent away, so we're loosening
>           * restrictions. */
> +        bool tighten_restrictions;
> +        Error *local_err = NULL;
> +        int ret;
> +
>          bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
> -        bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
> -                        NULL, &error_abort);
> -        bdrv_set_perm(old_bs, perm, shared_perm);
> +        ret = bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
> +                              &tighten_restrictions, &local_err);
> +        assert(tighten_restrictions == false);
> +        if (ret < 0) {
> +            warn_reportf_err(local_err, "Failed to loosen restrictions: ");
> +            bdrv_abort_perm_update(old_bs);
> +        } else {
> +            bdrv_set_perm(old_bs, perm, shared_perm);
> +        }
>      }
>  }
>  
> @@ -5322,7 +5341,9 @@ static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
>  static int bdrv_inactivate_recurse(BlockDriverState *bs)
>  {
>      BdrvChild *child, *parent;
> +    bool tighten_restrictions;
>      uint64_t perm, shared_perm;
> +    Error *local_err = NULL;
>      int ret;
>  
>      if (!bs->drv) {
> @@ -5358,8 +5379,15 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
>  
>      /* Update permissions, they may differ for inactive nodes */
>      bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
> -    bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &error_abort);
> -    bdrv_set_perm(bs, perm, shared_perm);
> +    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL,
> +                          &tighten_restrictions, &local_err);
> +    assert(tighten_restrictions == false);
> +    if (ret < 0) {
> +        warn_reportf_err(local_err, "Failed to loosen restrictions: ");
> +        bdrv_abort_perm_update(bs);
> +    } else {
> +        bdrv_set_perm(bs, perm, shared_perm);
> +    }
>  
>  
>      /* Recursively inactivate children */
>