From d8fbf9aa85aed64450907580a1d70583f097e9df Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 27 Mar 2023 13:39:59 +0200 Subject: [PATCH] block/export: Fix graph locking in blk_get_geometry() call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit blk_get_geometry() eventually calls bdrv_nb_sectors(), which is a co_wrapper_mixed_bdrv_rdlock. This means that when it is called from coroutine context, it already assume to have the graph locked. However, virtio_blk_sect_range_ok() in block/export/virtio-blk-handler.c (used by vhost-user-blk and VDUSE exports) runs in a coroutine, but doesn't take the graph lock - blk_*() functions are generally expected to do that internally. This causes an assertion failure when accessing an export for the first time if it runs in an iothread. This is an example of the crash: $ ./storage-daemon/qemu-storage-daemon --object iothread,id=th0 --blockdev file,filename=/home/kwolf/images/hd.img,node-name=disk --export vhost-user-blk,addr.type=unix,addr.path=/tmp/vhost.sock,node-name=disk,id=exp0,iothread=th0 qemu-storage-daemon: ../block/graph-lock.c:268: void assert_bdrv_graph_readable(void): Assertion `qemu_in_main_thread() || reader_count()' failed. (gdb) bt #0 0x00007ffff6eafe5c in __pthread_kill_implementation () from /lib64/libc.so.6 #1 0x00007ffff6e5fa76 in raise () from /lib64/libc.so.6 #2 0x00007ffff6e497fc in abort () from /lib64/libc.so.6 #3 0x00007ffff6e4971b in __assert_fail_base.cold () from /lib64/libc.so.6 #4 0x00007ffff6e58656 in __assert_fail () from /lib64/libc.so.6 #5 0x00005555556337a3 in assert_bdrv_graph_readable () at ../block/graph-lock.c:268 #6 0x00005555555fd5a2 in bdrv_co_nb_sectors (bs=0x5555564c5ef0) at ../block.c:5847 #7 0x00005555555ee949 in bdrv_nb_sectors (bs=0x5555564c5ef0) at block/block-gen.c:256 #8 0x00005555555fd6b9 in bdrv_get_geometry (bs=0x5555564c5ef0, nb_sectors_ptr=0x7fffef7fedd0) at ../block.c:5884 #9 0x000055555562ad6d in blk_get_geometry (blk=0x5555564cb200, nb_sectors_ptr=0x7fffef7fedd0) at ../block/block-backend.c:1624 #10 0x00005555555ddb74 in virtio_blk_sect_range_ok (blk=0x5555564cb200, block_size=512, sector=0, size=512) at ../block/export/virtio-blk-handler.c:44 #11 0x00005555555dd80d in virtio_blk_process_req (handler=0x5555564cbb98, in_iov=0x7fffe8003830, out_iov=0x7fffe8003860, in_num=1, out_num=0) at ../block/export/virtio-blk-handler.c:189 #12 0x00005555555dd546 in vu_blk_virtio_process_req (opaque=0x7fffe8003800) at ../block/export/vhost-user-blk-server.c:66 #13 0x00005555557bf4a1 in coroutine_trampoline (i0=-402635264, i1=32767) at ../util/coroutine-ucontext.c:177 #14 0x00007ffff6e75c20 in ?? () from /lib64/libc.so.6 #15 0x00007fffefffa870 in ?? () #16 0x0000000000000000 in ?? () Fix this by creating a new blk_co_get_geometry() that takes the lock, and changing blk_get_geometry() to be a co_wrapper_mixed around it. To make the resulting code cleaner, virtio-blk-handler.c can directly call the coroutine version now (though that wouldn't be necessary for fixing the bug, taking the lock in blk_co_get_geometry() is what fixes it). Fixes: 8ab8140a04cf771d63e9754d6ba6c1e676bfe507 Reported-by: Lukáš Doktor Signed-off-by: Kevin Wolf Message-Id: <20230327113959.60071-1-kwolf@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf --- block.c | 5 +++-- block/block-backend.c | 7 +++++-- block/export/virtio-blk-handler.c | 7 ++++--- include/block/block-io.h | 4 +++- include/sysemu/block-backend-io.h | 5 ++++- 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/block.c b/block.c index 0dd604d0f6a..e0c6c648b12 100644 --- a/block.c +++ b/block.c @@ -5879,9 +5879,10 @@ int64_t coroutine_fn bdrv_co_getlength(BlockDriverState *bs) } /* return 0 as number of sectors if no device present or error */ -void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr) +void coroutine_fn bdrv_co_get_geometry(BlockDriverState *bs, + uint64_t *nb_sectors_ptr) { - int64_t nb_sectors = bdrv_nb_sectors(bs); + int64_t nb_sectors = bdrv_co_nb_sectors(bs); IO_CODE(); *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors; diff --git a/block/block-backend.c b/block/block-backend.c index 278b04ce69a..2ee39229e4c 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1615,13 +1615,16 @@ int64_t coroutine_fn blk_co_getlength(BlockBackend *blk) return bdrv_co_getlength(blk_bs(blk)); } -void blk_get_geometry(BlockBackend *blk, uint64_t *nb_sectors_ptr) +void coroutine_fn blk_co_get_geometry(BlockBackend *blk, + uint64_t *nb_sectors_ptr) { IO_CODE(); + GRAPH_RDLOCK_GUARD(); + if (!blk_bs(blk)) { *nb_sectors_ptr = 0; } else { - bdrv_get_geometry(blk_bs(blk), nb_sectors_ptr); + bdrv_co_get_geometry(blk_bs(blk), nb_sectors_ptr); } } diff --git a/block/export/virtio-blk-handler.c b/block/export/virtio-blk-handler.c index 313666e8ab9..bc1cec67570 100644 --- a/block/export/virtio-blk-handler.c +++ b/block/export/virtio-blk-handler.c @@ -22,8 +22,9 @@ struct virtio_blk_inhdr { unsigned char status; }; -static bool virtio_blk_sect_range_ok(BlockBackend *blk, uint32_t block_size, - uint64_t sector, size_t size) +static bool coroutine_fn +virtio_blk_sect_range_ok(BlockBackend *blk, uint32_t block_size, + uint64_t sector, size_t size) { uint64_t nb_sectors; uint64_t total_sectors; @@ -41,7 +42,7 @@ static bool virtio_blk_sect_range_ok(BlockBackend *blk, uint32_t block_size, if ((sector << VIRTIO_BLK_SECTOR_BITS) % block_size) { return false; } - blk_get_geometry(blk, &total_sectors); + blk_co_get_geometry(blk, &total_sectors); if (sector > total_sectors || nb_sectors > total_sectors - sector) { return false; } diff --git a/include/block/block-io.h b/include/block/block-io.h index 5da99d4d60f..dbc034b7288 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -89,7 +89,9 @@ int64_t co_wrapper bdrv_get_allocated_file_size(BlockDriverState *bs); BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts, BlockDriverState *in_bs, Error **errp); -void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr); + +void coroutine_fn GRAPH_RDLOCK +bdrv_co_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr); int coroutine_fn GRAPH_RDLOCK bdrv_co_delete_file(BlockDriverState *bs, Error **errp); diff --git a/include/sysemu/block-backend-io.h b/include/sysemu/block-backend-io.h index 40ab1787192..c672b772472 100644 --- a/include/sysemu/block-backend-io.h +++ b/include/sysemu/block-backend-io.h @@ -70,7 +70,10 @@ void co_wrapper blk_eject(BlockBackend *blk, bool eject_flag); int64_t coroutine_fn blk_co_getlength(BlockBackend *blk); int64_t co_wrapper_mixed blk_getlength(BlockBackend *blk); -void blk_get_geometry(BlockBackend *blk, uint64_t *nb_sectors_ptr); +void coroutine_fn blk_co_get_geometry(BlockBackend *blk, + uint64_t *nb_sectors_ptr); +void co_wrapper_mixed blk_get_geometry(BlockBackend *blk, + uint64_t *nb_sectors_ptr); int64_t coroutine_fn blk_co_nb_sectors(BlockBackend *blk); int64_t co_wrapper_mixed blk_nb_sectors(BlockBackend *blk);