[Qemu-devel] [PATCH for-3.1? 0/3] block: Fix two minor reopen issues

[Qemu-devel] [PATCH for-3.1? 0/3] block: Fix two minor reopen issues

[Max Reitz]

These are fixes for issues I found when looking after something Berto
has reported.  The second patch fixes that issue Berto found, the first
one is only kind of related.

For the first patch:  bdrv_reopen_abort() or bdrv_reopen_commit() are
called only if the respective bdrv_reopen_prepare() has succeeded.
However, bdrv_reopen_prepare() can fail even if the block driver’s
.bdrv_reopen_prepare() succeeded.  In that case, the block driver is
expecting a .bdrv_reopen_abort() or .bdrv_reopen_commit(), but will
never get it.

This is fixed by making bdrv_reopen_prepare() call .bdrv_reopen_abort()
if an error occurs after .bdrv_reopen_prepare() has already succeeded.

In practice this just prevents resource leaks, so nothing too dramatic
(fine for qemu-next).  It also means I’ve been incapable of writing a
test, sorry.


The second issue is more severe: file-posix applies the inverse share
locks when reopening.  Now the user can only directly do reopens from
the monitor for now, so that wouldn’t be so bad.  But of course there
are other places in qemu that implicitly reopen nodes, like dropping
R/W to RO or gaining R/W from RO.  Most of these places are symmetrical
at least (they’ll get R/W on an RO image for a short period of time and
then drop back to RO), so you’ll see the wrong shared permission locks
only for a short duration.  But at least blockdev-snapshot and
blockdev-snapshot-sync are one way; they drop R/W to RO and stay there.
So if you do a blockdev-snapshot*, the shared permission bits will be
flipped.  This is therefore very much user-visible.

It’s not catastrophic, though, so I’m not sure whether we need it in
3.1.  It’s definitely a bugfix, and I think we have patches queued for
the next RC already, so I think it makes sense to include at least
patches 2 and 3 as well.


Max Reitz (3):
  block: Always abort reopen after prepare succeeded
  file-posix: Fix shared locks on reopen commit
  iotests: Test file-posix locking and reopen

 block.c                    | 17 +++++++--
 block/file-posix.c         |  2 +-
 tests/qemu-iotests/182     | 71 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/182.out |  9 +++++
 4 files changed, 96 insertions(+), 3 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH for-3.1? 1/3] block: Always abort reopen after prepare succeeded

[Max Reitz]

bdrv_reopen_multiple() does not invoke bdrv_reopen_abort() for the
element of the reopen queue for which bdrv_reopen_prepare() failed,
because it assumes that the prepare function will have rolled back all
changes already.

However, bdrv_reopen_prepare() does not do this in every case: It may
notice an error after BlockDriver.bdrv_reopen_prepare() succeeded, and
it will not invoke BlockDriver.bdrv_reopen_abort() then; and neither
will bdrv_reopen_multiple(), as explained above.

This is wrong because we must always call .bdrv_reopen_commit() or
.bdrv_reopen_abort() after .bdrv_reopen_prepare() has succeeded.
Otherwise, the block driver has no chance to undo what it has done in
its implementation of .bdrv_reopen_prepare().

To fix this, bdrv_reopen_prepare() has to call .bdrv_reopen_abort() if
it wants to return an error after .bdrv_reopen_prepare() has succeeded.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index fd67e14dfa..7f5859aa74 100644
--- a/block.c
+++ b/block.c
@@ -3332,7 +3332,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
             if (!qobject_is_equal(new, old)) {
                 error_setg(errp, "Cannot change the option '%s'", entry->key);
                 ret = -EINVAL;
-                goto error;
+                goto late_error;
             }
         } while ((entry = qdict_next(reopen_state->options, entry)));
     }
@@ -3340,7 +3340,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     ret = bdrv_check_perm(reopen_state->bs, queue, reopen_state->perm,
                           reopen_state->shared_perm, NULL, errp);
     if (ret < 0) {
-        goto error;
+        goto late_error;
     }
 
     ret = 0;
@@ -3354,6 +3354,19 @@ error:
     qobject_unref(orig_reopen_opts);
     g_free(discard);
     return ret;
+
+late_error:
+    /* drv->bdrv_reopen_prepare() has succeeded, so we need to call
+     * drv->bdrv_reopen_abort() before signaling an error
+     * (bdrv_reopen_multiple() will not call bdrv_reopen_abort() when
+     * the respective bdrv_reopen_prepare() failed) */
+    if (drv->bdrv_reopen_abort) {
+        drv->bdrv_reopen_abort(reopen_state);
+    }
+    qemu_opts_del(opts);
+    qobject_unref(orig_reopen_opts);
+    g_free(discard);
+    return ret;
 }
 
 /*
-- 
2.17.2

Re: [Qemu-devel] [PATCH for-3.1? 1/3] block: Always abort reopen after prepare succeeded

[Alberto Garcia]

On Fri 16 Nov 2018 04:53:00 PM CET, Max Reitz wrote:
> bdrv_reopen_multiple() does not invoke bdrv_reopen_abort() for the
> element of the reopen queue for which bdrv_reopen_prepare() failed,
> because it assumes that the prepare function will have rolled back all
> changes already.
>
> However, bdrv_reopen_prepare() does not do this in every case: It may
> notice an error after BlockDriver.bdrv_reopen_prepare() succeeded, and
> it will not invoke BlockDriver.bdrv_reopen_abort() then; and neither
> will bdrv_reopen_multiple(), as explained above.
>
> This is wrong because we must always call .bdrv_reopen_commit() or
> .bdrv_reopen_abort() after .bdrv_reopen_prepare() has succeeded.
> Otherwise, the block driver has no chance to undo what it has done in
> its implementation of .bdrv_reopen_prepare().
>
> To fix this, bdrv_reopen_prepare() has to call .bdrv_reopen_abort() if
> it wants to return an error after .bdrv_reopen_prepare() has succeeded.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index fd67e14dfa..7f5859aa74 100644
> --- a/block.c
> +++ b/block.c
> @@ -3332,7 +3332,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>              if (!qobject_is_equal(new, old)) {
>                  error_setg(errp, "Cannot change the option '%s'", entry->key);
>                  ret = -EINVAL;
> -                goto error;
> +                goto late_error;
>              }
>          } while ((entry = qdict_next(reopen_state->options, entry)));
>      }
> @@ -3340,7 +3340,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>      ret = bdrv_check_perm(reopen_state->bs, queue, reopen_state->perm,
>                            reopen_state->shared_perm, NULL, errp);
>      if (ret < 0) {
> -        goto error;
> +        goto late_error;
>      }
>  
>      ret = 0;
> @@ -3354,6 +3354,19 @@ error:
>      qobject_unref(orig_reopen_opts);
>      g_free(discard);
>      return ret;
> +
> +late_error:
> +    /* drv->bdrv_reopen_prepare() has succeeded, so we need to call
> +     * drv->bdrv_reopen_abort() before signaling an error
> +     * (bdrv_reopen_multiple() will not call bdrv_reopen_abort() when
> +     * the respective bdrv_reopen_prepare() failed) */
> +    if (drv->bdrv_reopen_abort) {
> +        drv->bdrv_reopen_abort(reopen_state);
> +    }
> +    qemu_opts_del(opts);
> +    qobject_unref(orig_reopen_opts);
> +    g_free(discard);
> +    return ret;
>  }

Instead of having two exit points you could also have something like
bool drv_prepared, set it to 'true' after drv->bdrv_reopen_prepare() has
succeeded and then simply add this at the end:

if (ret < 0 && drv_prepared) {
   drv->bdrv_reopen_abort(reopen_state);
} 

Berto

Re: [Qemu-devel] [PATCH for-3.1? 1/3] block: Always abort reopen after prepare succeeded

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3085 bytes --]

On 16.11.18 17:02, Alberto Garcia wrote:
> On Fri 16 Nov 2018 04:53:00 PM CET, Max Reitz wrote:
>> bdrv_reopen_multiple() does not invoke bdrv_reopen_abort() for the
>> element of the reopen queue for which bdrv_reopen_prepare() failed,
>> because it assumes that the prepare function will have rolled back all
>> changes already.
>>
>> However, bdrv_reopen_prepare() does not do this in every case: It may
>> notice an error after BlockDriver.bdrv_reopen_prepare() succeeded, and
>> it will not invoke BlockDriver.bdrv_reopen_abort() then; and neither
>> will bdrv_reopen_multiple(), as explained above.
>>
>> This is wrong because we must always call .bdrv_reopen_commit() or
>> .bdrv_reopen_abort() after .bdrv_reopen_prepare() has succeeded.
>> Otherwise, the block driver has no chance to undo what it has done in
>> its implementation of .bdrv_reopen_prepare().
>>
>> To fix this, bdrv_reopen_prepare() has to call .bdrv_reopen_abort() if
>> it wants to return an error after .bdrv_reopen_prepare() has succeeded.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index fd67e14dfa..7f5859aa74 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3332,7 +3332,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>>              if (!qobject_is_equal(new, old)) {
>>                  error_setg(errp, "Cannot change the option '%s'", entry->key);
>>                  ret = -EINVAL;
>> -                goto error;
>> +                goto late_error;
>>              }
>>          } while ((entry = qdict_next(reopen_state->options, entry)));
>>      }
>> @@ -3340,7 +3340,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>>      ret = bdrv_check_perm(reopen_state->bs, queue, reopen_state->perm,
>>                            reopen_state->shared_perm, NULL, errp);
>>      if (ret < 0) {
>> -        goto error;
>> +        goto late_error;
>>      }
>>  
>>      ret = 0;
>> @@ -3354,6 +3354,19 @@ error:
>>      qobject_unref(orig_reopen_opts);
>>      g_free(discard);
>>      return ret;
>> +
>> +late_error:
>> +    /* drv->bdrv_reopen_prepare() has succeeded, so we need to call
>> +     * drv->bdrv_reopen_abort() before signaling an error
>> +     * (bdrv_reopen_multiple() will not call bdrv_reopen_abort() when
>> +     * the respective bdrv_reopen_prepare() failed) */
>> +    if (drv->bdrv_reopen_abort) {
>> +        drv->bdrv_reopen_abort(reopen_state);
>> +    }
>> +    qemu_opts_del(opts);
>> +    qobject_unref(orig_reopen_opts);
>> +    g_free(discard);
>> +    return ret;
>>  }
> 
> Instead of having two exit points you could also have something like
> bool drv_prepared, set it to 'true' after drv->bdrv_reopen_prepare() has
> succeeded and then simply add this at the end:
> 
> if (ret < 0 && drv_prepared) {
>    drv->bdrv_reopen_abort(reopen_state);
> } 

Yup, sure.

Max



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH for-3.1? 2/3] file-posix: Fix shared locks on reopen commit

[Max Reitz]

s->locked_shared_perm is the set of bits locked in the file, which is
the inverse of the permissions actually shared.  So we need to pass them
as they are to raw_apply_lock_bytes() instead of inverting them again.

Reported-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index df3a8d7cdf..8460d003f0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -963,7 +963,7 @@ static void raw_reopen_commit(BDRVReopenState *state)
 
     /* Copy locks to the new fd before closing the old one. */
     raw_apply_lock_bytes(NULL, rs->fd, s->locked_perm,
-                         ~s->locked_shared_perm, false, &local_err);
+                         s->locked_shared_perm, false, &local_err);
     if (local_err) {
         /* shouldn't fail in a sane host, but report it just in case. */
         error_report_err(local_err);
-- 
2.17.2

[Qemu-devel] [PATCH for-3.1? 3/3] iotests: Test file-posix locking and reopen

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/182     | 71 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/182.out |  9 +++++
 2 files changed, 80 insertions(+)

diff --git a/tests/qemu-iotests/182 b/tests/qemu-iotests/182
index 4b31592fb8..3b7689c1d5 100755
--- a/tests/qemu-iotests/182
+++ b/tests/qemu-iotests/182
@@ -31,6 +31,7 @@ status=1	# failure is the default!
 _cleanup()
 {
     _cleanup_test_img
+    rm -f "$TEST_IMG.overlay"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -71,6 +72,76 @@ echo 'quit' | $QEMU -nographic -monitor stdio \
 
 _cleanup_qemu
 
+echo
+echo '=== Testing reopen ==='
+echo
+
+# This tests that reopening does not unshare any permissions it should
+# not unshare
+# (There was a bug where reopening shared exactly the opposite of the
+# permissions it was supposed to share)
+
+_launch_qemu
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'qmp_capabilities'}" \
+    'return'
+
+# Open the image without any format layer (we are not going to access
+# it, so that is fine)
+# This should keep all permissions shared.
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'blockdev-add',
+      'arguments': {
+          'node-name': 'node0',
+          'driver': 'file',
+          'filename': '$TEST_IMG',
+          'locking': 'on'
+          } }" \
+    'return' \
+    'error'
+
+# This snapshot will perform a reopen to drop R/W to RO.
+# It should still keep all permissions shared.
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'blockdev-snapshot-sync',
+      'arguments': {
+          'node-name': 'node0',
+          'snapshot-file': '$TEST_IMG.overlay',
+          'snapshot-node-name': 'node1'
+      } }" \
+    'return' \
+    'error'
+
+# Now open the same file again
+# This does not require any permissions (and does not unshare any), so
+# this will not conflict with node0.
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'blockdev-add',
+      'arguments': {
+          'node-name': 'node1',
+          'driver': 'file',
+          'filename': '$TEST_IMG',
+          'locking': 'on'
+          } }" \
+    'return' \
+    'error'
+
+# Now we attach the image to a virtio-blk device.  This device does
+# require some permissions (at least WRITE and READ_CONSISTENT), so if
+# reopening node0 unshared any (which it should not have), this will
+# fail (but it should not).
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'device_add',
+      'arguments': {
+          'driver': 'virtio-blk',
+          'drive': 'node1'
+      } }" \
+    'return' \
+    'error'
+
+_cleanup_qemu
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/182.out b/tests/qemu-iotests/182.out
index f1463c8862..af501ca3f3 100644
--- a/tests/qemu-iotests/182.out
+++ b/tests/qemu-iotests/182.out
@@ -5,4 +5,13 @@ Starting QEMU
 Starting a second QEMU using the same image should fail
 QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,id=drive0,file.locking=on: Failed to get "write" lock
 Is another process using the image [TEST_DIR/t.qcow2]?
+
+=== Testing reopen ===
+
+{"return": {}}
+{"return": {}}
+Formatting 'TEST_DIR/t.qcow2.overlay', fmt=qcow2 size=197120 backing_file=TEST_DIR/t.qcow2 backing_fmt=file cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{"return": {}}
+{"return": {}}
+{"return": {}}
 *** done
-- 
2.17.2