Re: [PATCH for-4.2?] block/qcow2-bitmap: fix crash bug in qcow2_co_remove_persistent_dirty_bitmap

[John Snow]

On 12/5/19 3:09 PM, Eric Blake wrote:
> On 12/5/19 1:30 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Here is double bug:
>>
>> First, return error but not set errp. This may lead to:
>> qmp block-dirty-bitmap-remove may report success when actually failed
>>
>> block-dirty-bitmap-remove used in a transaction will crash, as
>> qmp_transaction will think that it returned success and will cal
> 
> call
> 
>> block_dirty_bitmap_remove_commit which will crash, as state->bitmap is
>> NULL
>>
>> Second (like in anecdote), this case is not an error at all. As it is
>> documented in the comment above bdrv_co_remove_persistent_dirty_bitmap
>> definition, absence of bitmap is not an error, and similar case handled
>> at start of qcow2_co_remove_persistent_dirty_bitmap, it returns 0 when
>> there is no bitmaps at all..
> 
> double .
> 
>>
>> But when there are some bitmaps, but not the requested one, it return
>> error with errp unset.
>>
>> Fix that.
>>
>> Fixes: b56a1e31759b750
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>
>> Hi all!
>>
>> Ohm, suddenly we faced this bug. It's a regression in 4.2. I'm very
>> sorry for introducing it, and it sad that it's found so late..
>>
>> Personally, I think that this one worth rc5, as it makes new bitmap
>> interfaces unusable. But the decision is yours.
>>
>> Last minute edit: hmm, actually, transaction action introduced in
>> 4.2, so crash is not a regression, only broken block-dirty-bitmap-remove
>> command is a regression... Maybe it's OK for stable.
> 
> Libvirt REALLY wants to use transaction bitmap management (and require
> qemu 4.2) for its incremental backup patches that Peter is almost ready
> to merge in.  I'm trying to ascertain:
> 
> When exactly can this bug hit?  Can you give a sequence of QMP commands
> that will trigger it for easy reproduction?  Are there workarounds (such
> as checking that a bitmap exists prior to attempting to remove it)?  If
> it does NOT get fixed in rc5, how will libvirt be able to probe whether
> the fix is in place?
> 

It looks like:

- You need to have at least one bitmap
- You need to use transactional remove
- you need to misspell the bitmap name
- The remove will fail (return -EINVAL) but doesn't set errp
- The caller chokes on incomplete information, state->bitmap is NULL

>>
>>   block/qcow2-bitmap.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 8abaf632fc..c6c8ebbe89 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1469,8 +1469,10 @@ int coroutine_fn
>> qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>       Qcow2BitmapList *bm_list;
>>         if (s->nb_bitmaps == 0) {
>> -        /* Absence of the bitmap is not an error: see explanation above
>> -         * bdrv_remove_persistent_dirty_bitmap() definition. */
>> +        /*
>> +         * Absence of the bitmap is not an error: see explanation above
>> +         * bdrv_co_remove_persistent_dirty_bitmap() definition.
>> +         */
>>           return 0;
>>       }
>>   @@ -1485,7 +1487,8 @@ int coroutine_fn
>> qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>         bm = find_bitmap_by_name(bm_list, name);
>>       if (bm == NULL) {
>> -        ret = -EINVAL;
>> +        /* Absence of the bitmap is not an error, see above. */
>> +        ret = 0;
>>           goto out;
>>       }
>>  
>