From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57974) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VR0KL-0003Ph-VO for qemu-devel@nongnu.org; Tue, 01 Oct 2013 09:49:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VR0KF-0000tt-Tt for qemu-devel@nongnu.org; Tue, 01 Oct 2013 09:49:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50228) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VR0KF-0000tp-LM for qemu-devel@nongnu.org; Tue, 01 Oct 2013 09:49:19 -0400 Date: Tue, 1 Oct 2013 09:49:15 -0400 From: Jeff Cody Message-ID: <20131001134915.GF10859@localhost.localdomain> References: <20131001114218.GB6035@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20131001114218.GB6035@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 09/20] block: vhdx - add region overlap detection for image files List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On Tue, Oct 01, 2013 at 01:42:18PM +0200, Stefan Hajnoczi wrote: > On Wed, Sep 25, 2013 at 05:02:54PM -0400, Jeff Cody wrote: > > +/* Check for region overlaps inside the VHDX image */ > > +static int vhdx_region_check(BDRVVHDXState *s, uint64_t start, uint64_t length) > > +{ > > + int ret = 0; > > + uint64_t end; > > + VHDXRegionEntry *r; > > + > > + end = start + length; > > + QLIST_FOREACH(r, &s->regions, entries) { > > + if ((start >= r->start && start < r->end) || > > + (end > r->start && end <= r->end)) { > > + ret = -EINVAL; > > + goto exit; > > + } > > + } > > + > > +exit: > > + return ret; > > +} > > This check does not catch a region that spans existing regions: > > |----------------| new > |-----| existing > > This will catch all cases: > > QLIST_FOREACH(r, &s->regions, entries) { > if (!((start >= r->end) || (end <= r->start))) { > return -EINVAL; > } > } > return 0; > You are right, thanks. > > @@ -451,6 +499,15 @@ static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s) > > le32_to_cpus(&rt_entry.length); > > le32_to_cpus(&rt_entry.data_bits); > > > > + /* check for region overlap between these entries, and any > > + * other memory regions in the file */ > > + ret = vhdx_region_check(s, rt_entry.file_offset, rt_entry.length); > > + if (ret < 0) { > > + goto fail; > > + } > > + > > + vhdx_region_register(s, rt_entry.file_offset, rt_entry.length); > > + > > /* see if we recognize the entry */ > > if (guid_eq(rt_entry.guid, bat_guid)) { > > /* must be unique; if we have already found it this is invalid */ > > @@ -481,6 +538,12 @@ static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s) > > goto fail; > > } > > } > > + > > + if (!bat_rt_found || !metadata_rt_found) { > > + ret = -EINVAL; > > + goto fail; > > + } > > + > > ret = 0; > > > > fail: > > Another reason to avoid opening region tables before reading the > journal: we'll add regions twice if the journal had to be flushed. Yes, good point - I'll need to think of a good solution to this, if I want to be able to check table overlaps prior to flushing the log. Alternatively, just not worry about that and go ahead and flush the log, and detect overlaps afterwards. Maybe we'd get lucky and the log flush would fix the overlaps :)