Re: [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory

[John Snow <jsnow@redhat.com>]

On 06/04/2018 12:11 PM, Vladimir Sementsov-Ogievskiy wrote:
> 20.04.2018 15:12, Vladimir Sementsov-Ogievskiy wrote:
>> 19.04.2018 23:57, John Snow wrote:
>>>
>>> On 03/19/2018 04:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>
>>>> If it appropriate for 2.12, let's push it. If not - then for 2.13.
>>>>
>>> I wonder if I can make the case that this should be in 2.12.1; arguably
>>> it is important to prevent corruption no matter how unlikely it is to
>>> ever happen.
>>>
>>> Moving it into stable increases the likelihood it shows up in
>>> downstreams, so maybe let's see what we can get away with.
>>>
>>>> v2: - squash 02 (indentation fix) to 01
>>>>      - drop comment from qcow2_check_metadata_overlap()
>>>>      - set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
>>>>        bitmap_list_store. I don't think non-inplace case should be
>>>> changed,
>>>>        as it don't touch active bitmap directory.
>>>>
>>>>   block/qcow2.h          | 45
>>>> ++++++++++++++++++++++++---------------------
>>>>   block/qcow2-bitmap.c   |  7 ++++++-
>>>>   block/qcow2-refcount.c | 10 ++++++++++
>>>>   block/qcow2.c          | 22 ++++++++++++++--------
>>>>   4 files changed, 54 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>> index 6f0ff15dd0..896ad08e5b 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -98,6 +98,7 @@
>>>>   #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE
>>>> "overlap-check.snapshot-table"
>>>>   #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
>>>>   #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
>>>> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY
>>>> "overlap-check.bitmap-directory"
>>>>   #define QCOW2_OPT_CACHE_SIZE "cache-size"
>>>>   #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
>>>>   #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
>>>> @@ -398,34 +399,36 @@ typedef enum QCow2ClusterType {
>>>>   } QCow2ClusterType;
>>>>     typedef enum QCow2MetadataOverlap {
>>>> -    QCOW2_OL_MAIN_HEADER_BITNR    = 0,
>>>> -    QCOW2_OL_ACTIVE_L1_BITNR      = 1,
>>>> -    QCOW2_OL_ACTIVE_L2_BITNR      = 2,
>>>> -    QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
>>>> -    QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
>>>> -    QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
>>>> -    QCOW2_OL_INACTIVE_L1_BITNR    = 6,
>>>> -    QCOW2_OL_INACTIVE_L2_BITNR    = 7,
>>>> -
>>>> -    QCOW2_OL_MAX_BITNR            = 8,
>>>> -
>>>> -    QCOW2_OL_NONE           = 0,
>>>> -    QCOW2_OL_MAIN_HEADER    = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
>>>> -    QCOW2_OL_ACTIVE_L1      = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
>>>> -    QCOW2_OL_ACTIVE_L2      = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
>>>> -    QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
>>>> -    QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
>>>> -    QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
>>>> -    QCOW2_OL_INACTIVE_L1    = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
>>>> +    QCOW2_OL_MAIN_HEADER_BITNR      = 0,
>>>> +    QCOW2_OL_ACTIVE_L1_BITNR        = 1,
>>>> +    QCOW2_OL_ACTIVE_L2_BITNR        = 2,
>>>> +    QCOW2_OL_REFCOUNT_TABLE_BITNR   = 3,
>>>> +    QCOW2_OL_REFCOUNT_BLOCK_BITNR   = 4,
>>>> +    QCOW2_OL_SNAPSHOT_TABLE_BITNR   = 5,
>>>> +    QCOW2_OL_INACTIVE_L1_BITNR      = 6,
>>>> +    QCOW2_OL_INACTIVE_L2_BITNR      = 7,
>>>> +    QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
>>>> +
>>> A bit hard to read due to the formatting, but you've added #8 here, and
>>>
>>>> +    QCOW2_OL_MAX_BITNR              = 9,
>>>> +> +    QCOW2_OL_NONE             = 0,
>>>> +    QCOW2_OL_MAIN_HEADER      = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
>>>> +    QCOW2_OL_ACTIVE_L1        = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
>>>> +    QCOW2_OL_ACTIVE_L2        = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
>>>> +    QCOW2_OL_REFCOUNT_TABLE   = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
>>>> +    QCOW2_OL_REFCOUNT_BLOCK   = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
>>>> +    QCOW2_OL_SNAPSHOT_TABLE   = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
>>>> +    QCOW2_OL_INACTIVE_L1      = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
>>>>       /* NOTE: Checking overlaps with inactive L2 tables will result
>>>> in bdrv
>>>>        * reads. */
>>>> -    QCOW2_OL_INACTIVE_L2    = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>>>> +    QCOW2_OL_INACTIVE_L2      = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
>>>> +    QCOW2_OL_BITMAP_DIRECTORY = (1 <<
>>>> QCOW2_OL_BITMAP_DIRECTORY_BITNR),
>>> and this one down here.
>>>
>>>>   } QCow2MetadataOverlap;
>>>>     /* Perform all overlap checks which can be done in constant time */
>>>>   #define QCOW2_OL_CONSTANT \
>>>>       (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 |
>>>> QCOW2_OL_REFCOUNT_TABLE | \
>>>> -     QCOW2_OL_SNAPSHOT_TABLE)
>>>> +     QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
>>>>     /* Perform all overlap checks which don't require disk access */
>>>>   #define QCOW2_OL_CACHED \
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> index f45e46cfbd..fb750ba8d3 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -776,7 +776,12 @@ static int bitmap_list_store(BlockDriverState
>>>> *bs, Qcow2BitmapList *bm_list,
>>>>           }
>>>>       }
>>>>   -    ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset,
>>>> dir_size);
>>>> +    /* Actually, even in in-place case ignoring
>>>> QCOW2_OL_BITMAP_DIRECTORY is not
>>>> +     * necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when
>>>> updating bitmap
>>>> +     * directory in-place (actually, turn-off the extension), which
>>>> is checked
>>>> +     * in qcow2_check_metadata_overlap() */
>>> I might need you to rephrase this for me.
>>>
>>> I guess we're ignoring QCOW2_OL_BITMAP_DIRECTORY if in_place is true,
>>> but only as an optimization. Why is it not required?
>>
>> because, actually, we don't have any dirty bitmaps extension at the
>> moment when we rewrite bitmap directory in-place (as we drop autoclear
>> bit), so nothing will be checked.
> 
> Is it clear, do you want to rephrase this somehow?
> 

Oh, sorry, I didn't wind up having a better suggestion. Thanks for the ping.

Reviewed-by: John Snow <jsnow@redhat.com>