On 2018-03-29 11:39, Alberto Garcia wrote: > On Wed 28 Mar 2018 07:34:15 PM CEST, Max Reitz wrote: >>> diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122 >>> index 45b359c2ba..5b9593016c 100755 >>> --- a/tests/qemu-iotests/122 >>> +++ b/tests/qemu-iotests/122 >> >> Not sure if 122 is the right file for this... >> >> Or, let me rephrase, it does look to me like it is not the right file. >> But on the other hand, I don't see a more suitable file. > > Yeah I was tempted to create a new one, but in the end I decided to put > it there. We can still create a new one if you feel strongly about it. > >>> echo >>> +echo "=== Corrupted size field in compressed cluster descriptor ===" >>> +echo >>> +# Create an empty image, fill half of it with data and compress it. >>> +# The L2 entries of the two compressed clusters are located at >>> +# 0x800000 and 0x800008, their original values are 0x4008000000a00000 >>> +# and 0x4008000000a00802 (5 sectors for compressed data each). >>> +TEST_IMG="$TEST_IMG".1 _make_test_img 8M >>> +$QEMU_IO -c "write -P 0x11 0 4M" "$TEST_IMG".1 2>&1 | _filter_qemu_io | >>> _filter_testdir >>> +$QEMU_IMG convert -c -O qcow2 -o cluster_size=2M "$TEST_IMG".1 "$TEST_IMG" >> >> Why not just use "write -c" and drop the convert? (You'd have to use >> two writes, though, one for each cluster.) > > Yeah, it's also possible, you have to do it in the same qemu-io command > though, else it will allocate two host clusters. But yes, I can change > that. > >>> +# Reduce size of compressed data to 4 sectors: this corrupts the image. >>> +poke_file "$TEST_IMG" $((0x800000)) "\x40\x06" >>> +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | >>> _filter_testdir >>> + >>> +# 'qemu-img check' however doesn't see anything wrong because it >>> +# doesn't try to decompress the data and the refcounts are consistent. >>> +# TODO: update qemu-img so this can be detected >>> +_check_test_img >>> + >>> +# Increase size of compressed data to the maximum (8192 sectors). >>> +# This makes QEMU read more data (8192 sectors instead of 5, host >>> +# addresses [0xa00000, 0xdfffff]), but the decompression algorithm >>> +# stops once we have enough to restore the uncompressed cluster, so >>> +# the rest of the data is ignored. >>> +poke_file "$TEST_IMG" $((0x800000)) "\x7f\xfe" >>> +# Do it also for the second compressed cluster (L2 entry at 0x800008). >>> +# In this case the compressed data would span 3 host clusters >>> +# (host addresses: [0xa00802, 0xe00801]) >>> +poke_file "$TEST_IMG" $((0x800008)) "\x7f\xfe" >>> + >>> +# Here the image is too small so we're asking QEMU to read beyond the >>> +# end of the image. >>> +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | >>> _filter_testdir >>> +# But if we grow the image we won't be reading beyond its end anymore. >>> +$QEMU_IO -c "write -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | >>> _filter_testdir >>> +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | >>> _filter_testdir >> >> Both reads result in exactly the same output, though, so it doesn't >> seem like qemu cares. >> >> (This is the reason I'm not merging this patch now, because that looks >> a bit fishy.) > > In this example the size stored on the compressed cluster descriptor is > corrupted and larger than the size of the compressed data on disk. > > In the first read the image is too small so this makes QEMU read not > only past the end of the compressed data, but also past the end of the > qcow2 image itself. > > In the second read the qcow2 image is larger so QEMU reads actual data > from the subsequent clusters. > > It doesn't make much difference. In the first case the buffer is padded > with zeroes and in the second with data from other clusters, but QEMU > only uses the data needed to restore the original uncompressed cluster > and the rest is ignored. But I thought I could still test both cases, > that's why there's two reads.
Ah, OK. I would have preferred a comment then so that people know it's just expected to work. >>> +# The refcount data is however wrong because due to the increased size >>> +# of the compressed data it now reaches the following host clusters. >>> +# This can be repaired by qemu-img check. >> >> The OFLAG_COPIED repair looks a bit interesting, but, er, well. >> >> Max >> >> (Since a compressed cluster does not correspond 1:1 to a host cluster, >> you cannot say that a compressed cluster has a refcount -- only host >> clusters are refcounted. So what is it supposed to mean that a >> compressed cluster has a refcount of 1? > > Compressed clusters never have OFLAG_COPIED and we treat it as an error > if they do (see check_refcounts_l2()), Interesting. Wonder why the qcow2 specification doesn't mention that... > but their host clusters still > have a reference count. > > A host cluster with 4 compressed clusters has a reference count of 4. > > A compressed cluster that uses two host clusters (even if it's only > partially) increases the reference count of both of them. > > In this test case the size field of the compressed cluster is too large, > so it includes uncompressed clusters that are contiguous to it. The > uncompressed clusters have refcount == 1 and OFLAG_COPIED, but QEMU > detects that the refcount should be larger and that OFLAG_COPIED should > not be there, that's why it's repaired. Aaah, OK, I see. Thanks for explaining. > Admittedly the way it's repaired is not the best one: the uncompressed > clusters have now extra references, but other than that I don't see any > harmful effect. > > The proper fix for this would be to detect that the compressed cluster > size is incorrect and correct its value. There's a TODO for that (in the > first _check_test_img call where it's more severe), but now that I think > of it I should probably mention that in the second one as well. Well, the current repair method will at least prevent data loss, so I'm OK with it. Max
signature.asc
Description: OpenPGP digital signature
