[Qemu-devel] [PATCH 0/2] qcow2: Respect new_block in alloc_refcount_block()

[Qemu-devel] [PATCH 0/2] qcow2: Respect new_block in alloc_refcount_block()

[Max Reitz]

Under certain circumstances, making the refcount table grow can result
in leaking clusters. The first patch fixes at least some of those
circumstances (maybe there are more, but these are the ones I am aware
of), and the second patch adds a test case.


Max Reitz (2):
  qcow2: Respect new_block in alloc_refcount_block()
  iotests: Add tests for refcount table growth

 block/qcow2-refcount.c     |  16 ++++++-
 tests/qemu-iotests/121     | 102 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/121.out |  23 ++++++++++
 tests/qemu-iotests/group   |   1 +
 4 files changed, 140 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/121
 create mode 100644 tests/qemu-iotests/121.out

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] qcow2: Respect new_block in alloc_refcount_block()

[Max Reitz]

When choosing a new place for the refcount table, alloc_refcount_block()
tries to infer the number of clusters used so far from its argument
cluster_index (which comes from the idea that if any cluster with an
index greater than cluster_index was in use, the refcount table would
have to be big enough already to describe cluster_index).

However, there is a cluster that may be at or after cluster_index, and
which is not covered by the refcount structures, and that is the new
refcount block new_block. Therefore, it should be taken into account for
the blocks_used calculation.

Also, because new_block already describes (or is intended to describe)
cluster_index, we may not put the new refcount structures there.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 9b80ca7..b956365 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -326,8 +326,20 @@ static int alloc_refcount_block(BlockDriverState *bs,
      */
     BLKDBG_EVENT(bs->file, BLKDBG_REFTABLE_GROW);
 
-    /* Calculate the number of refcount blocks needed so far */
-    uint64_t blocks_used = DIV_ROUND_UP(cluster_index, s->refcount_block_size);
+    /* Calculate the number of refcount blocks needed so far; this will be the
+     * basis for calculating the index of the first cluster used for the
+     * self-describing refcount structures which we are about to create.
+     *
+     * Because we reached this point, there cannot be any refcount entries for
+     * cluster_index or higher indices yet. However, because new_block has been
+     * allocated to describe that cluster (and it will assume this role later
+     * on), we cannot use that index; also, new_block may actually have a higher
+     * cluster index than cluster_index, so it needs to be taken into account
+     * here (and 1 needs to be added to its value because that cluster is used).
+     */
+    uint64_t blocks_used = DIV_ROUND_UP(MAX(cluster_index + 1,
+                                            (new_block >> s->cluster_bits) + 1),
+                                        s->refcount_block_size);
 
     if (blocks_used > QCOW_MAX_REFTABLE_SIZE / sizeof(uint64_t)) {
         return -EFBIG;
-- 
2.1.0

[Qemu-devel] [PATCH 2/2] iotests: Add tests for refcount table growth

[Max Reitz]

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

diff --git a/tests/qemu-iotests/121 b/tests/qemu-iotests/121
new file mode 100755
index 0000000..0912c3f
--- /dev/null
+++ b/tests/qemu-iotests/121
@@ -0,0 +1,102 @@
+#!/bin/bash
+#
+# Test cases for qcow2 refcount table growth
+#
+# Copyright (C) 2015 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/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+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
+echo '=== New refcount structures may not conflict with existing structures ==='
+
+echo
+echo '--- Test 1 ---'
+echo
+
+# Preallocation speeds up the write operation, but preallocating everything will
+# destroy the purpose of the write; so preallocate one KB less than what would
+# cause a reftable growth...
+IMGOPTS='preallocation=metadata,cluster_size=1k' _make_test_img 64512K
+# ...and make the image the desired size afterwards.
+$QEMU_IMG resize "$TEST_IMG" 65M
+
+# The first write results in a growth of the refcount table during an allocation
+# which has precisely the required size so that the new refcount block allocated
+# in alloc_refcount_block() is right after cluster_index; this did lead to a
+# different refcount block being written to disk (a zeroed cluster) than what is
+# cached (a refblock with one entry having a refcount of 1), and the second
+# write would then result in that cached cluster being marked dirty and then
+# in it being written to disk.
+# This should not happen, the new refcount structures may not conflict with
+# new_block.
+# (Note that for some reason, 'write 63M 1K' does not trigger the problem)
+$QEMU_IO -c 'write 62M 1025K' -c 'write 64M 1M' "$TEST_IMG" | _filter_qemu_io
+
+_check_test_img
+
+
+echo
+echo '--- Test 2 ---'
+echo
+
+IMGOPTS='preallocation=metadata,cluster_size=1k' _make_test_img 64513K
+# This results in an L1 table growth which in turn results in some clusters at
+# the start of the image becoming free
+$QEMU_IMG resize "$TEST_IMG" 65M
+
+# This write results in a refcount table growth; but the refblock allocated
+# immediately before that (new_block) takes cluster index 4 (which is now free)
+# and is thus not self-describing (in contrast to test 1, where new_block was
+# self-describing). The refcount table growth algorithm then used to place the
+# new refcount structures at cluster index 65536 (which is the same as the
+# cluster_index parameter in this case), allocating a new refcount block for
+# that cluster while new_block already existed, leaking new_block.
+# Therefore, the new refcount structures may not be put at cluster_index
+# (because new_block already describes that cluster, and the new structures try
+# to be self-describing).
+$QEMU_IO -c 'write 63M 130K' "$TEST_IMG" | _filter_qemu_io
+
+_check_test_img
+
+
+# success, all done
+echo
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/121.out b/tests/qemu-iotests/121.out
new file mode 100644
index 0000000..ff18e2c
--- /dev/null
+++ b/tests/qemu-iotests/121.out
@@ -0,0 +1,23 @@
+QA output created by 121
+
+=== New refcount structures may not conflict with existing structures ===
+
+--- Test 1 ---
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=66060288 preallocation='metadata'
+Image resized.
+wrote 1049600/1049600 bytes at offset 65011712
+1.001 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 67108864
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+
+--- Test 2 ---
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=66061312 preallocation='metadata'
+Image resized.
+wrote 133120/133120 bytes at offset 66060288
+130 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 4b2b93b..7b74c27 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -117,3 +117,4 @@
 113 rw auto quick
 114 rw auto quick
 116 rw auto quick
+121 rw auto
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/2] qcow2: Respect new_block in alloc_refcount_block()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

On 02/10/2015 01:02 PM, Max Reitz wrote:
> Under certain circumstances, making the refcount table grow can result
> in leaking clusters. The first patch fixes at least some of those
> circumstances (maybe there are more, but these are the ones I am aware
> of), and the second patch adds a test case.
> 
> 
> Max Reitz (2):
>   qcow2: Respect new_block in alloc_refcount_block()
>   iotests: Add tests for refcount table growth

Series:
Reviewed-by: Eric Blake <eblake@redhat.com>

How'd you find the leak?

> 
>  block/qcow2-refcount.c     |  16 ++++++-
>  tests/qemu-iotests/121     | 102 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/121.out |  23 ++++++++++
>  tests/qemu-iotests/group   |   1 +
>  4 files changed, 140 insertions(+), 2 deletions(-)
>  create mode 100755 tests/qemu-iotests/121
>  create mode 100644 tests/qemu-iotests/121.out
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 0/2] qcow2: Respect new_block in alloc_refcount_block()

[Max Reitz]

On 2015-02-10 at 17:19, Eric Blake wrote:
> On 02/10/2015 01:02 PM, Max Reitz wrote:
>> Under certain circumstances, making the refcount table grow can result
>> in leaking clusters. The first patch fixes at least some of those
>> circumstances (maybe there are more, but these are the ones I am aware
>> of), and the second patch adds a test case.
>>
>>
>> Max Reitz (2):
>>    qcow2: Respect new_block in alloc_refcount_block()
>>    iotests: Add tests for refcount table growth
> Series:
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> How'd you find the leak?

Test 015 failed with refcount_bits=32 and refcount_bits=64 after my 
refcount_order series; but strangely only if the recent qcow2 cache 
patch 
(http://lists.nongnu.org/archive/html/qemu-devel/2015-02/msg00786.html) 
was applied before.

Max

Re: [Qemu-devel] [PATCH 0/2] qcow2: Respect new_block in alloc_refcount_block()

[Kevin Wolf]

Am 10.02.2015 um 21:02 hat Max Reitz geschrieben:
> Under certain circumstances, making the refcount table grow can result
> in leaking clusters. The first patch fixes at least some of those
> circumstances (maybe there are more, but these are the ones I am aware
> of), and the second patch adds a test case.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] qcow2: Respect new_block in alloc_refcount_block()

[Max Reitz]

On 2015-02-10 at 15:02, Max Reitz wrote:
> Under certain circumstances, making the refcount table grow can result
> in leaking clusters. The first patch fixes at least some of those
> circumstances (maybe there are more, but these are the ones I am aware
> of), and the second patch adds a test case.
>
>
> Max Reitz (2):
>    qcow2: Respect new_block in alloc_refcount_block()
>    iotests: Add tests for refcount table growth
>
>   block/qcow2-refcount.c     |  16 ++++++-
>   tests/qemu-iotests/121     | 102 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/121.out |  23 ++++++++++
>   tests/qemu-iotests/group   |   1 +
>   4 files changed, 140 insertions(+), 2 deletions(-)
>   create mode 100755 tests/qemu-iotests/121
>   create mode 100644 tests/qemu-iotests/121.out

Applied to my block tree:

https://github.com/XanClic/qemu/commits/block