Patchwork [09/12] savevm: Survive hot-unplug of snapshot device

login
register
mail settings
Submitter Markus Armbruster
Date June 25, 2010, 4:53 p.m.
Message ID <1277484812-22012-10-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/56962/
State New
Headers show

Comments

Markus Armbruster - June 25, 2010, 4:53 p.m.
savevm.c keeps a pointer to the snapshot block device.  If you manage
to get that device deleted, the pointer dangles, and the next snapshot
operation will crash & burn.  Unplugging a guest device that uses it
does the trick:

    $ MALLOC_PERTURB_=234 qemu-system-x86_64 [...]
    QEMU 0.12.50 monitor - type 'help' for more information
    (qemu) info snapshots
    No available block device supports snapshots
    (qemu) drive_add auto if=none,file=tmp.qcow2
    OK
    (qemu) device_add usb-storage,id=foo,drive=none1
    (qemu) info snapshots
    Snapshot devices: none1
    Snapshot list (from none1):
    ID        TAG                 VM SIZE                DATE       VM CLOCK
    (qemu) device_del foo
    (qemu) info snapshots
    Snapshot devices:
    Segmentation fault (core dumped)

Move management of that pointer to block.c, and zap it when the device
it points to goes away.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c  |   25 +++++++++++++++++++++++++
 block.h  |    1 +
 savevm.c |   31 ++++---------------------------
 3 files changed, 30 insertions(+), 27 deletions(-)
Christoph Hellwig - June 26, 2010, 10:12 a.m.
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Of course specifying an explicit medium for snapshot, be that the
snapshot section of a qcow2 image or just a separate flat file and
managing that one explicitly would be even better.
Markus Armbruster - June 30, 2010, 11:28 a.m.
Christoph Hellwig <hch@lst.de> writes:

> Looks good,
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> Of course specifying an explicit medium for snapshot, be that the
> snapshot section of a qcow2 image or just a separate flat file and
> managing that one explicitly would be even better.

Indeed.
Kevin Wolf - June 30, 2010, 1:37 p.m.
Am 25.06.2010 18:53, schrieb Markus Armbruster:
> savevm.c keeps a pointer to the snapshot block device.  If you manage
> to get that device deleted, the pointer dangles, and the next snapshot
> operation will crash & burn.  Unplugging a guest device that uses it
> does the trick:
> 
>     $ MALLOC_PERTURB_=234 qemu-system-x86_64 [...]
>     QEMU 0.12.50 monitor - type 'help' for more information
>     (qemu) info snapshots
>     No available block device supports snapshots
>     (qemu) drive_add auto if=none,file=tmp.qcow2
>     OK
>     (qemu) device_add usb-storage,id=foo,drive=none1
>     (qemu) info snapshots
>     Snapshot devices: none1
>     Snapshot list (from none1):
>     ID        TAG                 VM SIZE                DATE       VM CLOCK
>     (qemu) device_del foo
>     (qemu) info snapshots
>     Snapshot devices:
>     Segmentation fault (core dumped)
> 
> Move management of that pointer to block.c, and zap it when the device
> it points to goes away.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c  |   25 +++++++++++++++++++++++++
>  block.h  |    1 +
>  savevm.c |   31 ++++---------------------------
>  3 files changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 5e0ffa0..34055e0 100644
> --- a/block.c
> +++ b/block.c
> @@ -63,6 +63,9 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
>  static QLIST_HEAD(, BlockDriver) bdrv_drivers =
>      QLIST_HEAD_INITIALIZER(bdrv_drivers);
>  
> +/* The device to use for VM snapshots */
> +static BlockDriverState *bs_snapshots;
> +
>  /* If non-zero, use only whitelisted block drivers */
>  static int use_bdrv_whitelist;
>  
> @@ -660,6 +663,9 @@ void bdrv_close_all(void)
>  void bdrv_delete(BlockDriverState *bs)
>  {
>      assert(!bs->peer);
> +    if (bs == bs_snapshots) {
> +        bs_snapshots = NULL;
> +    }

This should probably be in bdrv_close() instead. A BlockDriverState can
be closed, but not deleted yet; it can't handle snapshots in this state,
though.

>      /* remove from list, if necessary */
>      if (bs->device_name[0] != '\0') {
> @@ -1772,6 +1778,25 @@ int bdrv_can_snapshot(BlockDriverState *bs)
>      return 1;
>  }
>  
> +BlockDriverState *bdrv_snapshots(void)
> +{
> +    BlockDriverState *bs;
> +
> +    if (bs_snapshots)
> +        return bs_snapshots;

I know that this function is just moved with no changes, but while we're
at it and you need to respin anyway, can we add braces here?

> +
> +    bs = NULL;
> +    while ((bs = bdrv_next(bs))) {
> +        if (bdrv_can_snapshot(bs)) {
> +            goto ok;
> +        }
> +    }
> +    return NULL;
> + ok:
> +    bs_snapshots = bs;
> +    return bs;
> +}

And instead of a goto we could do the right thing directly in the if block.

Kevin
Markus Armbruster - June 30, 2010, 4:34 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 25.06.2010 18:53, schrieb Markus Armbruster:
>> savevm.c keeps a pointer to the snapshot block device.  If you manage
>> to get that device deleted, the pointer dangles, and the next snapshot
>> operation will crash & burn.  Unplugging a guest device that uses it
>> does the trick:
>> 
>>     $ MALLOC_PERTURB_=234 qemu-system-x86_64 [...]
>>     QEMU 0.12.50 monitor - type 'help' for more information
>>     (qemu) info snapshots
>>     No available block device supports snapshots
>>     (qemu) drive_add auto if=none,file=tmp.qcow2
>>     OK
>>     (qemu) device_add usb-storage,id=foo,drive=none1
>>     (qemu) info snapshots
>>     Snapshot devices: none1
>>     Snapshot list (from none1):
>>     ID        TAG                 VM SIZE                DATE       VM CLOCK
>>     (qemu) device_del foo
>>     (qemu) info snapshots
>>     Snapshot devices:
>>     Segmentation fault (core dumped)
>> 
>> Move management of that pointer to block.c, and zap it when the device
>> it points to goes away.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block.c  |   25 +++++++++++++++++++++++++
>>  block.h  |    1 +
>>  savevm.c |   31 ++++---------------------------
>>  3 files changed, 30 insertions(+), 27 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 5e0ffa0..34055e0 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -63,6 +63,9 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
>>  static QLIST_HEAD(, BlockDriver) bdrv_drivers =
>>      QLIST_HEAD_INITIALIZER(bdrv_drivers);
>>  
>> +/* The device to use for VM snapshots */
>> +static BlockDriverState *bs_snapshots;
>> +
>>  /* If non-zero, use only whitelisted block drivers */
>>  static int use_bdrv_whitelist;
>>  
>> @@ -660,6 +663,9 @@ void bdrv_close_all(void)
>>  void bdrv_delete(BlockDriverState *bs)
>>  {
>>      assert(!bs->peer);
>> +    if (bs == bs_snapshots) {
>> +        bs_snapshots = NULL;
>> +    }
>
> This should probably be in bdrv_close() instead. A BlockDriverState can
> be closed, but not deleted yet; it can't handle snapshots in this state,
> though.

Right.  I was thinking about the dangling pointer only.

My patch works as advertized: it fixes the crash.  But zapping
bs_snapshots in bdrv_close() is even better.  I'll respin.


>>      /* remove from list, if necessary */
>>      if (bs->device_name[0] != '\0') {
>> @@ -1772,6 +1778,25 @@ int bdrv_can_snapshot(BlockDriverState *bs)
>>      return 1;
>>  }
>>  
>> +BlockDriverState *bdrv_snapshots(void)
>> +{
>> +    BlockDriverState *bs;
>> +
>> +    if (bs_snapshots)
>> +        return bs_snapshots;
>
> I know that this function is just moved with no changes, but while we're
> at it and you need to respin anyway, can we add braces here?
>
>> +
>> +    bs = NULL;
>> +    while ((bs = bdrv_next(bs))) {
>> +        if (bdrv_can_snapshot(bs)) {
>> +            goto ok;
>> +        }
>> +    }
>> +    return NULL;
>> + ok:
>> +    bs_snapshots = bs;
>> +    return bs;
>> +}
>
> And instead of a goto we could do the right thing directly in the if block.

Separate patch.  I hate it when people squash code motion and change
together.

Patch

diff --git a/block.c b/block.c
index 5e0ffa0..34055e0 100644
--- a/block.c
+++ b/block.c
@@ -63,6 +63,9 @@  static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
 static QLIST_HEAD(, BlockDriver) bdrv_drivers =
     QLIST_HEAD_INITIALIZER(bdrv_drivers);
 
+/* The device to use for VM snapshots */
+static BlockDriverState *bs_snapshots;
+
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
@@ -660,6 +663,9 @@  void bdrv_close_all(void)
 void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->peer);
+    if (bs == bs_snapshots) {
+        bs_snapshots = NULL;
+    }
 
     /* remove from list, if necessary */
     if (bs->device_name[0] != '\0') {
@@ -1772,6 +1778,25 @@  int bdrv_can_snapshot(BlockDriverState *bs)
     return 1;
 }
 
+BlockDriverState *bdrv_snapshots(void)
+{
+    BlockDriverState *bs;
+
+    if (bs_snapshots)
+        return bs_snapshots;
+
+    bs = NULL;
+    while ((bs = bdrv_next(bs))) {
+        if (bdrv_can_snapshot(bs)) {
+            goto ok;
+        }
+    }
+    return NULL;
+ ok:
+    bs_snapshots = bs;
+    return bs;
+}
+
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info)
 {
diff --git a/block.h b/block.h
index 88ac06e..012c2a1 100644
--- a/block.h
+++ b/block.h
@@ -193,6 +193,7 @@  const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
 int bdrv_can_snapshot(BlockDriverState *bs);
+BlockDriverState *bdrv_snapshots(void);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
 int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/savevm.c b/savevm.c
index 20354a8..f1f450e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -83,9 +83,6 @@ 
 #include "qemu_socket.h"
 #include "qemu-queue.h"
 
-/* point to the block driver where the snapshots are managed */
-static BlockDriverState *bs_snapshots;
-
 #define SELF_ANNOUNCE_ROUNDS 5
 
 #ifndef ETH_P_RARP
@@ -1575,26 +1572,6 @@  out:
     return ret;
 }
 
-static BlockDriverState *get_bs_snapshots(void)
-{
-    BlockDriverState *bs;
-
-    if (bs_snapshots)
-        return bs_snapshots;
-    /* FIXME what if bs_snapshots gets hot-unplugged? */
-
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs)) {
-            goto ok;
-        }
-    }
-    return NULL;
- ok:
-    bs_snapshots = bs;
-    return bs;
-}
-
 static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                               const char *name)
 {
@@ -1674,7 +1651,7 @@  void do_savevm(Monitor *mon, const QDict *qdict)
         }
     }
 
-    bs = get_bs_snapshots();
+    bs = bdrv_snapshots();
     if (!bs) {
         monitor_printf(mon, "No block device can accept snapshots\n");
         return;
@@ -1769,7 +1746,7 @@  int load_vmstate(const char *name)
         }
     }
 
-    bs = get_bs_snapshots();
+    bs = bdrv_snapshots();
     if (!bs) {
         error_report("No block device supports snapshots");
         return -EINVAL;
@@ -1833,7 +1810,7 @@  void do_delvm(Monitor *mon, const QDict *qdict)
     int ret;
     const char *name = qdict_get_str(qdict, "name");
 
-    bs = get_bs_snapshots();
+    bs = bdrv_snapshots();
     if (!bs) {
         monitor_printf(mon, "No block device supports snapshots\n");
         return;
@@ -1863,7 +1840,7 @@  void do_info_snapshots(Monitor *mon)
     int nb_sns, i;
     char buf[256];
 
-    bs = get_bs_snapshots();
+    bs = bdrv_snapshots();
     if (!bs) {
         monitor_printf(mon, "No available block device supports snapshots\n");
         return;