Re: [Qemu-devel] [PATCH v6 08/20] block: vhdx - log parsing, replay, and flush support

[Jeff Cody <jcody@redhat.com>]

On Tue, Oct 01, 2013 at 01:31:36PM +0200, Stefan Hajnoczi wrote:
> On Wed, Sep 25, 2013 at 05:02:53PM -0400, Jeff Cody wrote:
> > +static int vhdx_log_read_desc(BlockDriverState *bs, BDRVVHDXState *s,
> > +                              VHDXLogEntries *log, VHDXLogDescEntries **buffer)
> > +{
> > +    int ret = 0;
> > +    uint32_t desc_sectors;
> > +    uint32_t sectors_read;
> > +    VHDXLogEntryHeader hdr;
> > +    VHDXLogDescEntries *desc_entries = NULL;
> > +    int i;
> > +
> > +    assert(*buffer == NULL);
> > +
> > +    ret = vhdx_log_peek_hdr(bs, log, &hdr);
> > +    if (ret < 0) {
> > +        goto exit;
> > +    }
> > +    vhdx_log_entry_hdr_le_import(&hdr);
> > +    if (vhdx_log_hdr_is_valid(log, &hdr, s) == false) {
> > +        ret = -EINVAL;
> > +        goto exit;
> > +    }
> > +
> > +    desc_sectors = vhdx_compute_desc_sectors(hdr.descriptor_count);
> 
> I don't see input validation for hdr.descriptor_count.  It not exceed
> log length.

Good catch - I'll validate it on open.

> 
> > +static int vhdx_log_flush_desc(BlockDriverState *bs, VHDXLogDescriptor *desc,
> > +                               VHDXLogDataSector *data)
> > +{
> > +    int ret = 0;
> > +    uint64_t seq, file_offset;
> > +    uint32_t offset = 0;
> > +    void *buffer = NULL;
> > +    uint64_t count = 1;
> > +    int i;
> > +
> > +    buffer = qemu_blockalign(bs, VHDX_LOG_SECTOR_SIZE);
> > +
> > +    if (!memcmp(&desc->signature, "desc", 4)) {
> > +        /* data sector */
> > +        if (data == NULL) {
> > +            ret = -EFAULT;
> > +            goto exit;
> > +        }
> > +
> > +        /* The sequence number of the data sector must match that
> > +         * in the descriptor */
> > +        seq = data->sequence_high;
> > +        seq <<= 32;
> > +        seq |= data->sequence_low & 0xffffffff;
> > +
> > +        if (seq != desc->sequence_number) {
> > +            ret = -EINVAL;
> > +            goto exit;
> > +        }
> > +
> > +        /* Each data sector is in total 4096 bytes, however the first
> > +         * 8 bytes, and last 4 bytes, are located in the descriptor */
> > +        memcpy(buffer, &desc->leading_bytes, 8);
> > +        offset += 8;
> > +
> > +        memcpy(buffer+offset, data->data, 4084);
> > +        offset += 4084;
> > +
> > +        memcpy(buffer+offset, &desc->trailing_bytes, 4);
> > +
> > +    } else if (!memcmp(&desc->signature, "zero", 4)) {
> > +        /* write 'count' sectors of sector */
> > +        memset(buffer, 0, VHDX_LOG_SECTOR_SIZE);
> > +        count = desc->zero_length / VHDX_LOG_SECTOR_SIZE;
> 
> Zero descriptors also have a sequence number that should be checked.
> 

At this point, they already are - vhdx_log_desc_is_valid() has
validated the descriptor sequence numbers by now.  This check is
making sure the sequence number in the actual data sector is a match
(there is no zero sector by definition).  

vhdx_log_desc_is_valid() is called from within vhdx_log_read_desc(),
and will return -EINVAL if the sequence number is not a match.


> > +/* Parse the replay log.  Per the VHDX spec, if the log is present
> > + * it must be replayed prior to opening the file, even read-only.
> > + *
> > + * If read-only, we must replay the log in RAM (or refuse to open
> > + * a dirty VHDX file read-only */
> 
> read-only) <-- missing parenthesis
> 
> > @@ -794,10 +753,12 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
> >      uint32_t i;
> >      uint64_t signature;
> >      uint32_t data_blocks_cnt, bitmap_blocks_cnt;
> > +    bool log_flushed = false;
> >  
> >  
> >      s->bat = NULL;
> >      s->first_visible_write = true;
> > +    s->log.write = s->log.read = 0;
> 
> This is not really necessary since bs->opaque is zeroed before block.c
> calls vhdx_open().
>

OK

> >  
> >      qemu_co_mutex_init(&s->lock);
> >  
> > @@ -821,20 +782,33 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
> >          goto fail;
> >      }
> >  
> > -    ret = vhdx_parse_log(bs, s);
> > +    ret = vhdx_open_region_tables(bs, s);
> >      if (ret) {
> >          goto fail;
> >      }
> >  
> > -    ret = vhdx_open_region_tables(bs, s);
> > +    ret = vhdx_parse_metadata(bs, s);
> >      if (ret) {
> >          goto fail;
> >      }
> >  
> > -    ret = vhdx_parse_metadata(bs, s);
> > +    ret = vhdx_parse_log(bs, s, &log_flushed);
> >      if (ret) {
> >          goto fail;
> >      }
> 
> Why replay the log *after* reading logged metadata?  We could read stale
> or corrupted values.
> 
> I guess there is a dependency here - the log replay code needs some
> field that vhdx_open_region_tables() or vhdx_parse_metadata() load?

The main reason I did it this way was so that memory region overlap
could be detected, prior to writing anything to the file.  If the log
overlaps with any region, we can error out without modifying the image
file.

Outside of the header section, there is no direct metadata or region
table dependency for the log, except from needing to parse those to
determine any potential overlaps with the log.

I originally had the log flushed first - but moved it down after
adding the region table detection.