Re: [Qemu-devel] [PATCH v6 09/20] block: vhdx - add region overlap detection for image files

[Jeff Cody <jcody@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 :)