The Friday 12 Sep 2014 à 21:26:22 (+0200), Markus Armbruster wrote :
> If the BDS's refcnt > 0, drive_del() destroys the DriveInfo, but not
> the BDS. This can happen in three places:
>
> * Device model destruction during unplug: blockdev_auto_del()
>
> * Xen IDE unplug: pci_piix3_xen_ide_unplug()
>
> * drive_del command when no device model is attached: do_drive_del()
>
> The other callers of drive_del are on error paths where refcnt == 1.
>
> If the user somehow manages to plug in a device model using a BDS that
> has gone through drive_del(), the legacy configuration passed in
> DriveInfo doesn't reach the device model, and automatic deletion on
> unplug doesn't work. Worse, some device models such as scsi-disk
> crash when DriveInfo doesn't exist.
>
> This is theoretical; I didn't research an actual reproducer.
>
> Fix by keeping DriveInfo alive until its BDS dies.
>
> This affects qemu_drive_opts: now you can't reuse the same ID for new
> drive options until the BDS dies. Before, you could, but since the
> code always attempts to create a BDS with the same ID next, the
> enclosing operation "create a new drive" failed anyway. Different
> error path, same result.
>
> Unfortunately, the fix involves use of blockdev.c stuff from block.c,
> which is a layering violation. Fortunately, my forthcoming
> BlockBackend work will get rid of it again.
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> block.c | 2 ++
> blockdev.c | 13 ++++++++-----
> include/sysemu/blockdev.h | 1 +
> stubs/Makefile.objs | 1 +
> stubs/blockdev.c | 12 ++++++++++++
> 5 files changed, 24 insertions(+), 5 deletions(-)
> create mode 100644 stubs/blockdev.c
>
> diff --git a/block.c b/block.c
> index d06dd51..6faf36f 100644
> --- a/block.c
> +++ b/block.c
> @@ -29,6 +29,7 @@
> #include "qemu/module.h"
> #include "qapi/qmp/qjson.h"
> #include "sysemu/sysemu.h"
> +#include "sysemu/blockdev.h" /* FIXME layering violation */
> #include "qemu/notify.h"
> #include "block/coroutine.h"
> #include "block/qapi.h"
> @@ -2110,6 +2111,7 @@ static void bdrv_delete(BlockDriverState *bs)
> /* remove from list, if necessary */
> bdrv_make_anon(bs);
>
> + drive_info_del(drive_get_by_blockdev(bs));
> g_free(bs);
> }
>
> diff --git a/blockdev.c b/blockdev.c
> index 5ec4635..450f95c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -216,11 +216,17 @@ static void bdrv_format_print(void *opaque, const char
> *name)
>
> void drive_del(DriveInfo *dinfo)
> {
> + bdrv_unref(dinfo->bdrv);
> +}
> +
> +void drive_info_del(DriveInfo *dinfo)
> +{
> + if (!dinfo) {
> + return;
> + }
> if (dinfo->opts) {
> qemu_opts_del(dinfo->opts);
> }
> -
> - bdrv_unref(dinfo->bdrv);
> g_free(dinfo->id);
> QTAILQ_REMOVE(&drives, dinfo, next);
> g_free(dinfo->serial);
> @@ -525,9 +531,6 @@ static DriveInfo *blockdev_init(const char *file, QDict
> *bs_opts,
>
> err:
> bdrv_unref(bs);
> - QTAILQ_REMOVE(&drives, dinfo, next);
> - g_free(dinfo->id);
> - g_free(dinfo);
> early_err:
> qemu_opts_del(opts);
> err_no_opts:
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 23a5d10..abec381 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -56,6 +56,7 @@ QemuOpts *drive_add(BlockInterfaceType type, int index,
> const char *file,
> const char *optstr);
> DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
> void drive_del(DriveInfo *dinfo);
> +void drive_info_del(DriveInfo *dinfo);
>
> /* device-hotplug */
>
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 5e347d0..c0b1f6a 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -1,5 +1,6 @@
> stub-obj-y += arch-query-cpu-def.o
> stub-obj-y += bdrv-commit-all.o
> +stub-obj-y += blockdev.o
> stub-obj-y += chr-baum-init.o
> stub-obj-y += chr-msmouse.o
> stub-obj-y += chr-testdev.o
> diff --git a/stubs/blockdev.c b/stubs/blockdev.c
> new file mode 100644
> index 0000000..5d0a79c
> --- /dev/null
> +++ b/stubs/blockdev.c
> @@ -0,0 +1,12 @@
> +#include <assert.h>
> +#include "sysemu/blockdev.h"
> +
> +DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
> +{
> + return NULL;
> +}
> +
> +void drive_info_del(DriveInfo *dinfo)
> +{
> + assert(!dinfo);
> +}
> --
> 1.9.3
>
Reviewed-by: Benoît Canet <benoit.ca...@nodalink.com>
