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

[Stefan Hajnoczi <stefanha@gmail.com>]

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.

> +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.

> +/* 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().

>  
>      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?