[PATCH 0/5] block/amend: Fix failures seen in iotest 296

[PATCH 0/5] block/amend: Fix failures seen in iotest 296

[Hanna Reitz]

Hi,

I’ve tried basing my block branch on Kevin’s and noticed that after
“crypto: perform permission checks under BQL”, iotest 296 was failing.
I/We have debugged those failures and here are fixes for it.

Hence, this series is based on Kevin’s block branch
(efa33ed9b298d39e2b8c19c5f4bdd80a3b632260 at the time of writing this
cover letter).  I’ve pushed it here:

  https://gitlab.com/hreitz/qemu/-/commits/amend-job-fixes-v1

Patch 1 adds clean-up of the amend job in an error path that said commit
adds to qmp_x_blockdev_amend().

Patch 2 changes the type of a JobDriver callback added in that commit;
together with patch 3, this is kind of a matter of style, but it can
also replace patch 3 and fix the bug that it fixes in another way.

Patch 3 fixes a permission bug: When changing the permissions fails
before amend, block/crypto will still keep updating_keys to be true.
Without patch 2, that will remains so indefinitely and then
block_crypto_child_perms() will continue to unshare the CONSISTENT_READ
permission, which is wrong.  (Patch 2 fixes this problem, too,
specifically because with it, block_crypto_amend_cleanup() will always
be called when the job is dismissed, and so updating_keys will be reset
at least then.)

Patch 4 fixes an issue that’s not related to “crypto: perform permission
checks under BQL”, but it became appearent only while debugging the
other issues here, so it’s part of this series, too.

Patch 5 fixes the test itself.  It expects permission-related errors to
occur when the job is already running, not as an immediate result of the
QMP x-blockdev-amend command.  “crypto: perform permission checks under
BQL” has changed this, so the test needs to take that into account.


Ideally, I believe the following patches should be squashed into
“crypto: perform permission checks under BQL” lest bisect breaks:
- Patch 1
- Patch 2 or 3 (or both)
- Patch 5

But if that isn’t feasible, we can just take the whole series on top.


Hanna Reitz (5):
  block/amend: Clean up job on early failure
  block/amend: Always call .bdrv_amend_clean()
  block/crypto: Reset updating_keys on perm failure
  block/amend: Keep strong reference to BDS
  iotests/296: Accept early failure

 block/amend.c              |  8 ++++++--
 block/crypto.c             |  8 +++++++-
 tests/qemu-iotests/296     |  8 ++++++--
 tests/qemu-iotests/296.out | 17 +++++------------
 4 files changed, 24 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH 1/5] block/amend: Clean up job on early failure

[Hanna Reitz]

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block/amend.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/amend.c b/block/amend.c
index 329bca53dc..f465738665 100644
--- a/block/amend.c
+++ b/block/amend.c
@@ -134,6 +134,7 @@ void qmp_x_blockdev_amend(const char *job_id,
     s->force = has_force ? force : false;
 
     if (blockdev_amend_pre_run(s, errp)) {
+        job_early_fail(&s->common);
         return;
     }
 
-- 
2.34.1

[PATCH 2/5] block/amend: Always call .bdrv_amend_clean()

[Hanna Reitz]

.bdrv_amend_clean() says block drivers can use it to clean up what was
done in .bdrv_amend_pre_run().  Therefore, it should always be called
after .bdrv_amend_pre_run(), which means we need it to call it in the
JobDriver.free() callback, not in JobDriver.clean().

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block/amend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/amend.c b/block/amend.c
index f465738665..553890ded9 100644
--- a/block/amend.c
+++ b/block/amend.c
@@ -62,7 +62,7 @@ static int blockdev_amend_pre_run(BlockdevAmendJob *s, Error **errp)
     return 0;
 }
 
-static void blockdev_amend_clean(Job *job)
+static void blockdev_amend_free(Job *job)
 {
     BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
 
@@ -75,7 +75,7 @@ static const JobDriver blockdev_amend_job_driver = {
     .instance_size = sizeof(BlockdevAmendJob),
     .job_type      = JOB_TYPE_AMEND,
     .run           = blockdev_amend_run,
-    .clean         = blockdev_amend_clean,
+    .free          = blockdev_amend_free,
 };
 
 void qmp_x_blockdev_amend(const char *job_id,
-- 
2.34.1

[PATCH 3/5] block/crypto: Reset updating_keys on perm failure

[Hanna Reitz]

When the permissions could not be changed as would be necessary for
updating the keys, reset updating_keys to false so
block_crypto_child_perms() will not continue to try claiming these
permissions.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block/crypto.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/crypto.c b/block/crypto.c
index 5225a68a54..9d5fecbef8 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -781,10 +781,16 @@ static int
 block_crypto_amend_prepare(BlockDriverState *bs, Error **errp)
 {
     BlockCrypto *crypto = bs->opaque;
+    int ret;
 
     /* apply for exclusive read/write permissions to the underlying file */
     crypto->updating_keys = true;
-    return bdrv_child_refresh_perms(bs, bs->file, errp);
+    ret = bdrv_child_refresh_perms(bs, bs->file, errp);
+    if (ret < 0) {
+        /* Well, in this case we will not be updating any keys */
+        crypto->updating_keys = false;
+    }
+    return ret;
 }
 
 static void
-- 
2.34.1

[PATCH 4/5] block/amend: Keep strong reference to BDS

[Hanna Reitz]

Otherwise, the BDS might be freed while the job is running, which would
cause a use-after-free.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block/amend.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/amend.c b/block/amend.c
index 553890ded9..f696a006e3 100644
--- a/block/amend.c
+++ b/block/amend.c
@@ -69,6 +69,8 @@ static void blockdev_amend_free(Job *job)
     if (s->bs->drv->bdrv_amend_clean) {
         s->bs->drv->bdrv_amend_clean(s->bs);
     }
+
+    bdrv_unref(s->bs);
 }
 
 static const JobDriver blockdev_amend_job_driver = {
@@ -129,6 +131,7 @@ void qmp_x_blockdev_amend(const char *job_id,
         return;
     }
 
+    bdrv_ref(bs);
     s->bs = bs,
     s->opts = QAPI_CLONE(BlockdevAmendOptions, options),
     s->force = has_force ? force : false;
-- 
2.34.1

[PATCH 5/5] iotests/296: Accept early failure

[Hanna Reitz]

As of "crypto: perform permission checks under BQL", permission failures
occur early in the blockdev-amend job to update a LUKS volume's keys.
Expect them in x-blockdev-amend's QMP reply instead of waiting for the
actual job to fail later.

(Note that the job will still be created, so we need to wait for it to
disappear even when the QMP command failed.  Otherwise, the job ID
"job0" will remain in use and we cannot launch another job with the same
ID.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/296     |  8 ++++++--
 tests/qemu-iotests/296.out | 17 +++++------------
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index 099a3eeaa5..f80ef3434a 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -174,8 +174,12 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
         }
 
         result = vm.qmp('x-blockdev-amend', **args)
-        assert result['return'] == {}
-        vm.run_job('job0')
+        iotests.log(result)
+        # Run the job only if it was created
+        event = ('JOB_STATUS_CHANGE',
+                 {'data': {'id': 'job0', 'status': 'created'}})
+        if vm.events_wait([event], timeout=0.0) is not None:
+            vm.run_job('job0')
 
     # test that when the image opened by two qemu processes,
     # neither of them can update the encryption keys
diff --git a/tests/qemu-iotests/296.out b/tests/qemu-iotests/296.out
index 42205cc981..609826eaa0 100644
--- a/tests/qemu-iotests/296.out
+++ b/tests/qemu-iotests/296.out
@@ -1,11 +1,9 @@
 
-{"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
-Job failed: Failed to get shared "consistent read" lock
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
-Job failed: Failed to get shared "consistent read" lock
-{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"error": {"class": "GenericError", "desc": "Failed to get shared \"consistent read\" lock"}}
+{"error": {"class": "GenericError", "desc": "Failed to get shared \"consistent read\" lock"}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
@@ -13,14 +11,9 @@ qemu-img: Failed to get shared "consistent read" lock
 Is another process using the image [TEST_DIR/test.img]?
 
 .
-Job failed: Block node is read-only
-{"execute": "job-dismiss", "arguments": {"id": "job0"}}
-{"return": {}}
-Job failed: Failed to get shared "consistent read" lock
-{"execute": "job-dismiss", "arguments": {"id": "job0"}}
-{"return": {}}
-Job failed: Failed to get shared "consistent read" lock
-{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"error": {"class": "GenericError", "desc": "Block node is read-only"}}
+{"error": {"class": "GenericError", "desc": "Failed to get shared \"consistent read\" lock"}}
+{"error": {"class": "GenericError", "desc": "Failed to get shared \"consistent read\" lock"}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
-- 
2.34.1

Re: [PATCH 0/5] block/amend: Fix failures seen in iotest 296

[Kevin Wolf]

Am 04.03.2022 um 16:37 hat Hanna Reitz geschrieben:
> Hi,
> 
> I’ve tried basing my block branch on Kevin’s and noticed that after
> “crypto: perform permission checks under BQL”, iotest 296 was failing.
> I/We have debugged those failures and here are fixes for it.
> 
> Hence, this series is based on Kevin’s block branch
> (efa33ed9b298d39e2b8c19c5f4bdd80a3b632260 at the time of writing this
> cover letter).  I’ve pushed it here:
> 
>   https://gitlab.com/hreitz/qemu/-/commits/amend-job-fixes-v1
> 
> Patch 1 adds clean-up of the amend job in an error path that said commit
> adds to qmp_x_blockdev_amend().
> 
> Patch 2 changes the type of a JobDriver callback added in that commit;
> together with patch 3, this is kind of a matter of style, but it can
> also replace patch 3 and fix the bug that it fixes in another way.
> 
> Patch 3 fixes a permission bug: When changing the permissions fails
> before amend, block/crypto will still keep updating_keys to be true.
> Without patch 2, that will remains so indefinitely and then
> block_crypto_child_perms() will continue to unshare the CONSISTENT_READ
> permission, which is wrong.  (Patch 2 fixes this problem, too,
> specifically because with it, block_crypto_amend_cleanup() will always
> be called when the job is dismissed, and so updating_keys will be reset
> at least then.)
> 
> Patch 4 fixes an issue that’s not related to “crypto: perform permission
> checks under BQL”, but it became appearent only while debugging the
> other issues here, so it’s part of this series, too.
> 
> Patch 5 fixes the test itself.  It expects permission-related errors to
> occur when the job is already running, not as an immediate result of the
> QMP x-blockdev-amend command.  “crypto: perform permission checks under
> BQL” has changed this, so the test needs to take that into account.
> 
> 
> Ideally, I believe the following patches should be squashed into
> “crypto: perform permission checks under BQL” lest bisect breaks:
> - Patch 1
> - Patch 2 or 3 (or both)
> - Patch 5
> 
> But if that isn’t feasible, we can just take the whole series on top.

Thanks, I'm squashing in 1, 3 and 5 and taking 2 and 4 on top.

Kevin