| Message ID | 20171130164750.47320-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2: add overlap check for bitmap directory | expand |
[adding Dan in cc] On 11/30/2017 10:47 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.h | 7 +++++-- > block/qcow2-refcount.c | 12 ++++++++++++ > block/qcow2.c | 6 ++++++ > 3 files changed, 23 insertions(+), 2 deletions(-) > Does the LUKS encryption header need similar overlap protection checks? > diff --git a/block/qcow2.h b/block/qcow2.h > index 6f0ff15dd0..8f226a3609 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > +++ b/block/qcow2-refcount.c > @@ -2585,6 +2585,18 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset, > } > } > > + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && > + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) > + { > + /* update_ext_header_and_dir_in_place firstly drop autoclear flag, > + * so it will not fail */ Wording is awkward, how about: The autoclear flag was previously dropped by update_ext_header_and_dir_in_place, so this will not fail but I'm not sure if that is the intended meaning. > + if (overlaps_with(s->bitmap_directory_offset, > + s->bitmap_directory_size)) > + { > + return QCOW2_OL_BITMAP_DIRECTORY; > + } > + } > + > return 0; > } Do we need to add/update any iotests to cover this?
On 11/30/2017 11:47 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.h | 7 +++++-- > block/qcow2-refcount.c | 12 ++++++++++++ > block/qcow2.c | 6 ++++++ > 3 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index 6f0ff15dd0..8f226a3609 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" > @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap { > QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5, > QCOW2_OL_INACTIVE_L1_BITNR = 6, > QCOW2_OL_INACTIVE_L2_BITNR = 7, > + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8, > > - QCOW2_OL_MAX_BITNR = 8, > + QCOW2_OL_MAX_BITNR = 9, > > QCOW2_OL_NONE = 0, > QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR), > @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap { > /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv > * reads. */ > QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR), > + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR), > } 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-refcount.c b/block/qcow2-refcount.c > index 3de1ab51ba..a7a2703f26 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -2585,6 +2585,18 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset, > } > } > > + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && > + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) > + { > + /* update_ext_header_and_dir_in_place firstly drop autoclear flag, > + * so it will not fail */ > + if (overlaps_with(s->bitmap_directory_offset, > + s->bitmap_directory_size)) > + { > + return QCOW2_OL_BITMAP_DIRECTORY; > + } > + } > + Isn't the purpose of this function to test if a given offset conflicts with known regions of the file? I don't see you actually utilize the 'offset' parameter here, but maybe I don't understand what you're trying to accomplish. > return 0; > } > > diff --git a/block/qcow2.c b/block/qcow2.c > index 1914a940e5..8278c0e124 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -655,6 +655,11 @@ static QemuOptsList qcow2_runtime_opts = { > .help = "Check for unintended writes into an inactive L2 table", > }, > { > + .name = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY, > + .type = QEMU_OPT_BOOL, > + .help = "Check for unintended writes into the bitmap directory", > + }, > + { > .name = QCOW2_OPT_CACHE_SIZE, > .type = QEMU_OPT_SIZE, > .help = "Maximum combined metadata (L2 tables and refcount blocks) " > @@ -690,6 +695,7 @@ static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = { > [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE, > [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1, > [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2, > + [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY, > }; > > static void cache_clean_timer_cb(void *opaque) >
On 12/07/2017 04:43 AM, Vladimir Sementsov-Ogievskiy wrote: > 07.12.2017 01:56, John Snow wrote: >> >> On 11/30/2017 11:47 AM, Vladimir Sementsov-Ogievskiy wrote: >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block/qcow2.h | 7 +++++-- >>> block/qcow2-refcount.c | 12 ++++++++++++ >>> block/qcow2.c | 6 ++++++ >>> 3 files changed, 23 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/qcow2.h b/block/qcow2.h >>> index 6f0ff15dd0..8f226a3609 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" >>> @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap { >>> QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5, >>> QCOW2_OL_INACTIVE_L1_BITNR = 6, >>> QCOW2_OL_INACTIVE_L2_BITNR = 7, >>> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8, >>> - QCOW2_OL_MAX_BITNR = 8, >>> + QCOW2_OL_MAX_BITNR = 9, >>> QCOW2_OL_NONE = 0, >>> QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR), >>> @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap { >>> /* NOTE: Checking overlaps with inactive L2 tables will result >>> in bdrv >>> * reads. */ >>> QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR), >>> + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR), >>> } 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-refcount.c b/block/qcow2-refcount.c >>> index 3de1ab51ba..a7a2703f26 100644 >>> --- a/block/qcow2-refcount.c >>> +++ b/block/qcow2-refcount.c >>> @@ -2585,6 +2585,18 @@ int >>> qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t >>> offset, >>> } >>> } >>> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && >>> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) >>> + { >>> + /* update_ext_header_and_dir_in_place firstly drop autoclear >>> flag, >>> + * so it will not fail */ >>> + if (overlaps_with(s->bitmap_directory_offset, >>> + s->bitmap_directory_size)) >>> + { >>> + return QCOW2_OL_BITMAP_DIRECTORY; >>> + } >>> + } >>> + >> Isn't the purpose of this function to test if a given offset conflicts >> with known regions of the file? I don't see you actually utilize the >> 'offset' parameter here, but maybe I don't understand what you're trying >> to accomplish. > > #define overlaps_with(ofs, sz) \ > ranges_overlap(offset, size, ofs, sz) > > I've just copied one of similar blocks in qcow2_check_metadata_overlap() > Sigh, I didn't realize that was a macro. I don't really like lowercase macros, but you didn't add it. Reviewed-by: John Snow <jsnow@redhat.com> Carry on...
On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.h | 7 +++++-- > block/qcow2-refcount.c | 12 ++++++++++++ > block/qcow2.c | 6 ++++++ > 3 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index 6f0ff15dd0..8f226a3609 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" > @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap { > QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5, > QCOW2_OL_INACTIVE_L1_BITNR = 6, > QCOW2_OL_INACTIVE_L2_BITNR = 7, > + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8, > > - QCOW2_OL_MAX_BITNR = 8, > + QCOW2_OL_MAX_BITNR = 9, > > QCOW2_OL_NONE = 0, > QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR), > @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap { > /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv > * reads. */ > QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR), > + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR), > } 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-refcount.c b/block/qcow2-refcount.c > index 3de1ab51ba..a7a2703f26 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -2585,6 +2585,18 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset, > } > } > > + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && > + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) > + { > + /* update_ext_header_and_dir_in_place firstly drop autoclear flag, > + * so it will not fail */ That's really not an argument. bitmap_list_store() has to pass QCOW2_OL_BITMAP_DIRECTORY to @ign anyway. (Because there is no reason not to.) Max > + if (overlaps_with(s->bitmap_directory_offset, > + s->bitmap_directory_size)) > + { > + return QCOW2_OL_BITMAP_DIRECTORY; > + } > + } > + > return 0; > } > > diff --git a/block/qcow2.c b/block/qcow2.c > index 1914a940e5..8278c0e124 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -655,6 +655,11 @@ static QemuOptsList qcow2_runtime_opts = { > .help = "Check for unintended writes into an inactive L2 table", > }, > { > + .name = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY, > + .type = QEMU_OPT_BOOL, > + .help = "Check for unintended writes into the bitmap directory", > + }, > + { > .name = QCOW2_OPT_CACHE_SIZE, > .type = QEMU_OPT_SIZE, > .help = "Maximum combined metadata (L2 tables and refcount blocks) " > @@ -690,6 +695,7 @@ static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = { > [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE, > [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1, > [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2, > + [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY, > }; > > static void cache_clean_timer_cb(void *opaque) >
29.01.2018 18:34, Max Reitz wrote: > On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote: >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2.h | 7 +++++-- >> block/qcow2-refcount.c | 12 ++++++++++++ >> block/qcow2.c | 6 ++++++ >> 3 files changed, 23 insertions(+), 2 deletions(-) >> >> diff --git a/block/qcow2.h b/block/qcow2.h >> index 6f0ff15dd0..8f226a3609 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" >> @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap { >> QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5, >> QCOW2_OL_INACTIVE_L1_BITNR = 6, >> QCOW2_OL_INACTIVE_L2_BITNR = 7, >> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8, >> >> - QCOW2_OL_MAX_BITNR = 8, >> + QCOW2_OL_MAX_BITNR = 9, >> >> QCOW2_OL_NONE = 0, >> QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR), >> @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap { >> /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv >> * reads. */ >> QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR), >> + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR), >> } 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-refcount.c b/block/qcow2-refcount.c >> index 3de1ab51ba..a7a2703f26 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -2585,6 +2585,18 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset, >> } >> } >> >> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && >> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) >> + { >> + /* update_ext_header_and_dir_in_place firstly drop autoclear flag, >> + * so it will not fail */ > That's really not an argument. bitmap_list_store() has to pass > QCOW2_OL_BITMAP_DIRECTORY to @ign anyway. (Because there is no reason > not to.) in_place is a reason. When we store directory in_place, it definitely overlaps with current directory. But this is done with cleared autoclear flag (to make it safe), so we will skip this check and will not fail. > > Max > >> + if (overlaps_with(s->bitmap_directory_offset, >> + s->bitmap_directory_size)) >> + { >> + return QCOW2_OL_BITMAP_DIRECTORY; >> + } >> + } >> + >> return 0; >> } >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 1914a940e5..8278c0e124 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -655,6 +655,11 @@ static QemuOptsList qcow2_runtime_opts = { >> .help = "Check for unintended writes into an inactive L2 table", >> }, >> { >> + .name = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY, >> + .type = QEMU_OPT_BOOL, >> + .help = "Check for unintended writes into the bitmap directory", >> + }, >> + { >> .name = QCOW2_OPT_CACHE_SIZE, >> .type = QEMU_OPT_SIZE, >> .help = "Maximum combined metadata (L2 tables and refcount blocks) " >> @@ -690,6 +695,7 @@ static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = { >> [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE, >> [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1, >> [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2, >> + [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY, >> }; >> >> static void cache_clean_timer_cb(void *opaque) >> >
On 2018-02-02 13:07, Vladimir Sementsov-Ogievskiy wrote: > 29.01.2018 18:34, Max Reitz wrote: >> On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote: >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block/qcow2.h | 7 +++++-- >>> block/qcow2-refcount.c | 12 ++++++++++++ >>> block/qcow2.c | 6 ++++++ >>> 3 files changed, 23 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/qcow2.h b/block/qcow2.h >>> index 6f0ff15dd0..8f226a3609 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" >>> @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap { >>> QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5, >>> QCOW2_OL_INACTIVE_L1_BITNR = 6, >>> QCOW2_OL_INACTIVE_L2_BITNR = 7, >>> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8, >>> - QCOW2_OL_MAX_BITNR = 8, >>> + QCOW2_OL_MAX_BITNR = 9, >>> QCOW2_OL_NONE = 0, >>> QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR), >>> @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap { >>> /* NOTE: Checking overlaps with inactive L2 tables will result >>> in bdrv >>> * reads. */ >>> QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR), >>> + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR), >>> } 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-refcount.c b/block/qcow2-refcount.c >>> index 3de1ab51ba..a7a2703f26 100644 >>> --- a/block/qcow2-refcount.c >>> +++ b/block/qcow2-refcount.c >>> @@ -2585,6 +2585,18 @@ int >>> qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t >>> offset, >>> } >>> } >>> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && >>> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) >>> + { >>> + /* update_ext_header_and_dir_in_place firstly drop autoclear >>> flag, >>> + * so it will not fail */ >> That's really not an argument. bitmap_list_store() has to pass >> QCOW2_OL_BITMAP_DIRECTORY to @ign anyway. (Because there is no reason >> not to.) > > in_place is a reason. When we store directory in_place, it definitely > overlaps with current directory. Well, then you just pass QCOW2_OL_BITMAP_DIRECTORY to @ign, which is what that argument is for? :-) Max > But this is done with cleared autoclear flag (to make it safe), so we > will skip this check and will not > fail.
02.02.2018 16:00, Max Reitz wrote: > On 2018-02-02 13:07, Vladimir Sementsov-Ogievskiy wrote: >> 29.01.2018 18:34, Max Reitz wrote: >>> On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote: >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> block/qcow2.h | 7 +++++-- >>>> block/qcow2-refcount.c | 12 ++++++++++++ >>>> block/qcow2.c | 6 ++++++ >>>> 3 files changed, 23 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/block/qcow2.h b/block/qcow2.h >>>> index 6f0ff15dd0..8f226a3609 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" >>>> @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap { >>>> QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5, >>>> QCOW2_OL_INACTIVE_L1_BITNR = 6, >>>> QCOW2_OL_INACTIVE_L2_BITNR = 7, >>>> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8, >>>> - QCOW2_OL_MAX_BITNR = 8, >>>> + QCOW2_OL_MAX_BITNR = 9, >>>> QCOW2_OL_NONE = 0, >>>> QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR), >>>> @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap { >>>> /* NOTE: Checking overlaps with inactive L2 tables will result >>>> in bdrv >>>> * reads. */ >>>> QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR), >>>> + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR), >>>> } 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-refcount.c b/block/qcow2-refcount.c >>>> index 3de1ab51ba..a7a2703f26 100644 >>>> --- a/block/qcow2-refcount.c >>>> +++ b/block/qcow2-refcount.c >>>> @@ -2585,6 +2585,18 @@ int >>>> qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t >>>> offset, >>>> } >>>> } >>>> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && >>>> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) >>>> + { >>>> + /* update_ext_header_and_dir_in_place firstly drop autoclear >>>> flag, >>>> + * so it will not fail */ >>> That's really not an argument. bitmap_list_store() has to pass >>> QCOW2_OL_BITMAP_DIRECTORY to @ign anyway. (Because there is no reason >>> not to.) >> in_place is a reason. When we store directory in_place, it definitely >> overlaps with current directory. > Well, then you just pass QCOW2_OL_BITMAP_DIRECTORY to @ign, which is > what that argument is for? :-) hmm. but actually, I should not, because of zeroed autoclear flag. So, do you think, it is better to pass it, anyway? > > Max > >> But this is done with cleared autoclear flag (to make it safe), so we >> will skip this check and will not >> fail.
On 2018-02-02 14:48, Vladimir Sementsov-Ogievskiy wrote: > 02.02.2018 16:00, Max Reitz wrote: >> On 2018-02-02 13:07, Vladimir Sementsov-Ogievskiy wrote: >>> 29.01.2018 18:34, Max Reitz wrote: >>>> On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote: >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>> --- >>>>> block/qcow2.h | 7 +++++-- >>>>> block/qcow2-refcount.c | 12 ++++++++++++ >>>>> block/qcow2.c | 6 ++++++ >>>>> 3 files changed, 23 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/block/qcow2.h b/block/qcow2.h >>>>> index 6f0ff15dd0..8f226a3609 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" >>>>> @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap { >>>>> QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5, >>>>> QCOW2_OL_INACTIVE_L1_BITNR = 6, >>>>> QCOW2_OL_INACTIVE_L2_BITNR = 7, >>>>> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8, >>>>> - QCOW2_OL_MAX_BITNR = 8, >>>>> + QCOW2_OL_MAX_BITNR = 9, >>>>> QCOW2_OL_NONE = 0, >>>>> QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR), >>>>> @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap { >>>>> /* NOTE: Checking overlaps with inactive L2 tables will result >>>>> in bdrv >>>>> * reads. */ >>>>> QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR), >>>>> + QCOW2_OL_BITMAP_DIRECTORY = (1 << >>>>> QCOW2_OL_BITMAP_DIRECTORY_BITNR), >>>>> } 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-refcount.c b/block/qcow2-refcount.c >>>>> index 3de1ab51ba..a7a2703f26 100644 >>>>> --- a/block/qcow2-refcount.c >>>>> +++ b/block/qcow2-refcount.c >>>>> @@ -2585,6 +2585,18 @@ int >>>>> qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t >>>>> offset, >>>>> } >>>>> } >>>>> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && >>>>> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) >>>>> + { >>>>> + /* update_ext_header_and_dir_in_place firstly drop autoclear >>>>> flag, >>>>> + * so it will not fail */ >>>> That's really not an argument. bitmap_list_store() has to pass >>>> QCOW2_OL_BITMAP_DIRECTORY to @ign anyway. (Because there is no reason >>>> not to.) >>> in_place is a reason. When we store directory in_place, it definitely >>> overlaps with current directory. >> Well, then you just pass QCOW2_OL_BITMAP_DIRECTORY to @ign, which is >> what that argument is for? :-) > > hmm. but actually, I should not, because of zeroed autoclear flag. So, > do you think, it is better to pass it, anyway? Yes. That flag describes what kind of metadata structures you are planning to overwrite, and you *are* planning to overwrite the bitmap directory, so you should set it. Max
02.02.2018 16:53, Max Reitz wrote: > On 2018-02-02 14:48, Vladimir Sementsov-Ogievskiy wrote: >> 02.02.2018 16:00, Max Reitz wrote: >>> On 2018-02-02 13:07, Vladimir Sementsov-Ogievskiy wrote: >>>> 29.01.2018 18:34, Max Reitz wrote: >>>>> On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote: >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>>> --- >>>>>> block/qcow2.h | 7 +++++-- >>>>>> block/qcow2-refcount.c | 12 ++++++++++++ >>>>>> block/qcow2.c | 6 ++++++ >>>>>> 3 files changed, 23 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/block/qcow2.h b/block/qcow2.h >>>>>> index 6f0ff15dd0..8f226a3609 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" >>>>>> @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap { >>>>>> QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5, >>>>>> QCOW2_OL_INACTIVE_L1_BITNR = 6, >>>>>> QCOW2_OL_INACTIVE_L2_BITNR = 7, >>>>>> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8, >>>>>> - QCOW2_OL_MAX_BITNR = 8, >>>>>> + QCOW2_OL_MAX_BITNR = 9, >>>>>> QCOW2_OL_NONE = 0, >>>>>> QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR), >>>>>> @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap { >>>>>> /* NOTE: Checking overlaps with inactive L2 tables will result >>>>>> in bdrv >>>>>> * reads. */ >>>>>> QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR), >>>>>> + QCOW2_OL_BITMAP_DIRECTORY = (1 << >>>>>> QCOW2_OL_BITMAP_DIRECTORY_BITNR), >>>>>> } 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-refcount.c b/block/qcow2-refcount.c >>>>>> index 3de1ab51ba..a7a2703f26 100644 >>>>>> --- a/block/qcow2-refcount.c >>>>>> +++ b/block/qcow2-refcount.c >>>>>> @@ -2585,6 +2585,18 @@ int >>>>>> qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t >>>>>> offset, >>>>>> } >>>>>> } >>>>>> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && >>>>>> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) >>>>>> + { >>>>>> + /* update_ext_header_and_dir_in_place firstly drop autoclear >>>>>> flag, >>>>>> + * so it will not fail */ >>>>> That's really not an argument. bitmap_list_store() has to pass >>>>> QCOW2_OL_BITMAP_DIRECTORY to @ign anyway. (Because there is no reason >>>>> not to.) >>>> in_place is a reason. When we store directory in_place, it definitely >>>> overlaps with current directory. >>> Well, then you just pass QCOW2_OL_BITMAP_DIRECTORY to @ign, which is >>> what that argument is for? :-) >> hmm. but actually, I should not, because of zeroed autoclear flag. So, >> do you think, it is better to pass it, anyway? > Yes. That flag describes what kind of metadata structures you are > planning to overwrite, and you *are* planning to overwrite the bitmap > directory, so you should set it. > > Max > Ok, reasonable. I'll respin with that fixed.
diff --git a/block/qcow2.h b/block/qcow2.h index 6f0ff15dd0..8f226a3609 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" @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap { QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5, QCOW2_OL_INACTIVE_L1_BITNR = 6, QCOW2_OL_INACTIVE_L2_BITNR = 7, + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8, - QCOW2_OL_MAX_BITNR = 8, + QCOW2_OL_MAX_BITNR = 9, QCOW2_OL_NONE = 0, QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR), @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap { /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv * reads. */ QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR), + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR), } 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-refcount.c b/block/qcow2-refcount.c index 3de1ab51ba..a7a2703f26 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -2585,6 +2585,18 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset, } } + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) + { + /* update_ext_header_and_dir_in_place firstly drop autoclear flag, + * so it will not fail */ + if (overlaps_with(s->bitmap_directory_offset, + s->bitmap_directory_size)) + { + return QCOW2_OL_BITMAP_DIRECTORY; + } + } + return 0; } diff --git a/block/qcow2.c b/block/qcow2.c index 1914a940e5..8278c0e124 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -655,6 +655,11 @@ static QemuOptsList qcow2_runtime_opts = { .help = "Check for unintended writes into an inactive L2 table", }, { + .name = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY, + .type = QEMU_OPT_BOOL, + .help = "Check for unintended writes into the bitmap directory", + }, + { .name = QCOW2_OPT_CACHE_SIZE, .type = QEMU_OPT_SIZE, .help = "Maximum combined metadata (L2 tables and refcount blocks) " @@ -690,6 +695,7 @@ static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = { [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE, [QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1, [QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2, + [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY, }; static void cache_clean_timer_cb(void *opaque)
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2.h | 7 +++++-- block/qcow2-refcount.c | 12 ++++++++++++ block/qcow2.c | 6 ++++++ 3 files changed, 23 insertions(+), 2 deletions(-)