* Markus Armbruster <arm...@redhat.com> [2011-03-15 04:48]:
> Sorry for the long delay, I was out of action for a week.
>
> 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()
> > drive_uninit()
> > bdrv_delete()
> >
> > When we bdrv_delete() we end up qemu_free() the BlockDriverState pointer
> > however, the block devices retain a copy of this pointer, see
> > hw/virtio-blk.c:virtio_blk_init() where we s->bs = conf->bs.
> >
> > We now have a use-after-free situation. If the guest continues to issue IO
> > against the device, and we've reallocated the memory that the
> > BlockDriverState
> > pointed at, then we will fail the bs->drv checks in the various bdrv_
> > methods.
>
> "we will fail the bs->drv checks" is misleading, in my opinion. Here's
> what happens:
>
> 1. bdrv_close(bs) zaps bs->drv, which makes any subsequent I/O get
> dropped. Works as designed.
>
> 2. drive_uninit() 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. I/O crashes or worse once that
> changed. Could be right on free, could be much later.
>
> If you respin anyway, please clarify your description.
Sure. I wasn't planning a new version, but I'll update and send anyhow
as I didn't see it get included in pull from the block branch.
>
> > 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 it's own function, bdrv_remove().
>
> Why do we remove the BlockDriverState from bdrv_states? Because we want
> drive_del make its *name* go away.
>
> Begs the question: is the code prepared for a BlockDriverState object
> that isn't on bdrv_states? Turns out we're in luck: bdrv_new() already
> creates such objects when the device_name is empty. This is used for
> internal BlockDriverStates such as COW backing files. Your code makes
> device_name empty when taking the object off bdrv_states, so we're good.
>
> Begs yet another question: how does the behavior of a BlockDriverState
> change when it's taken off bdrv_states, and is that the behavior we
> want? Changes:
>
> * bdrv_delete() no longer takes it off bdrv_states. Good.
>
> * bdrv_close_all(), bdrv_commit_all() and bdrv_flush_all() no longer
> cover it. Okay, because bdrv_close(), bdrv_commit() and bdrv_flush()
> do nothing anyway for closed BlockDriverStates.
>
> * "info block" and "info blockstats" no longer show it, because
> bdrv_info() and bdrv_info_stats() no longer see it. Okay.
>
> * bdrv_find(), bdrv_next(), bdrv_iterate() no longer see it. Impact?
> Please check their uses and report.
1 664 block-migration.c <<block_load>>
bs = bdrv_find(device_name);
- no longer see it. This is fine since we can't migrate a block
device that has been removed
2 562 blockdev.c <<do_commit>>
bs = bdrv_find(device);
- do_commit won't see it in either when calling bdrv_commit_all()
Fine as you mention above. If user specifies the device name
we won't find it, that's OK because we can't commit data against
a closed BlockDriverState.
3 587 blockdev.c <<do_snapshot_blkdev>>
bs = bdrv_find(device);
- OK, cannot take a snapshot against a deleted BlockDriverState
4 662 blockdev.c <<do_eject>>
bs = bdrv_find(filename);
- OK, cannot eject a deleted BlockDriverState;
5 676 blockdev.c <<do_block_set_passwd>>
bs = bdrv_find(qdict_get_str(qdict, "device"));
- OK, cannot set password a deleted BlockDriverState;
6 701 blockdev.c <<do_change_block>>
bs = bdrv_find(device);
- OK, cannot change the file/device of a deleted BlockDriverState;
7 732 blockdev.c <<do_drive_del>>
bs = bdrv_find(id);
- OK, cannot delete an already deleted Drive
8 783 blockdev.c <<do_block_resize>>
bs = bdrv_find(device);
- OK, cannot resize a deleted Drive
9 312 hw/qdev-properties.c <<parse_drive>>
bs = bdrv_find(str);
- Used when invoking qdev_prop_drive .parse method; parse method is invoked
via
qdev_device_add() which calls set_property() which invokes parse. AFAICT,
this is OK
since we won't be going down the device add path worrying about a
deleted block device.
>
> > 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.
>
> Yep. But there's one more question: is the BlockDriverState freed when
> the device using it gets destroyed?
>
> qdev_free() runs prop->info->free() for all properties. The drive
> property's free() is free_drive():
>
> static void free_drive(DeviceState *dev, Property *prop)
> {
> BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>
> if (*ptr) {
> bdrv_detach(*ptr, dev);
> blockdev_auto_del(*ptr);
> }
> }
>
> This should indeed delete the drive. But only if the property still
> points to it. See below.
>
> > Reported-by: Marcus Armbruster <arm...@redhat.com>
> > Signed-off-by: Ryan Harper <ry...@us.ibm.com>
> > ---
> > v1->v2
> > - NULL bs->device_name after removing from list to prevent
> > second removal.
> >
> > block.c | 12 +++++++++---
> > block.h | 1 +
> > blockdev.c | 2 +-
> > 3 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 1544d81..0df9942 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -697,14 +697,20 @@ void bdrv_close_all(void)
> > }
> > }
> >
> > +void bdrv_remove(BlockDriverState *bs)
> > +{
> > + if (bs->device_name[0] != '\0') {
> > + QTAILQ_REMOVE(&bdrv_states, bs, list);
> > + }
> > + bs->device_name[0] = '\0';
> > +}
> > +
>
> I don't like this name. What's the difference between "delete" and
> "remove"?
>
> The function zaps the device name. bdrv_make_anon()?
bdrv_delist? bdrv_hide? I'm also fine with make_anon.
>
> > 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_remove(bs);
> >
> > bdrv_close(bs);
> > if (bs->file != NULL) {
> > diff --git a/block.h b/block.h
> > index 5d78fc0..8447397 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_remove(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 0690cc8..1f57b50 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -760,7 +760,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict,
> > QObject **ret_data)
>
> Let me add a bit more context:
>
> bdrv_flush(bs);
> bdrv_close(bs);
>
> /* clean up guest state from pointing to host resource by
> * finding and removing DeviceState "drive" property */
> 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;
> }
> }
> }
> > }
>
> This zaps the drive property. A subsequent free_drive() will do
> nothing. We leak the BlockDriverState on device unplug, I'm afraid.
>
> Any reason why we still want to zap the drive property?
IIRC, it was to prevent qdev from keeping a ptr around to the bs; but if
we're keeping the bs around anyhow, then I don't think we need to remove
the property. One last check would be to see what if the device still
shows up in qtree if we don't remove the property.
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ry...@us.ibm.com
