Re: [PATCH for-8.0] block/export: Fix graph locking in blk_get_geometry() call

[Emanuele Giuseppe Esposito]

Am 27/03/2023 um 13:39 schrieb Kevin Wolf:
> 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
> 
> 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 <ldoktor@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

> ---
>  include/block/block-io.h          | 4 +++-
>  include/sysemu/block-backend-io.h | 5 ++++-
>  block.c                           | 5 +++--
>  block/block-backend.c             | 7 +++++--
>  block/export/virtio-blk-handler.c | 7 ++++---
>  5 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/include/block/block-io.h b/include/block/block-io.h
> index 5da99d4d60..dbc034b728 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 40ab178719..c672b77247 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);
> diff --git a/block.c b/block.c
> index 0dd604d0f6..e0c6c648b1 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 278b04ce69..2ee39229e4 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 313666e8ab..bc1cec6757 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;
>      }