[PATCH for-4.2 0/2] qcow2: Fix QCOW2_COMPRESSED_SECTOR_MASK

[PATCH for-4.2 0/2] qcow2: Fix QCOW2_COMPRESSED_SECTOR_MASK

[Max Reitz]

This fixes a bug reported on
https://bugs.launchpad.net/qemu/+bug/1850000.  The problem is that
QCOW2_COMPRESSED_SECTOR_MASK is a 32-bit mask when it really needs to be
a 64-bit mask.

The launchpad report mentions only problems with qemu-img check on large
compressed images, but I think it might extend further than that:
- I suppose qcow2_free_any_clusters() would free every compressed offset
  modulo 4G, which isn’t good
- qcow2_update_snapshot_refcount() will probably update compressed
  cluster’s refcounts (in snapshots) modulo 4G, which also isn’t good
- And then we have check_refcount_l2() which updates the wrong clusters
  for qemu-img check (as demonstrated in the bug report)

- (qcow2_co_preadv_compressed() is safe because it uses the inverted
  mask, which of course is again just 32 bit)


But I haven’t tested those other cases.


Max Reitz (2):
  qcow2: Fix QCOW2_COMPRESSED_SECTOR_MASK
  iotests: Add test for 4G+ compressed qcow2 write

 block/qcow2.h              |  2 +-
 tests/qemu-iotests/272     | 79 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/272.out | 10 +++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 91 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/272
 create mode 100644 tests/qemu-iotests/272.out

-- 
2.21.0

[PATCH for-4.2 1/2] qcow2: Fix QCOW2_COMPRESSED_SECTOR_MASK

[Max Reitz]

Masks for L2 table entries should have 64 bit.

Fixes: b6c246942b14d3e0dec46a6c5868ed84e7dbea19
Buglink: https://bugs.launchpad.net/qemu/+bug/1850000
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 601c2e4c82..0942126232 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -83,7 +83,7 @@
 
 /* Defined in the qcow2 spec (compressed cluster descriptor) */
 #define QCOW2_COMPRESSED_SECTOR_SIZE 512U
-#define QCOW2_COMPRESSED_SECTOR_MASK (~(QCOW2_COMPRESSED_SECTOR_SIZE - 1))
+#define QCOW2_COMPRESSED_SECTOR_MASK (~(QCOW2_COMPRESSED_SECTOR_SIZE - 1ULL))
 
 /* Must be at least 2 to cover COW */
 #define MIN_L2_CACHE_SIZE 2 /* cache entries */
-- 
2.21.0

[PATCH for-4.2 2/2] iotests: Add test for 4G+ compressed qcow2 write

[Max Reitz]

Test what qemu-img check says about an image after one has written
compressed data to an offset above 4 GB.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/272     | 79 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/272.out | 10 +++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 90 insertions(+)
 create mode 100755 tests/qemu-iotests/272
 create mode 100644 tests/qemu-iotests/272.out

diff --git a/tests/qemu-iotests/272 b/tests/qemu-iotests/272
new file mode 100755
index 0000000000..c2f782d47b
--- /dev/null
+++ b/tests/qemu-iotests/272
@@ -0,0 +1,79 @@
+#!/usr/bin/env bash
+#
+# Test compressed write to a qcow2 image at an offset above 4 GB
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# 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/>.
+#
+
+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
+
+# This is a qcow2 regression test
+_supported_fmt qcow2
+_supported_proto file
+
+# External data files do not support compression;
+# We need an exact cluster size (2M) and refcount width (2) so we can
+# get this test quickly over with; and this in turn require
+# compat=1.1
+_unsupported_imgopts data_file cluster_size refcount_bits 'compat=0.10'
+
+# The idea is: Create an empty file, mark the first 4 GB as used, then
+# do a compressed write that thus must be put beyond 4 GB.
+# (This used to fail because the compressed sector mask was just a
+# 32 bit mask, so qemu-img check will count a cluster before 4 GB as
+# referenced twice.)
+
+# We would like to use refcount_bits=1 here, but then qemu-img check
+# will throw an error when trying to count a cluster as referenced
+# twice.
+_make_test_img -o cluster_size=2M,refcount_bits=2 64M
+
+reft_offs=$(peek_file_be "$TEST_IMG" 48 8)
+refb_offs=$(peek_file_be "$TEST_IMG" $reft_offs 8)
+
+# We want to cover 4 GB, those are 2048 clusters, equivalent to
+# 4096 bit = 512 B.
+truncate -s 4G "$TEST_IMG"
+for ((in_refb_offs = 0; in_refb_offs < 512; in_refb_offs += 8)); do
+    poke_file "$TEST_IMG" $((refb_offs + in_refb_offs)) \
+        '\x55\x55\x55\x55\x55\x55\x55\x55'
+done
+
+$QEMU_IO -c 'write -c -P 42 0 2M' "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo '--- Check ---'
+
+# This should only print the leaked clusters in the first 4 GB
+_check_test_img | grep -v '^Leaked cluster '
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/272.out b/tests/qemu-iotests/272.out
new file mode 100644
index 0000000000..35698b0e73
--- /dev/null
+++ b/tests/qemu-iotests/272.out
@@ -0,0 +1,10 @@
+QA output created by 272
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+--- Check ---
+
+2044 leaked clusters were found on the image.
+This means waste of disk space, but no harm to data.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 28871604cd..8ead114f7c 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -283,3 +283,4 @@
 267 rw auto quick snapshot
 268 rw auto quick
 270 rw backing quick
+272 rw
-- 
2.21.0

Re: [PATCH for-4.2 0/2] qcow2: Fix QCOW2_COMPRESSED_SECTOR_MASK

[Alberto Garcia]

On Mon 28 Oct 2019 05:18:39 PM CET, Max Reitz <mreitz@redhat.com> wrote:
> This fixes a bug reported on
> https://bugs.launchpad.net/qemu/+bug/1850000.  The problem is that
> QCOW2_COMPRESSED_SECTOR_MASK is a 32-bit mask when it really needs to be
> a 64-bit mask.

Ouch!

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH for-4.2 0/2] qcow2: Fix QCOW2_COMPRESSED_SECTOR_MASK

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1549 bytes --]

On 28.10.19 17:18, Max Reitz wrote:
> This fixes a bug reported on
> https://bugs.launchpad.net/qemu/+bug/1850000.  The problem is that
> QCOW2_COMPRESSED_SECTOR_MASK is a 32-bit mask when it really needs to be
> a 64-bit mask.
> 
> The launchpad report mentions only problems with qemu-img check on large
> compressed images, but I think it might extend further than that:
> - I suppose qcow2_free_any_clusters() would free every compressed offset
>   modulo 4G, which isn’t good
> - qcow2_update_snapshot_refcount() will probably update compressed
>   cluster’s refcounts (in snapshots) modulo 4G, which also isn’t good
> - And then we have check_refcount_l2() which updates the wrong clusters
>   for qemu-img check (as demonstrated in the bug report)
> 
> - (qcow2_co_preadv_compressed() is safe because it uses the inverted
>   mask, which of course is again just 32 bit)
> 
> 
> But I haven’t tested those other cases.
> 
> 
> Max Reitz (2):
>   qcow2: Fix QCOW2_COMPRESSED_SECTOR_MASK
>   iotests: Add test for 4G+ compressed qcow2 write
> 
>  block/qcow2.h              |  2 +-
>  tests/qemu-iotests/272     | 79 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/272.out | 10 +++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 91 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/272
>  create mode 100644 tests/qemu-iotests/272.out

Thanks for the review, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]