diff mbox

Do not delete BlockDriverState when deleting the drive

Message ID 20110302150342.GZ13257@us.ibm.com
State New
Headers show

Commit Message

Ryan Harper March 2, 2011, 3:03 p.m. UTC 
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()'ing 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.

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().

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.  This state will be deleted if the guest
is asked and responds to a pci device removal request.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
---
 block.c    |   11 ++++++++---
 block.h    |    1 +
 blockdev.c |    2 +-
 3 files changed, 10 insertions(+), 4 deletions(-)

Comments

Stefan Hajnoczi March 3, 2011, 10:19 a.m. UTC | #1 
On Wed, Mar 2, 2011 at 3:03 PM, Ryan Harper <ryanh@us.ibm.com> wrote:
> +void bdrv_remove(BlockDriverState *bs)
> +{
> +    if (bs->device_name[0] != '\0') {
> +        QTAILQ_REMOVE(&bdrv_states, bs, list);
> +    }
> +}

It's not safe to invoke QTAILQ_REMOVE() twice.  Since both
do_drive_del() and bdrv_delete() call bdrv_remove(), could it be
invoked twice?  A quick fix is to set bs->device_name[0] = '\0' to
prevent the second removal.

Stefan
diff mbox

Patch

diff --git a/block.c b/block.c
index f7d91a2..92dd3fe 100644
--- a/block.c
+++ b/block.c
@@ -697,14 +697,19 @@  void bdrv_close_all(void)
     }
 }
 
+void bdrv_remove(BlockDriverState *bs)
+{
+    if (bs->device_name[0] != '\0') {
+        QTAILQ_REMOVE(&bdrv_states, bs, list);
+    }
+}
+
 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)
     }
 
     /* clean up host side */
-    drive_uninit(drive_get_by_blockdev(bs));
+    bdrv_remove(bs);
 
     return 0;
 }