From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33360) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eMibq-0003eH-WD for qemu-devel@nongnu.org; Wed, 06 Dec 2017 17:56:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eMibp-0004b5-U3 for qemu-devel@nongnu.org; Wed, 06 Dec 2017 17:56:11 -0500 References: <20171130164750.47320-1-vsementsov@virtuozzo.com> <20171130164750.47320-2-vsementsov@virtuozzo.com> From: John Snow Message-ID: <2291b175-1d72-1c85-9767-bf8dbaeedef5@redhat.com> Date: Wed, 6 Dec 2017 17:56:00 -0500 MIME-Version: 1.0 In-Reply-To: <20171130164750.47320-2-vsementsov@virtuozzo.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] qcow2: add overlap check for bitmap directory List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org, qemu-block@nongnu.org Cc: kwolf@redhat.com, mreitz@redhat.com On 11/30/2017 11:47 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > 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) >