1. Patchew
  2. QEMU
  3. View series

[PATCH] block: fix luks 'amend' when run in coroutine

Daniel P. Berrangé posted 1 patch 4 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250919112213.1530079-1-berrange@redhat.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
block/crypto.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
[PATCH] block: fix luks 'amend' when run in coroutine
Posted by Daniel P. Berrangé 4 months, 3 weeks ago 
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é <berrange@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.

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).

 block/crypto.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index d4226cc68a..a1f8d24df1 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -67,11 +67,18 @@ static int block_crypto_read_func(QCryptoBlock *block,
     BlockCrypto *crypto = bs->opaque;
     ssize_t ret;
 
-    GLOBAL_STATE_CODE();
-    GRAPH_RDLOCK_GUARD_MAINLOOP();
+    if (qemu_in_coroutine()) {
+        GRAPH_RDLOCK_GUARD();
 
-    ret = bdrv_pread(crypto->header ? crypto->header : bs->file,
-                     offset, buflen, buf, 0);
+        ret = bdrv_co_pread(crypto->header ? crypto->header : bs->file,
+                            offset, buflen, buf, 0);
+    } else {
+        GLOBAL_STATE_CODE();
+        GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+        ret = bdrv_pread(crypto->header ? crypto->header : bs->file,
+                         offset, buflen, buf, 0);
+    }
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not read encryption header");
         return ret;
@@ -90,11 +97,18 @@ static int block_crypto_write_func(QCryptoBlock *block,
     BlockCrypto *crypto = bs->opaque;
     ssize_t ret;
 
-    GLOBAL_STATE_CODE();
-    GRAPH_RDLOCK_GUARD_MAINLOOP();
+    if (qemu_in_coroutine()) {
+        GRAPH_RDLOCK_GUARD();
 
-    ret = bdrv_pwrite(crypto->header ? crypto->header : bs->file,
-                      offset, buflen, buf, 0);
+        ret = bdrv_co_pwrite(crypto->header ? crypto->header : bs->file,
+                             offset, buflen, buf, 0);
+    } else {
+        GLOBAL_STATE_CODE();
+        GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+        ret = bdrv_pwrite(crypto->header ? crypto->header : bs->file,
+                          offset, buflen, buf, 0);
+    }
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not write encryption header");
         return ret;
-- 
2.50.1
Re: [PATCH] block: fix luks 'amend' when run in coroutine
Posted by Kevin Wolf 4 months, 2 weeks ago 
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é <berrange@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.

> 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
Re: [PATCH] block: fix luks 'amend' when run in coroutine
Posted by Daniel P. Berrangé 4 months, 2 weeks ago 
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é <berrange@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 :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.