I'm surprised it didn't come up sooner, but sometimes we have a +busy
bitmap as a source. This is dangerous from the QMP API, but if we are
the owner that marked the bitmap busy, it's safe to merge it using it as
a read only source.

It is not safe in the general case to allow users to read from in-use
bitmaps, so create an internal variant that foregoes the safety
checking.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c      | 51 +++++++++++++++++++++++++++++++++------
 include/block/block_int.h |  3 +++
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 95a9c2a5d8..b0f76826b3 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -810,11 +810,15 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
     return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
 }
 
+/**
+ * bdrv_merge_dirty_bitmap: merge src into dest.
+ * Ensures permissions on bitmaps are reasonable; use for public API.
+ *
+ * @backup: If provided, make a copy of dest here prior to merge.
+ */
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
                              HBitmap **backup, Error **errp)
 {
-    bool ret;
-
     qemu_mutex_lock(dest->mutex);
     if (src->mutex != dest->mutex) {
         qemu_mutex_lock(src->mutex);
@@ -833,6 +837,37 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
         goto out;
     }
 
+    assert(bdrv_dirty_bitmap_merge_internal(dest, src, backup, false));
+
+out:
+    qemu_mutex_unlock(dest->mutex);
+    if (src->mutex != dest->mutex) {
+        qemu_mutex_unlock(src->mutex);
+    }
+}
+
+/**
+ * bdrv_dirty_bitmap_merge_internal: merge src into dest.
+ * Does NOT check bitmap permissions; not suitable for use as public API.
+ *
+ * @backup: If provided, make a copy of dest here prior to merge.
+ * @lock: If true, lock and unlock bitmaps on the way in/out.
+ * returns true if the merge succeeded; false if unattempted.
+ */
+bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
+                                      const BdrvDirtyBitmap *src,
+                                      HBitmap **backup,
+                                      bool lock)
+{
+    bool ret;
+
+    if (lock) {
+        qemu_mutex_lock(dest->mutex);
+        if (src->mutex != dest->mutex) {
+            qemu_mutex_lock(src->mutex);
+        }
+    }
+
     if (backup) {
         *backup = dest->bitmap;
         dest->bitmap = hbitmap_alloc(dest->size, hbitmap_granularity(*backup));
@@ -840,11 +875,13 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
     } else {
         ret = hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
     }
-    assert(ret);
 
-out:
-    qemu_mutex_unlock(dest->mutex);
-    if (src->mutex != dest->mutex) {
-        qemu_mutex_unlock(src->mutex);
+    if (lock) {
+        qemu_mutex_unlock(dest->mutex);
+        if (src->mutex != dest->mutex) {
+            qemu_mutex_unlock(src->mutex);
+        }
     }
+
+    return ret;
 }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e1f2aa627e..83ffdf4950 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1238,6 +1238,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
 
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
 void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup);
+bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
+                                      const BdrvDirtyBitmap *src,
+                                      HBitmap **backup, bool lock);
 
 void bdrv_inc_in_flight(BlockDriverState *bs);
 void bdrv_dec_in_flight(BlockDriverState *bs);
-- 
2.21.0

On 03.07.19 23:55, John Snow wrote:
> I'm surprised it didn't come up sooner, but sometimes we have a +busy
> bitmap as a source. This is dangerous from the QMP API, but if we are
> the owner that marked the bitmap busy, it's safe to merge it using it as
> a read only source.
> 
> It is not safe in the general case to allow users to read from in-use
> bitmaps, so create an internal variant that foregoes the safety
> checking.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/dirty-bitmap.c      | 51 +++++++++++++++++++++++++++++++++------
>  include/block/block_int.h |  3 +++
>  2 files changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 95a9c2a5d8..b0f76826b3 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -810,11 +810,15 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>      return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
>  }
>  
> +/**
> + * bdrv_merge_dirty_bitmap: merge src into dest.
> + * Ensures permissions on bitmaps are reasonable; use for public API.
> + *
> + * @backup: If provided, make a copy of dest here prior to merge.
> + */
>  void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>                               HBitmap **backup, Error **errp)
>  {
> -    bool ret;
> -
>      qemu_mutex_lock(dest->mutex);
>      if (src->mutex != dest->mutex) {
>          qemu_mutex_lock(src->mutex);
> @@ -833,6 +837,37 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>          goto out;
>      }
>  
> +    assert(bdrv_dirty_bitmap_merge_internal(dest, src, backup, false));

Please keep the explicit @ret.  We never define NDEBUG, but doing things
with side effects inside of assert() is bad style nonetheless.

> +
> +out:
> +    qemu_mutex_unlock(dest->mutex);
> +    if (src->mutex != dest->mutex) {
> +        qemu_mutex_unlock(src->mutex);
> +    }
> +}
> +
> +/**
> + * bdrv_dirty_bitmap_merge_internal: merge src into dest.
> + * Does NOT check bitmap permissions; not suitable for use as public API.
> + *
> + * @backup: If provided, make a copy of dest here prior to merge.
> + * @lock: If true, lock and unlock bitmaps on the way in/out.
> + * returns true if the merge succeeded; false if unattempted.
> + */
> +bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
> +                                      const BdrvDirtyBitmap *src,
> +                                      HBitmap **backup,
> +                                      bool lock)
> +{
> +    bool ret;
> +
> +    if (lock) {
> +        qemu_mutex_lock(dest->mutex);
> +        if (src->mutex != dest->mutex) {
> +            qemu_mutex_lock(src->mutex);
> +        }
> +    }
> +

Why not check for INCONSISTENT and RO still?

Max

>      if (backup) {
>          *backup = dest->bitmap;
>          dest->bitmap = hbitmap_alloc(dest->size, hbitmap_granularity(*backup));

On 7/4/19 12:49 PM, Max Reitz wrote:
> On 03.07.19 23:55, John Snow wrote:
>> I'm surprised it didn't come up sooner, but sometimes we have a +busy
>> bitmap as a source. This is dangerous from the QMP API, but if we are
>> the owner that marked the bitmap busy, it's safe to merge it using it as
>> a read only source.
>>
>> It is not safe in the general case to allow users to read from in-use
>> bitmaps, so create an internal variant that foregoes the safety
>> checking.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/dirty-bitmap.c      | 51 +++++++++++++++++++++++++++++++++------
>>  include/block/block_int.h |  3 +++
>>  2 files changed, 47 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 95a9c2a5d8..b0f76826b3 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -810,11 +810,15 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>>      return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
>>  }
>>  
>> +/**
>> + * bdrv_merge_dirty_bitmap: merge src into dest.
>> + * Ensures permissions on bitmaps are reasonable; use for public API.
>> + *
>> + * @backup: If provided, make a copy of dest here prior to merge.
>> + */
>>  void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>                               HBitmap **backup, Error **errp)
>>  {
>> -    bool ret;
>> -
>>      qemu_mutex_lock(dest->mutex);
>>      if (src->mutex != dest->mutex) {
>>          qemu_mutex_lock(src->mutex);
>> @@ -833,6 +837,37 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>          goto out;
>>      }
>>  
>> +    assert(bdrv_dirty_bitmap_merge_internal(dest, src, backup, false));
> 
> Please keep the explicit @ret.  We never define NDEBUG, but doing things
> with side effects inside of assert() is bad style nonetheless.
> 

Thank you for saving me from myself. I consistently forget this :(

>> +
>> +out:
>> +    qemu_mutex_unlock(dest->mutex);
>> +    if (src->mutex != dest->mutex) {
>> +        qemu_mutex_unlock(src->mutex);
>> +    }
>> +}
>> +
>> +/**
>> + * bdrv_dirty_bitmap_merge_internal: merge src into dest.
>> + * Does NOT check bitmap permissions; not suitable for use as public API.
>> + *
>> + * @backup: If provided, make a copy of dest here prior to merge.
>> + * @lock: If true, lock and unlock bitmaps on the way in/out.
>> + * returns true if the merge succeeded; false if unattempted.
>> + */
>> +bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>> +                                      const BdrvDirtyBitmap *src,
>> +                                      HBitmap **backup,
>> +                                      bool lock)
>> +{
>> +    bool ret;
>> +
>> +    if (lock) {
>> +        qemu_mutex_lock(dest->mutex);
>> +        if (src->mutex != dest->mutex) {
>> +            qemu_mutex_lock(src->mutex);
>> +        }
>> +    }
>> +
> 
> Why not check for INCONSISTENT and RO still?
> 
> Max
> 

Well. "why", I guess. The user-facing API will always use the
non-internal version. This is the scary dangerous version that you don't
call unless you are Very Sure You Know What You're Doing.

I guess I just intended for the suitability checking to happen in
bdrv_dirty_bitmap_merge, and this is the implementation that you can
shoot yourself in the foot with if you'd like.

>>      if (backup) {
>>          *backup = dest->bitmap;
>>          dest->bitmap = hbitmap_alloc(dest->size, hbitmap_granularity(*backup));
> 

On 05.07.19 18:45, John Snow wrote:
> 
> 
> On 7/4/19 12:49 PM, Max Reitz wrote:
>> On 03.07.19 23:55, John Snow wrote:

[...]

>>> +
>>> +/**
>>> + * bdrv_dirty_bitmap_merge_internal: merge src into dest.
>>> + * Does NOT check bitmap permissions; not suitable for use as public API.
>>> + *
>>> + * @backup: If provided, make a copy of dest here prior to merge.
>>> + * @lock: If true, lock and unlock bitmaps on the way in/out.
>>> + * returns true if the merge succeeded; false if unattempted.
>>> + */
>>> +bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>>> +                                      const BdrvDirtyBitmap *src,
>>> +                                      HBitmap **backup,
>>> +                                      bool lock)
>>> +{
>>> +    bool ret;
>>> +
>>> +    if (lock) {
>>> +        qemu_mutex_lock(dest->mutex);
>>> +        if (src->mutex != dest->mutex) {
>>> +            qemu_mutex_lock(src->mutex);
>>> +        }
>>> +    }
>>> +
>>
>> Why not check for INCONSISTENT and RO still?
>>
>> Max
>>
> 
> Well. "why", I guess. The user-facing API will always use the
> non-internal version. This is the scary dangerous version that you don't
> call unless you are Very Sure You Know What You're Doing.
> 
> I guess I just intended for the suitability checking to happen in
> bdrv_dirty_bitmap_merge, and this is the implementation that you can
> shoot yourself in the foot with if you'd like.

I’m asking because the reasoning behind being allowed to call this
function is that BUSY means someone who is not the monitor has exclusive
access to the bitmap, and that someone is the caller of this function.

There is no justification for why it should be allowed to call this
function for bitmaps that are inconsistent or read-only.  If someone
needs that, they should justify it with a patch, I think.

Max

On 7/8/19 7:44 AM, Max Reitz wrote:
> On 05.07.19 18:45, John Snow wrote:
>>
>>
>> On 7/4/19 12:49 PM, Max Reitz wrote:
>>> On 03.07.19 23:55, John Snow wrote:
> 
> [...]
> 
>>>> +
>>>> +/**
>>>> + * bdrv_dirty_bitmap_merge_internal: merge src into dest.
>>>> + * Does NOT check bitmap permissions; not suitable for use as public API.
>>>> + *
>>>> + * @backup: If provided, make a copy of dest here prior to merge.
>>>> + * @lock: If true, lock and unlock bitmaps on the way in/out.
>>>> + * returns true if the merge succeeded; false if unattempted.
>>>> + */
>>>> +bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>>>> +                                      const BdrvDirtyBitmap *src,
>>>> +                                      HBitmap **backup,
>>>> +                                      bool lock)
>>>> +{
>>>> +    bool ret;
>>>> +
>>>> +    if (lock) {
>>>> +        qemu_mutex_lock(dest->mutex);
>>>> +        if (src->mutex != dest->mutex) {
>>>> +            qemu_mutex_lock(src->mutex);
>>>> +        }
>>>> +    }
>>>> +
>>>
>>> Why not check for INCONSISTENT and RO still?
>>>
>>> Max
>>>
>>
>> Well. "why", I guess. The user-facing API will always use the
>> non-internal version. This is the scary dangerous version that you don't
>> call unless you are Very Sure You Know What You're Doing.
>>
>> I guess I just intended for the suitability checking to happen in
>> bdrv_dirty_bitmap_merge, and this is the implementation that you can
>> shoot yourself in the foot with if you'd like.
> 
> I’m asking because the reasoning behind being allowed to call this
> function is that BUSY means someone who is not the monitor has exclusive
> access to the bitmap, and that someone is the caller of this function.
> 

Unfortunately, there's no way mechanically to check that this is the
exact circumstance the function is being called in. I have named it
_internal and documented the source to explain when it's safe to use.

We do not keep track of who set +BUSY and therefore who is allowed to
call functions that normally prohibit that flag from being set.

I don't think it's worth implementing that, either.

> There is no justification for why it should be allowed to call this
> function for bitmaps that are inconsistent or read-only.  If someone
> needs that, they should justify it with a patch, I think.

The only caller here has already verified that this bitmap is not
inconsistent or read-only (because the caller MADE the bitmap). Do you
feel strongly enough that the check should be duplicated for this one
particular function?

There are many other dirty bitmap functions that, because they are
called as an interior function not directly invoked by the QMP API
layer, do not do any such checking.

GIGO, surely?

--js

On 08.07.19 20:24, John Snow wrote:
> 
> 
> On 7/8/19 7:44 AM, Max Reitz wrote:
>> On 05.07.19 18:45, John Snow wrote:
>>>
>>>
>>> On 7/4/19 12:49 PM, Max Reitz wrote:
>>>> On 03.07.19 23:55, John Snow wrote:
>>
>> [...]
>>
>>>>> +
>>>>> +/**
>>>>> + * bdrv_dirty_bitmap_merge_internal: merge src into dest.
>>>>> + * Does NOT check bitmap permissions; not suitable for use as public API.
>>>>> + *
>>>>> + * @backup: If provided, make a copy of dest here prior to merge.
>>>>> + * @lock: If true, lock and unlock bitmaps on the way in/out.
>>>>> + * returns true if the merge succeeded; false if unattempted.
>>>>> + */
>>>>> +bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>>>>> +                                      const BdrvDirtyBitmap *src,
>>>>> +                                      HBitmap **backup,
>>>>> +                                      bool lock)
>>>>> +{
>>>>> +    bool ret;
>>>>> +
>>>>> +    if (lock) {
>>>>> +        qemu_mutex_lock(dest->mutex);
>>>>> +        if (src->mutex != dest->mutex) {
>>>>> +            qemu_mutex_lock(src->mutex);
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>
>>>> Why not check for INCONSISTENT and RO still?
>>>>
>>>> Max
>>>>
>>>
>>> Well. "why", I guess. The user-facing API will always use the
>>> non-internal version. This is the scary dangerous version that you don't
>>> call unless you are Very Sure You Know What You're Doing.
>>>
>>> I guess I just intended for the suitability checking to happen in
>>> bdrv_dirty_bitmap_merge, and this is the implementation that you can
>>> shoot yourself in the foot with if you'd like.
>>
>> I’m asking because the reasoning behind being allowed to call this
>> function is that BUSY means someone who is not the monitor has exclusive
>> access to the bitmap, and that someone is the caller of this function.
>>
> 
> Unfortunately, there's no way mechanically to check that this is the
> exact circumstance the function is being called in. I have named it
> _internal and documented the source to explain when it's safe to use.
> 
> We do not keep track of who set +BUSY and therefore who is allowed to
> call functions that normally prohibit that flag from being set.
> 
> I don't think it's worth implementing that, either.

Fully agreed.  I meant to say that calling this function on a busy
bitmap is completely fine, so I understand why there is no check and I
wouldn’t add one.

>> There is no justification for why it should be allowed to call this
>> function for bitmaps that are inconsistent or read-only.  If someone
>> needs that, they should justify it with a patch, I think.
> 
> The only caller here has already verified that this bitmap is not
> inconsistent or read-only (because the caller MADE the bitmap).

Which is why implementing it would be trivial, as the caller could just
pass &error_abort.

Well, or the function just asserts that !readonly && !inconsistent.
(Which would probably be better, because bdrv_dirty_bitmap_check() is
probably something better suited for external interfaces.  No need to
use it if all callers of bdrv_dirty_bitmap_merge_internal() only pass
&error_abort anyway.)

> Do you
> feel strongly enough that the check should be duplicated for this one
> particular function?

I don’t see a good reason not to.

I see a weak reason to add some form of a check (and be it just an
assertion), and that is that if someone needs to remove that check,
they’ll have to explicitly justify why that is OK.

Just like this patch justifies why it’s sometimes OK to call this
function on a busy bitmap.


So I feel weakly.

> There are many other dirty bitmap functions that, because they are
> called as an interior function not directly invoked by the QMP API
> layer, do not do any such checking.

Some still contain assertions, though.

Max

On 7/8/19 2:33 PM, Max Reitz wrote:
> On 08.07.19 20:24, John Snow wrote:
>>
>>
>> On 7/8/19 7:44 AM, Max Reitz wrote:
>>> On 05.07.19 18:45, John Snow wrote:
>>>>
>>>>
>>>> On 7/4/19 12:49 PM, Max Reitz wrote:
>>>>> On 03.07.19 23:55, John Snow wrote:
>>>
>>> [...]
>>>
>>>>>> +
>>>>>> +/**
>>>>>> + * bdrv_dirty_bitmap_merge_internal: merge src into dest.
>>>>>> + * Does NOT check bitmap permissions; not suitable for use as public API.
>>>>>> + *
>>>>>> + * @backup: If provided, make a copy of dest here prior to merge.
>>>>>> + * @lock: If true, lock and unlock bitmaps on the way in/out.
>>>>>> + * returns true if the merge succeeded; false if unattempted.
>>>>>> + */
>>>>>> +bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
>>>>>> +                                      const BdrvDirtyBitmap *src,
>>>>>> +                                      HBitmap **backup,
>>>>>> +                                      bool lock)
>>>>>> +{
>>>>>> +    bool ret;
>>>>>> +
>>>>>> +    if (lock) {
>>>>>> +        qemu_mutex_lock(dest->mutex);
>>>>>> +        if (src->mutex != dest->mutex) {
>>>>>> +            qemu_mutex_lock(src->mutex);
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>
>>>>> Why not check for INCONSISTENT and RO still?
>>>>>
>>>>> Max
>>>>>
>>>>
>>>> Well. "why", I guess. The user-facing API will always use the
>>>> non-internal version. This is the scary dangerous version that you don't
>>>> call unless you are Very Sure You Know What You're Doing.
>>>>
>>>> I guess I just intended for the suitability checking to happen in
>>>> bdrv_dirty_bitmap_merge, and this is the implementation that you can
>>>> shoot yourself in the foot with if you'd like.
>>>
>>> I’m asking because the reasoning behind being allowed to call this
>>> function is that BUSY means someone who is not the monitor has exclusive
>>> access to the bitmap, and that someone is the caller of this function.
>>>
>>
>> Unfortunately, there's no way mechanically to check that this is the
>> exact circumstance the function is being called in. I have named it
>> _internal and documented the source to explain when it's safe to use.
>>
>> We do not keep track of who set +BUSY and therefore who is allowed to
>> call functions that normally prohibit that flag from being set.
>>
>> I don't think it's worth implementing that, either.
> 
> Fully agreed.  I meant to say that calling this function on a busy
> bitmap is completely fine, so I understand why there is no check and I
> wouldn’t add one.
> 
>>> There is no justification for why it should be allowed to call this
>>> function for bitmaps that are inconsistent or read-only.  If someone
>>> needs that, they should justify it with a patch, I think.
>>
>> The only caller here has already verified that this bitmap is not
>> inconsistent or read-only (because the caller MADE the bitmap).
> 
> Which is why implementing it would be trivial, as the caller could just
> pass &error_abort.
> 
> Well, or the function just asserts that !readonly && !inconsistent.
> (Which would probably be better, because bdrv_dirty_bitmap_check() is
> probably something better suited for external interfaces.  No need to
> use it if all callers of bdrv_dirty_bitmap_merge_internal() only pass
> &error_abort anyway.)
> 
>> Do you
>> feel strongly enough that the check should be duplicated for this one
>> particular function?
> 
> I don’t see a good reason not to.
> 

"Fine," but I'll want the V3 changes eyed over first before I post a fixup.