On Wed, Sep 24, 2025 at 06:24:20PM +0200, Kevin Wolf wrote:
> 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é <berra...@redhat.com>
> > ---
> >
> > 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.
Great.
> > 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.
We don't store the function pointers anywhere - they must be passed in
from the context that needs to use them, so the calling codepaths are
an accurate reflection of the the use context.
> qemu-iotests 295 passes for qcow2 while it crashed for luks, so that's
> probably another indication that the qcow2 code is okay.
Yep, that's good.
>
> > block/crypto.c | 30 ++++++++++++++++++++++--------
> > 1 file changed, 22 insertions(+), 8 deletions(-)
>
> Thanks, applied to the block branch.
>
> Kevin
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
