[2/4] block: Keep DriveInfo alive until BlockDriverState dies

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 <armbru@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

On 12.09.2014 21:26, 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 <armbru@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

Seems reasonable.

Reviewed-by: Max Reitz <mreitz@redhat.com>

Markus Armbruster <armbru@redhat.com> writes:

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

Broken when we replaced DriveInfo reference counting by BDS reference
counting in commit a94a3fa..fa510eb.

Kevin, would you mind inserting that into the commit message?

> 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 <armbru@redhat.com>
[...]

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 <armbru@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.canet@nodalink.com>