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

[Max Reitz]

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 <address@hidden>
> ---
>  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 */
>

signature.asc
Description: OpenPGP digital signature