Re: [Qemu-devel] [PATCH v3] Do not delete BlockDriverState when deleting the drive

[Markus Armbruster]

Ryan Harper <address@hidden> writes:

> When removing a drive from the host-side via drive_del we currently have the
> following path:
>
> drive_del
> qemu_aio_flush()
> bdrv_close()    // zaps bs->drv, which makes any subsequent I/O get
>                 // dropped.  Works as designed
> drive_uninit()
> bdrv_delete()   // frees the bs.  Since the device is still connected to
>                 // bs, any subsequent I/O is a use-after-free.
>
> The value of bs->drv becomes unpredictable on free.  As long as it
> remains null, I/O still gets dropped, however it could become non-null at any
> point after the free resulting SEGVs or other QEMU state corruption.
>
> To resolve this issue as simply as possible, we can chose to not actually
> delete the BlockDriverState pointer.  Since bdrv_close() handles setting the 
> drv
> pointer to NULL, we just need to remove the BlockDriverState from the QLIST
> that is used to enumerate the block devices.  This is currently handled within
> bdrv_delete, so move this into its own function, bdrv_make_anon().
>
> The result is that we can now invoke drive_del, this closes the file 
> descriptors
> and sets BlockDriverState->drv to NULL which prevents futher IO to the device,
> and since we do not free BlockDriverState, we don't have to worry about the 
> copy
> retained in the block devices.
>
> We also don't attempt to remove the qdev property since we are no longer 
> deleting
> the BlockDriverState on drives with associated drives.  This also allows for
> removing Drives with no devices associated either.
>
> Reported-by: Markus Armbruster <address@hidden>
> Signed-off-by: Ryan Harper <address@hidden>
> ---
> v2->v3
>  - Update drive_del use after free description
>  - s/bdrv_remove/bdrv_make_anon/g
>  - Don't remove qdev property since we don't delete bs any more
>  - If (bs->peer) bdrv_make_anon else bdrv_delete to handle removing
>    drives with no device.
> v1->v2
>   - NULL bs->device_name after removing from list to prevent
>     second removal.
>
>  block.c    |   13 ++++++++++---
>  block.h    |    1 +
>  blockdev.c |   25 ++++++++-----------------
>  3 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/block.c b/block.c
> index c8e2f97..6a5d3f2 100644
> --- a/block.c
> +++ b/block.c
> @@ -697,14 +697,21 @@ void bdrv_close_all(void)
>      }
>  }
>  
> +/* make a BlockDriverState anonymous by removing from bdrv_state list.
> +   Also, NULL terminate the device_name to prevent double remove */
> +void bdrv_make_anon(BlockDriverState *bs)
> +{
> +    if (bs->device_name[0] != '\0') {
> +        QTAILQ_REMOVE(&bdrv_states, bs, list);
> +    }

You lost

+    bs->device_name[0] = '\0';

since v2.  Oops.

> +}
> +
>  void bdrv_delete(BlockDriverState *bs)
>  {
>      assert(!bs->peer);
>  
>      /* remove from list, if necessary */
> -    if (bs->device_name[0] != '\0') {
> -        QTAILQ_REMOVE(&bdrv_states, bs, list);
> -    }
> +    bdrv_make_anon(bs);
>  
>      bdrv_close(bs);
>      if (bs->file != NULL) {
> diff --git a/block.h b/block.h
> index 5d78fc0..52e9cad 100644
> --- a/block.h
> +++ b/block.h
> @@ -66,6 +66,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>      QEMUOptionParameter *options);
>  int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
>  BlockDriverState *bdrv_new(const char *device_name);
> +void bdrv_make_anon(BlockDriverState *bs);
>  void bdrv_delete(BlockDriverState *bs);
>  int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
>  int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
> diff --git a/blockdev.c b/blockdev.c
> index ecf2252..2c0eb06 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -737,8 +737,6 @@ int do_drive_del(Monitor *mon, const QDict *qdict, 
> QObject **ret_data)
>  {
>      const char *id = qdict_get_str(qdict, "id");
>      BlockDriverState *bs;
> -    BlockDriverState **ptr;
> -    Property *prop;
>  
>      bs = bdrv_find(id);
>      if (!bs) {
> @@ -755,24 +753,17 @@ int do_drive_del(Monitor *mon, const QDict *qdict, 
> QObject **ret_data)
>      bdrv_flush(bs);
>      bdrv_close(bs);
>  
> -    /* clean up guest state from pointing to host resource by
> -     * finding and removing DeviceState "drive" property */
> +    /* if we have a device associated with this BlockDriverState (bs->peer)
> +     * then we need to make the drive anonymous until the device
> +     * can be removed.  If this is a drive with no device backing
> +     * then we can just get rid of the block driver state right here.
> +     */
>      if (bs->peer) {
> -        for (prop = bs->peer->info->props; prop && prop->name; prop++) {
> -            if (prop->info->type == PROP_TYPE_DRIVE) {
> -                ptr = qdev_get_prop_ptr(bs->peer, prop);
> -                if (*ptr == bs) {
> -                    bdrv_detach(bs, bs->peer);
> -                    *ptr = NULL;
> -                    break;
> -                }
> -            }
> -        }
> +        bdrv_make_anon(bs);
> +    } else {
> +        bdrv_delete(bs);

Don't you need drive_uninit() here?

>      }
>  
> -    /* clean up host side */
> -    drive_uninit(drive_get_by_blockdev(bs));
> -
>      return 0;
>  }
>  
> -- 
> 1.7.1