| Message ID | 4a27dc359f8211700662949bdecdd992f8918c12.1578505678.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: Misc BDRV_SECTOR_SIZE updates | expand |
On Wed, Jan 8, 2020 at 7:52 PM Alberto Garcia <berto@igalia.com> 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 <berto@igalia.com> > --- > 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
Am 08.01.2020 um 20:46 hat Nir Soffer geschrieben: > On Wed, Jan 8, 2020 at 7:52 PM Alberto Garcia <berto@igalia.com> 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. I still hope that we'll convert bs->total_sectors to something based on bytes so we can actually handle byte-granularity image sizes, but for the time being, I guess this fix makes sense (though we don't have the check in other drivers which have the same problem). > 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? A qcow2 image should be able to provide an image for any disk the user wants to expose to the guest, and 4k alignment would certainly limit that ability. I suspect that if CHS geometry matters for a guest, not having a 4k aligned size is actually quite likely. Kevin
On Wed 08 Jan 2020 08:46:11 PM CET, Nir Soffer wrote: >> 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? I wouldn't force that on the user. The only reason why I'm not allowing the non-sector-aligned case is because it's currently broken, but I wouldn't have a problem with it if it was working fine. >> +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" Ok >> +== 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. Ok, I can also replace the "qemu-io write" with a "qemu-img info" or something like that to make it a bit less confusing (i.e. the error happens already when you open the image, not when you write to it). Berto
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 +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 *** done
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. Signed-off-by: Alberto Garcia <berto@igalia.com> --- 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(-)