[Qemu-devel] [PATCH v2 0/2] block: fix libvirt snapshot with existing bitmaps

[Qemu-devel] [PATCH v2 0/2] block: fix libvirt snapshot with existing bitmaps

[Vladimir Sementsov-Ogievskiy]

v2:
 - additional patch with asserts for hbitmap. As BdrvDirtyBitmap is only
   an interlayer here, I decided to plase asserts into HBitmap public
   functions.
 - add comment
 - from Max's comments:
    - assert after if block
    - bitmap->size instead of bdrv_nb_sectors. It is not beautiful, but
      with bdrv_nb_sectors we have to handle error code here, which
      would be worse I think.

Vladimir Sementsov-Ogievskiy (2):
  hbitmap: add 'pos < size' asserts
  block: fix libvirt snapshot with existing bitmaps

 block/dirty-bitmap.c | 14 ++++++++++++++
 util/hbitmap.c       |  3 +++
 2 files changed, 17 insertions(+)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] hbitmap: add 'pos < size' asserts

[Vladimir Sementsov-Ogievskiy]

For now, fail in hbitmap_set on start + count > size will come from
hbitmap_set
  hb_count_between
    hbitmap_iter_init
      assert(pos < hb->size)

This patch adds such checks to set/get/reset functions of hbitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 util/hbitmap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/util/hbitmap.c b/util/hbitmap.c
index 7121b11..99fd2ba 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -269,6 +269,7 @@ void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t count)
     start >>= hb->granularity;
     last >>= hb->granularity;
     count = last - start + 1;
+    assert(last < hb->size);
 
     hb->count += count - hb_count_between(hb, start, last);
     hb_set_between(hb, HBITMAP_LEVELS - 1, start, last);
@@ -348,6 +349,7 @@ void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count)
 
     start >>= hb->granularity;
     last >>= hb->granularity;
+    assert(last < hb->size);
 
     hb->count -= hb_count_between(hb, start, last);
     hb_reset_between(hb, HBITMAP_LEVELS - 1, start, last);
@@ -371,6 +373,7 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item)
     /* Compute position and bit in the last layer.  */
     uint64_t pos = item >> hb->granularity;
     unsigned long bit = 1UL << (pos & (BITS_PER_LONG - 1));
+    assert(pos < hb->size);
 
     return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] & bit) != 0;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] block: fix libvirt snapshot with existing bitmaps

[Vladimir Sementsov-Ogievskiy]

Fix the following bug:

 # virsh start test
 Domain test started

 #  virsh qemu-monitor-command test \
     '{"execute":"block-dirty-bitmap-add",\
      "arguments":{"node":"drive0","name":"ab"}}'
 {"return":{},"id":"libvirt-36"}'}'

 # virsh snapshot-create test
 error: Unable to read from monitor: Connection reset by peer

Actually, assert "assert(pos < hb->size)" in hbitmap_iter_init fires,
because qcow2_save_vmstate just writes to bs (not to bs->file->bs) after
the end of the drive.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4902ca5..d28b49c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -364,6 +364,20 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
                     int nr_sectors)
 {
     BdrvDirtyBitmap *bitmap;
+    int64_t bitmap_size;
+
+    if (QLIST_EMPTY(&bs->dirty_bitmaps)) {
+        return;
+    }
+
+    bitmap_size = QLIST_FIRST(&bs->dirty_bitmaps)->size;
+
+    if (cur_sector >= bitmap_size) {
+        /* this may come from qcow2_save_vmstate */
+        return;
+    }
+    assert(cur_sector + nr_sectors <= bitmap_size);
+
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
         if (!bdrv_dirty_bitmap_enabled(bitmap)) {
             continue;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/2] block: fix libvirt snapshot with existing bitmaps

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On 14.06.2016 19:08, Vladimir Sementsov-Ogievskiy wrote:
> v2:
>  - additional patch with asserts for hbitmap. As BdrvDirtyBitmap is only
>    an interlayer here, I decided to plase asserts into HBitmap public
>    functions.
>  - add comment
>  - from Max's comments:
>     - assert after if block
>     - bitmap->size instead of bdrv_nb_sectors. It is not beautiful, but
>       with bdrv_nb_sectors we have to handle error code here, which
>       would be worse I think.
> 
> Vladimir Sementsov-Ogievskiy (2):
>   hbitmap: add 'pos < size' asserts
>   block: fix libvirt snapshot with existing bitmaps
> 
>  block/dirty-bitmap.c | 14 ++++++++++++++
>  util/hbitmap.c       |  3 +++
>  2 files changed, 17 insertions(+)

Thanks Vladimir, I've applied the series to my block branch:

https://github.com/XanClic/qemu/commits/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 2/2] block: fix libvirt snapshot with existing bitmaps

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

On 06/14/2016 11:08 AM, Vladimir Sementsov-Ogievskiy wrote:
> Fix the following bug:
> 
>  # virsh start test
>  Domain test started
> 
>  #  virsh qemu-monitor-command test \
>      '{"execute":"block-dirty-bitmap-add",\
>       "arguments":{"node":"drive0","name":"ab"}}'
>  {"return":{},"id":"libvirt-36"}'}'
> 
>  # virsh snapshot-create test
>  error: Unable to read from monitor: Connection reset by peer
> 
> Actually, assert "assert(pos < hb->size)" in hbitmap_iter_init fires,
> because qcow2_save_vmstate just writes to bs (not to bs->file->bs) after
> the end of the drive.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/dirty-bitmap.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 4902ca5..d28b49c 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -364,6 +364,20 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>                      int nr_sectors)
>  {
>      BdrvDirtyBitmap *bitmap;
> +    int64_t bitmap_size;
> +
> +    if (QLIST_EMPTY(&bs->dirty_bitmaps)) {
> +        return;
> +    }
> +
> +    bitmap_size = QLIST_FIRST(&bs->dirty_bitmaps)->size;
> +
> +    if (cur_sector >= bitmap_size) {
> +        /* this may come from qcow2_save_vmstate */
> +        return;
> +    }

Do we still need this patch after Kevin's work to fix vmstate to no
longer go through the block layer?

https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02832.html

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] block: fix libvirt snapshot with existing bitmaps

[Vladimir Sementsov-Ogievskiy]

On 15.06.2016 00:33, Eric Blake wrote:
> On 06/14/2016 11:08 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Fix the following bug:
>>
>>   # virsh start test
>>   Domain test started
>>
>>   #  virsh qemu-monitor-command test \
>>       '{"execute":"block-dirty-bitmap-add",\
>>        "arguments":{"node":"drive0","name":"ab"}}'
>>   {"return":{},"id":"libvirt-36"}'}'
>>
>>   # virsh snapshot-create test
>>   error: Unable to read from monitor: Connection reset by peer
>>
>> Actually, assert "assert(pos < hb->size)" in hbitmap_iter_init fires,
>> because qcow2_save_vmstate just writes to bs (not to bs->file->bs) after
>> the end of the drive.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/dirty-bitmap.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 4902ca5..d28b49c 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -364,6 +364,20 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>>                       int nr_sectors)
>>   {
>>       BdrvDirtyBitmap *bitmap;
>> +    int64_t bitmap_size;
>> +
>> +    if (QLIST_EMPTY(&bs->dirty_bitmaps)) {
>> +        return;
>> +    }
>> +
>> +    bitmap_size = QLIST_FIRST(&bs->dirty_bitmaps)->size;
>> +
>> +    if (cur_sector >= bitmap_size) {
>> +        /* this may come from qcow2_save_vmstate */
>> +        return;
>> +    }
> Do we still need this patch after Kevin's work to fix vmstate to no
> longer go through the block layer?

I think not. If we are not going through block layer we are not touching 
dirty bitmaps.

>
> https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02832.html
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/2] block: fix libvirt snapshot with existing bitmaps

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

On 15.06.2016 15:08, Vladimir Sementsov-Ogievskiy wrote:
> On 15.06.2016 00:33, Eric Blake wrote:
>> On 06/14/2016 11:08 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Fix the following bug:
>>>
>>>   # virsh start test
>>>   Domain test started
>>>
>>>   #  virsh qemu-monitor-command test \
>>>       '{"execute":"block-dirty-bitmap-add",\
>>>        "arguments":{"node":"drive0","name":"ab"}}'
>>>   {"return":{},"id":"libvirt-36"}'}'
>>>
>>>   # virsh snapshot-create test
>>>   error: Unable to read from monitor: Connection reset by peer
>>>
>>> Actually, assert "assert(pos < hb->size)" in hbitmap_iter_init fires,
>>> because qcow2_save_vmstate just writes to bs (not to bs->file->bs) after
>>> the end of the drive.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/dirty-bitmap.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 4902ca5..d28b49c 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -364,6 +364,20 @@ void bdrv_set_dirty(BlockDriverState *bs,
>>> int64_t cur_sector,
>>>                       int nr_sectors)
>>>   {
>>>       BdrvDirtyBitmap *bitmap;
>>> +    int64_t bitmap_size;
>>> +
>>> +    if (QLIST_EMPTY(&bs->dirty_bitmaps)) {
>>> +        return;
>>> +    }
>>> +
>>> +    bitmap_size = QLIST_FIRST(&bs->dirty_bitmaps)->size;
>>> +
>>> +    if (cur_sector >= bitmap_size) {
>>> +        /* this may come from qcow2_save_vmstate */
>>> +        return;
>>> +    }
>> Do we still need this patch after Kevin's work to fix vmstate to no
>> longer go through the block layer?
> 
> I think not. If we are not going through block layer we are not touching
> dirty bitmaps.

OK, I'll drop this patch, then. I'll keep the first patch, though,
because it seems useful anyway.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]