| Message ID | 20200504155217.10325-1-berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] qcow2: Avoid integer wraparound in qcow2_co_truncate() | expand |
On 5/4/20 10:52 AM, Alberto Garcia wrote: > After commit f01643fb8b47e8a70c04bbf45e0f12a9e5bc54de when an image is > extended and BDRV_REQ_ZERO_WRITE is set then the new clusters are > zeroized. > > The code however does not detect correctly situations when the old and > the new end of the image are within the same cluster. The problem can > be reproduced with these steps: > > qemu-img create -f qcow2 backing.qcow2 1M > qemu-img create -f qcow2 -F qcow2 -b backing.qcow2 top.qcow2 > qemu-img resize --shrink top.qcow2 520k > qemu-img resize top.qcow2 567k > > In the last step offset - zero_start causes an integer wraparound. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > +++ b/block/qcow2.c > @@ -4239,15 +4239,17 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > * requires a cluster-aligned start. The end may be unaligned if it is > * at the end of the image (which it is here). > */ > - ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0); > - if (ret < 0) { > - error_setg_errno(errp, -ret, "Failed to zero out new clusters"); > - goto fail; > + if (offset > zero_start) { > + ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Failed to zero out new clusters"); > + goto fail; > + } > } > > /* Write explicit zeros for the unaligned head */ > if (zero_start > old_length) { > - uint64_t len = zero_start - old_length; > + uint64_t len = MIN(zero_start, offset) - old_length; Yes, that's better. > +++ b/tests/qemu-iotests/292 > @@ -0,0 +1,73 @@ > +_supported_fmt qcow2 > +_supported_proto file Do we have to limit it to qcow2 and file? Yes, it's testing a bugfix for qcow2, but are there other formats that it doesn't hurt to have the extra testing? Also, I don't see anything preventing this from working with non-file protocol. But whether we widen the test scope or not, we at least test that we don't regress. Reviewed-by: Eric Blake <eblake@redhat.com>
On Mon 04 May 2020 06:01:19 PM CEST, Eric Blake wrote: >> +_supported_fmt qcow2 >> +_supported_proto file > > Do we have to limit it to qcow2 and file? Yes, it's testing a bugfix > for qcow2, but are there other formats that it doesn't hurt to have > the extra testing? It doesn't work with any other format at the moment (meaning: reading the tail of the image after growing it returns the data from the backing file). Also, it seems that qemu-img's -F does not work with other formats either. > Also, I don't see anything preventing this from working with non-file > protocol. Right, that can be updated I guess (whoever commits this, feel free to do it). Berto
Patchew URL: https://patchew.org/QEMU/20200504155217.10325-1-berto@igalia.com/ Hi, This series failed build test on FreeBSD host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch if qemu-system-x86_64 --help >/dev/null 2>&1; then QEMU=qemu-system-x86_64 elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then QEMU=/usr/libexec/qemu-kvm else exit 1 fi make vm-build-freebsd J=21 QEMU=$QEMU exit 0 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200504155217.10325-1-berto@igalia.com/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Am 04.05.2020 um 19:07 hat Alberto Garcia geschrieben: > On Mon 04 May 2020 06:01:19 PM CEST, Eric Blake wrote: > >> +_supported_fmt qcow2 > >> +_supported_proto file > > > > Do we have to limit it to qcow2 and file? Yes, it's testing a bugfix > > for qcow2, but are there other formats that it doesn't hurt to have > > the extra testing? > > It doesn't work with any other format at the moment (meaning: reading > the tail of the image after growing it returns the data from the backing > file). > > Also, it seems that qemu-img's -F does not work with other formats > either. > > > Also, I don't see anything preventing this from working with non-file > > protocol. > > Right, that can be updated I guess (whoever commits this, feel free to > do it). I don't know for which protocols it works. I know that qcow2 over nbd doesn't work. But I think there is a more important problem with the test: It seems to pass even with old binaries that don't have the fix. Is this only on my system or do you get the same? Kevin
Am 05.05.2020 um 10:54 hat Kevin Wolf geschrieben: > Am 04.05.2020 um 19:07 hat Alberto Garcia geschrieben: > > On Mon 04 May 2020 06:01:19 PM CEST, Eric Blake wrote: > > >> +_supported_fmt qcow2 > > >> +_supported_proto file > > > > > > Do we have to limit it to qcow2 and file? Yes, it's testing a bugfix > > > for qcow2, but are there other formats that it doesn't hurt to have > > > the extra testing? > > > > It doesn't work with any other format at the moment (meaning: reading > > the tail of the image after growing it returns the data from the backing > > file). > > > > Also, it seems that qemu-img's -F does not work with other formats > > either. > > > > > Also, I don't see anything preventing this from working with non-file > > > protocol. > > > > Right, that can be updated I guess (whoever commits this, feel free to > > do it). > > I don't know for which protocols it works. I know that qcow2 over nbd > doesn't work. > > But I think there is a more important problem with the test: It seems to > pass even with old binaries that don't have the fix. Is this only on my > system or do you get the same? Ah, I do get the overflow in the calculation of the length for qcow2_cluster_zeroize(), but size_to_clusters() inside the function overflows back the other direction, so this ends up with nb_clusters = 0 and we don't do anything bad. We could probably trigger a bad case with data_file_raw=on, but then we don't have a backing file, so nothing sets BDRV_REQ_ZERO_WRITE. So I guess the bug isn't even really testable, but we just add the test in case something else in the same scenario breaks? Kevin
On Tue 05 May 2020 10:54:12 AM CEST, Kevin Wolf wrote: > But I think there is a more important problem with the test: It seems > to pass even with old binaries that don't have the fix. Is this only > on my system or do you get the same? With old binaries when qcow2_cluster_zeroize() is called it receives bytes = (UINT64_MAX - 9216), however that number is then used to calculate the number of affected clusters, so it's rounded up, wraps around again and back to zero. There's no visible sign of the error, it just happens to work fine. If there was a raw data file then we would try to write UINT64_MAX-9216 bytes to it, but in this case there's no backing file allowed and therefore the image is not zeroed, so qcow2_cluster_zeroize() never happens. Why the test case then? There was a mistake with my first patch and there it crashed (due to an assertion), that's why Eric thought it would be a good idea to add a test case anyway, in case we have to change that code in the future and we screw up. Berto
Am 05.05.2020 um 11:16 hat Alberto Garcia geschrieben: > On Tue 05 May 2020 10:54:12 AM CEST, Kevin Wolf wrote: > > But I think there is a more important problem with the test: It seems > > to pass even with old binaries that don't have the fix. Is this only > > on my system or do you get the same? > > With old binaries when qcow2_cluster_zeroize() is called it receives > bytes = (UINT64_MAX - 9216), however that number is then used to > calculate the number of affected clusters, so it's rounded up, wraps > around again and back to zero. There's no visible sign of the error, it > just happens to work fine. > > If there was a raw data file then we would try to write UINT64_MAX-9216 > bytes to it, but in this case there's no backing file allowed and > therefore the image is not zeroed, so qcow2_cluster_zeroize() never > happens. > > Why the test case then? There was a mistake with my first patch and > there it crashed (due to an assertion), that's why Eric thought it would > be a good idea to add a test case anyway, in case we have to change that > code in the future and we screw up. Thanks for the explanation, this makes sense. I'll apply the patch now. Kevin
diff --git a/block/qcow2.c b/block/qcow2.c index 2ba0b17c39..8c97b06783 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -4239,15 +4239,17 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, * requires a cluster-aligned start. The end may be unaligned if it is * at the end of the image (which it is here). */ - ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0); - if (ret < 0) { - error_setg_errno(errp, -ret, "Failed to zero out new clusters"); - goto fail; + if (offset > zero_start) { + ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to zero out new clusters"); + goto fail; + } } /* Write explicit zeros for the unaligned head */ if (zero_start > old_length) { - uint64_t len = zero_start - old_length; + uint64_t len = MIN(zero_start, offset) - old_length; uint8_t *buf = qemu_blockalign0(bs, len); QEMUIOVector qiov; qemu_iovec_init_buf(&qiov, buf, len); diff --git a/tests/qemu-iotests/292 b/tests/qemu-iotests/292 new file mode 100755 index 0000000000..a2de27cca4 --- /dev/null +++ b/tests/qemu-iotests/292 @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# +# Test resizing a qcow2 image with a backing file +# +# Copyright (C) 2020 Igalia, S.L. +# Author: Alberto Garcia <berto@igalia.com> +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +# creator +owner=berto@igalia.com + +seq=`basename $0` +echo "QA output created by $seq" + +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt qcow2 +_supported_proto file +_supported_os Linux + +echo '### Create the backing image' +BACKING_IMG="$TEST_IMG.base" +TEST_IMG="$BACKING_IMG" _make_test_img 1M + +echo '### Fill the backing image with data (0x11)' +$QEMU_IO -c 'write -P 0x11 0 1M' "$BACKING_IMG" | _filter_qemu_io + +echo '### Create the top image' +_make_test_img -F "$IMGFMT" -b "$BACKING_IMG" + +echo '### Fill the top image with data (0x22)' +$QEMU_IO -c 'write -P 0x22 0 1M' "$TEST_IMG" | _filter_qemu_io + +# Both offsets are part of the same cluster. +echo '### Shrink the image to 520k' +$QEMU_IMG resize --shrink "$TEST_IMG" 520k +echo '### Grow the image to 567k' +$QEMU_IMG resize "$TEST_IMG" 567k + +echo '### Check that the tail of the image reads as zeroes' +$QEMU_IO -c 'read -P 0x22 0 520k' "$TEST_IMG" | _filter_qemu_io +$QEMU_IO -c 'read -P 0x00 520k 47k' "$TEST_IMG" | _filter_qemu_io + +echo '### Show output of qemu-img map' +$QEMU_IMG map "$TEST_IMG" | _filter_testdir + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/292.out b/tests/qemu-iotests/292.out new file mode 100644 index 0000000000..807e0530c3 --- /dev/null +++ b/tests/qemu-iotests/292.out @@ -0,0 +1,24 @@ +QA output created by 292 +### Create the backing image +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576 +### Fill the backing image with data (0x11) +wrote 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +### Create the top image +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT +### Fill the top image with data (0x22) +wrote 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +### Shrink the image to 520k +Image resized. +### Grow the image to 567k +Image resized. +### Check that the tail of the image reads as zeroes +read 532480/532480 bytes at offset 0 +520 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 48128/48128 bytes at offset 532480 +47 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +### Show output of qemu-img map +Offset Length Mapped to File +0 0x8dc00 0x50000 TEST_DIR/t.qcow2 +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 1710470e70..fe649c5b73 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -298,3 +298,4 @@ 288 quick 289 rw quick 290 rw auto quick +292 rw auto quick
After commit f01643fb8b47e8a70c04bbf45e0f12a9e5bc54de when an image is extended and BDRV_REQ_ZERO_WRITE is set then the new clusters are zeroized. The code however does not detect correctly situations when the old and the new end of the image are within the same cluster. The problem can be reproduced with these steps: qemu-img create -f qcow2 backing.qcow2 1M qemu-img create -f qcow2 -F qcow2 -b backing.qcow2 top.qcow2 qemu-img resize --shrink top.qcow2 520k qemu-img resize top.qcow2 567k In the last step offset - zero_start causes an integer wraparound. Signed-off-by: Alberto Garcia <berto@igalia.com> --- v3: - Add test case [Kevin] - Move MIN(zero_start, offset) to the block that writes zeroes [Kevin] v2: - Don't call qcow2_cluster_zeroize() if offset == zero_start block/qcow2.c | 12 ++++--- tests/qemu-iotests/292 | 73 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/292.out | 24 +++++++++++++ tests/qemu-iotests/group | 1 + 4 files changed, 105 insertions(+), 5 deletions(-) create mode 100755 tests/qemu-iotests/292 create mode 100644 tests/qemu-iotests/292.out