Am 19.09.2025 um 13:22 hat Daniel P. Berrangé geschrieben:
> Launch QEMU with
>
> $ qemu-img create \
> --object secret,id=sec0,data=123456 \
> -f luks -o key-secret=sec0 demo.luks 1g
>
> $ qemu-system-x86_64 \
> --object secret,id=sec0,data=123456 \
> -blockdev
> driver=luks,key-secret=sec0,file.filename=demo.luks,file.driver=file,node-name=luks
>
> Then in QMP shell attempt
>
> x-blockdev-amend job-id=fish node-name=luks
> options={'state':'active','new-secret':'sec0','driver':'luks'}
>
> It will result in an assertion
>
> #0 __pthread_kill_implementation (threadid=<optimized out>,
> signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
> #1 0x00007fad18b73f63 in __pthread_kill_internal (threadid=<optimized
> out>, signo=6) at pthread_kill.c:89
> #2 0x00007fad18b19f3e in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #3 0x00007fad18b016d0 in __GI_abort () at abort.c:77
> #4 0x00007fad18b01639 in __assert_fail_base
> (fmt=<optimized out>, assertion=<optimized out>, file=<optimized out>,
> line=<optimized out>, function=<optimized out>) at assert.c:118
> #5 0x00007fad18b120af in __assert_fail (assertion=<optimized out>,
> file=<optimized out>, line=<optimized out>, function=<optimized out>)
> at assert.c:127
> #6 0x000055ff74fdbd46 in bdrv_graph_rdlock_main_loop () at
> ../block/graph-lock.c:260
> #7 0x000055ff7548521b in graph_lockable_auto_lock_mainloop (x=<optimized
> out>)
> at
> /usr/src/debug/qemu-9.2.4-1.fc42.x86_64/include/block/graph-lock.h:266
> #8 block_crypto_read_func (block=<optimized out>, offset=4096,
> buf=0x55ffb6d66ef0 "", buflen=256000, opaque=0x55ffb5edcc30,
> errp=0x55ffb6f00700)
> at ../block/crypto.c:71
> #9 0x000055ff75439f8b in qcrypto_block_luks_load_key
> (block=block@entry=0x55ffb5edbe90, slot_idx=slot_idx@entry=0,
> password=password@entry=0x55ffb67dc260 "123456",
> masterkey=masterkey@entry=0x55ffb5fb0c40 "",
> readfunc=readfunc@entry=0x55ff754851e0 <block_crypto_read_func>,
> opaque=opaque@entry=0x55ffb5edcc30, errp=0x55ffb6f00700)
> at ../crypto/block-luks.c:927
> #10 0x000055ff7543b90f in qcrypto_block_luks_find_key
> (block=<optimized out>, password=<optimized out>, masterkey=<optimized
> out>, readfunc=<optimized out>, opaque=<optimized out>, errp=<optimized out>)
> at ../crypto/block-luks.c:1045
> #11 qcrypto_block_luks_amend_add_keyslot
> (block=0x55ffb5edbe90, readfunc=0x55ff754851e0
> <block_crypto_read_func>, writefunc=0x55ff75485100 <block_crypto_write_func>,
> opaque=0x55ffb5edcc3, opts_luks=0x7fad1715aef8, force=<optimized out>,
> errp=0x55ffb6f00700) at ../crypto/block-luks.c:1673
> #12 qcrypto_block_luks_amend_options
> (block=0x55ffb5edbe90, readfunc=0x55ff754851e0
> <block_crypto_read_func>, writefunc=0x55ff75485100 <block_crypto_write_func>,
> opaque=0x55ffb5edcc30, options=0x7fad1715aef0, force=<optimized out>,
> errp=0x55ffb6f00700) at ../crypto/block-luks.c:1865
> #13 0x000055ff75485b95 in block_crypto_amend_options_generic_luks
> (bs=<optimized out>, amend_options=<optimized out>, force=<optimized
> out>, errp=<optimized out>) at ../block/crypto.c:949
> #14 0x000055ff75485c28 in block_crypto_co_amend_luks (bs=<optimized out>,
> opts=<optimized out>, force=<optimized out>, errp=<optimized out>)
> at ../block/crypto.c:1008
> #15 0x000055ff754778e5 in blockdev_amend_run (job=0x55ffb6f00640,
> errp=0x55ffb6f00700) at ../block/amend.c:52
> #16 0x000055ff75468b90 in job_co_entry (opaque=0x55ffb6f00640) at
> ../job.c:1106
> #17 0x000055ff755a0fc2 in coroutine_trampoline (i0=<optimized out>,
> i1=<optimized out>) at ../util/coroutine-ucontext.c:175
>
> This changes the read/write callbacks to not assert that they
> are run in mainloop context if already in a coroutine.
>
> Fixes: 1f051dcbdf2e4b6f518db731c84e304b2b9d15ce
> Signed-off-by: Daniel P. Berrangé <[email protected]>
> ---
>
> I'm not really sure if this is correct or not, as the docs on the
> graph lock usage are not very clear on what I should be doing.
It looks right to me, this should do the trick.
> Also I'm wondering if qcow2.c needs work too. When 1f051dc added
> the GRAPH_RDLOCK_GUARD_MAINLOOP in read_func/write_func for the
> crypto.c file, it did not do the same with the read_func/write_func
> for the qcow2.c file, despite them being used from the same call
> paths AFAICT (both image creation & amend).
I wondered why we need the GRAPH_RDLOCK_GUARD_MAINLOOP() at all, because
bdrv_pread() doesn't require it. Just removing it and trying to compile
gave me the answer: It's for accessing bs->file.
qcow2 needs the lock, too, but it's marked GRAPH_RDLOCK. So we (or in
fact probably I myself) decided that all callers already hold the lock.
Does QCrypto store these function pointers somewhere or are they only
for the functions that take them as a parameter? All calls to
qcrypto_block_open() and qcrypto_block_amend_options() in qcow2 are from
places that are in a coroutine and already hold the lock. So I don't see
an obvious problem at least.
qemu-iotests 295 passes for qcow2 while it crashed for luks, so that's
probably another indication that the qcow2 code is okay.
> block/crypto.c | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
Thanks, applied to the block branch.
Kevin
