* Markus Armbruster <arm...@redhat.com> [2011-03-29 02:44]:
> Ryan Harper <ry...@us.ibm.com> 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 <arm...@redhat.com>
> > Signed-off-by: Ryan Harper <ry...@us.ibm.com>
> > ---
> > 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.
Crap.
>
> > +}
> > +
> > 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?
You're right. We don't need it if we have a device associated since the
qdev cleanup code on device removal will remove it (via auto_delete).
I'll fix that up as well.
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ry...@us.ibm.com
