From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40273 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q4b3G-0004Pe-K4 for qemu-devel@nongnu.org; Tue, 29 Mar 2011 11:41:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q4b3F-0007aw-1i for qemu-devel@nongnu.org; Tue, 29 Mar 2011 11:41:50 -0400 Received: from e3.ny.us.ibm.com ([32.97.182.143]:49636) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q4b3E-0007aY-VE for qemu-devel@nongnu.org; Tue, 29 Mar 2011 11:41:49 -0400 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e3.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p2TFL7gA020331 for ; Tue, 29 Mar 2011 11:21:07 -0400 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 17FBA38C8038 for ; Tue, 29 Mar 2011 11:41:41 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2TFflo9145162 for ; Tue, 29 Mar 2011 11:41:47 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2TFfklv010478 for ; Tue, 29 Mar 2011 12:41:46 -0300 Date: Tue, 29 Mar 2011 10:41:37 -0500 From: Ryan Harper Subject: Re: [Qemu-devel] [PATCH v3] Do not delete BlockDriverState when deleting the drive Message-ID: <20110329154137.GD5445@us.ibm.com> References: <20110328184616.GH20374@us.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , Stefan Hajnoczi , Ryan Harper , qemu-devel@nongnu.org * Markus Armbruster [2011-03-29 02:44]: > Ryan Harper 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 > > Signed-off-by: Ryan Harper > > --- > > 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 ryanh@us.ibm.com