On 24.11.21 07:44, Emanuele Giuseppe Esposito wrote:
block_crypto_amend_options_generic_luks uses the block layer
permission API, therefore it should be called with the BQL held.
However, the same function is being called ib two BlockDriver
s/ ib / by /
callbacks: bdrv_amend_options (under BQL) and bdrv_co_amend (I/O).
The latter is I/O because it is invoked by block/amend.c's
blockdev_amend_run(), a .run callback of the amend JobDriver
Therefore we want to 1) change block_crypto_amend_options_generic_luks
to use the permission API only when the BQL is held, and
2) use the .pre_run JobDriver callback to check for
permissions before switching to the job aiocontext. This has also
the benefit of applying the same permission operation to all
amend implementations, not only luks.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/amend.c | 20 ++++++++++++++++++++
block/crypto.c | 18 ++++++++++++------
2 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/block/amend.c b/block/amend.c
index 392df9ef83..fba6add51a 100644
--- a/block/amend.c
+++ b/block/amend.c
@@ -53,10 +53,30 @@ static int coroutine_fn blockdev_amend_run(Job
*job, Error **errp)
return ret;
}
+static int blockdev_amend_refresh_perms(Job *job, Error **errp)
+{
+ BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
+
+ return bdrv_child_refresh_perms(s->bs, s->bs->file, errp);
+}
I miss some documentation for this function, why we do it and how it
works together with the bdrv_co_amend implementation.
I was trying to come up with an example text, but then I wondered –
how does it actually work? bdrv_child_refresh_perms() eventually ends
up in block_crypto_child_perms(). However, that will only return
exceptional permissions if crypto->updating_keys is true. But that’s
set only in block_crypto_amend_options_generic_luks() – i.e. when the
job runs. That’s exactly why that function calls
bdrv_child_refresh_perms() only after it has modified
crypto->updating_keys.
Reproducer (amend on a LUKS image with read-only=true, so it doesn’t
have the WRITE permission continuously, but needs to take it as an
exception in block_crypto_child_perms()):
$ qemu-img create \
-f luks \
--object secret,id=sec0,data=123456 \
-o key-secret=sec0 \
test.luks \
64M
Formatting 'test.luks', fmt=luks size=67108864 key-secret=sec0
$ ./qemu-system-x86_64 \
-object secret,id=sec0,data=123456 \
-object iothread,id=iothr0 \
-blockdev file,node-name=node0,filename=test.luks \
-blockdev
luks,node-name=node1,key-secret=sec0,file=node0,read-only=true \
-device virtio-blk,drive=node1,iothread=iothr0 -qmp stdio \
<<EOF
{"execute": "qmp_capabilities"}
{
"execute": "x-blockdev-amend",
"arguments": {
"job-id": "amend0",
"node-name": "node1",
"options": {
"driver": "luks",
"state": "active",
"new-secret": "sec0"
}
}
}
EOF
{"QMP": {"version": {"qemu": {"micro": 93, "minor": 1, "major": 6},
"package": "v6.2.0-rc3-50-gdb635fc4e7"}, "capabilities": ["oob"]}}
{"return": {}}
{"timestamp": {"seconds": 1639742600, "microseconds": 574641},
"event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id":
"amend0"}}
{"timestamp": {"seconds": 1639742600, "microseconds": 574919},
"event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id":
"amend0"}}
{"return": {}}
qemu-system-x86_64: ../block/io.c:2041: bdrv_co_write_req_prepare:
Assertion `child->perm & BLK_PERM_WRITE' failed.
[1] 55880 IOT instruction (core dumped) ./qemu-system-x86_64
-object secret,id=sec0,data=123456 -object -blockdev