Re: [PATCH 1/3] qcow2: Require that the virtual size is a multiple of the sector size

[Nir Soffer]

On Wed, Jan 8, 2020 at 7:52 PM Alberto Garcia <address@hidden> wrote:
>
> The qcow2 header specifies the virtual size of the image in bytes, but
> BlockDriverState stores it as a number of 512-byte sectors.
>
> If the user tries to create an image with a size that is not a
> multiple of the sector size then this is fixed on creation by
> silently rounding the image size up (see commit c2eb918e32).
> qcow2_co_truncate() is more strict and returns an error instead.
>
> However when an image is opened the virtual size is rounded down,
> which means that trying to access the last few advertised bytes will
> result in an error. As seen above QEMU cannot create such images and
> there's no good use case that would require us to try to handle them
> so let's just treat them as unsupported.

Making error impossible is best.

Can we require multiple of 4k to avoid unaligned read/write at the end
of an image
aligned to 512 bytes on storage with 4k sector size?

>
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block/qcow2.c              | 7 +++++++
>  docs/interop/qcow2.txt     | 3 ++-
>  tests/qemu-iotests/080     | 7 +++++++
>  tests/qemu-iotests/080.out | 4 ++++
>  4 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7fbaac8457..92474849db 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1326,6 +1326,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>          goto fail;
>      }
>
> +    if (header.size % BDRV_SECTOR_SIZE) {
> +        error_setg(errp, "Virtual size is not a multiple of %u",
> +                   (unsigned) BDRV_SECTOR_SIZE);
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>      if (header.header_length > sizeof(header)) {
>          s->unknown_header_fields_size = header.header_length - 
> sizeof(header);
>          s->unknown_header_fields = g_malloc(s->unknown_header_fields_size);
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index af5711e533..891f5662d8 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -40,7 +40,8 @@ The first cluster of a qcow2 image contains the file header:
>                      with larger cluster sizes.
>
>           24 - 31:   size
> -                    Virtual disk size in bytes.
> +                    Virtual disk size in bytes. qemu can only handle
> +                    sizes that are a multiple of 512 bytes.
>
>                      Note: qemu has an implementation limit of 32 MB as
>                      the maximum L1 table size.  With a 2 MB cluster
> diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
> index 4bcb5021e8..2563b2c052 100755
> --- a/tests/qemu-iotests/080
> +++ b/tests/qemu-iotests/080
> @@ -48,6 +48,7 @@ header_size=104
>
>  offset_backing_file_offset=8
>  offset_backing_file_size=16
> +offset_virtual_size=24
>  offset_l1_size=36
>  offset_l1_table_offset=40
>  offset_refcount_table_offset=48
> @@ -197,6 +198,12 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_size" 
> "\x10\x00\x00\x00"
>  { $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir
>  _check_test_img
>
> +echo
> +echo "== Image size not a multiple of the sector size =="
> +_make_test_img 64k

Logging the change here would make the test and the output more clear:

    echo "modifying virtual size to 65535"

> +poke_file "$TEST_IMG" "$offset_virtual_size" 
> "\x00\x00\x00\x00\x00\x00\xff\xff"
> +{ $QEMU_IO -c "write 65530 1" $TEST_IMG; } 2>&1 | _filter_qemu_io | 
> _filter_testdir
> +
>  # success, all done
>  echo "*** done"
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
> index 45ab01db8e..e1c969e2ba 100644
> --- a/tests/qemu-iotests/080.out
> +++ b/tests/qemu-iotests/080.out
> @@ -104,4 +104,8 @@ Data may be corrupted, or further writes to the image may 
> corrupt it.
>
>  3 leaked clusters were found on the image.
>  This means waste of disk space, but no harm to data.
> +
> +== Image size not a multiple of the sector size ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
> +qemu-io: can't open device TEST_DIR/t.qcow2: Virtual size is not a multiple 
> of 512

The output is confusing, looks like we created image with aligned
size, and on the next
line it complains about the size.

>  *** done
> --
> 2.20.1
>
>

Nir