Re: [PATCH] qcow2: Forbid discard in qcow2 v2 images with backing files

[Max Reitz]

On 23.03.20 20:44, Alberto Garcia wrote:
> A discard request deallocates the selected clusters so they read back
> as zeroes. This is done by clearing the cluster offset field and
> setting QCOW_OFLAG_ZERO in the L2 entry.
> 
> This flag is however only supported when qcow_version >= 3. In older
> images the cluster is simply deallocated, exposing any possible stale
> data from the backing file.
> 
> Since discard is an advisory operation it's safer to simply forbid it
> in this scenario.
> 
> Note that we are adding this check to qcow2_co_pdiscard() and not to
> qcow2_cluster_discard() or discard_in_l2_slice() because the last
> two are also used by qcow2_snapshot_create() to discard the clusters
> used by the VM state. In this case there's no risk of exposing stale
> data to the guest and we really want that the clusters are always
> discarded.

Sounds good to me.

> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block/qcow2.c              |  6 +++
>  tests/qemu-iotests/060     |  5 ++-
>  tests/qemu-iotests/060.out |  2 -
>  tests/qemu-iotests/289     | 90 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/289.out | 52 ++++++++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 152 insertions(+), 4 deletions(-)
>  create mode 100755 tests/qemu-iotests/289
>  create mode 100644 tests/qemu-iotests/289.out
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d44b45633d..7bb7e392e1 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3763,6 +3763,12 @@ static coroutine_fn int 
> qcow2_co_pdiscard(BlockDriverState *bs,
>      int ret;
>      BDRVQcow2State *s = bs->opaque;
>  
> +    /* If the image does not support QCOW_OFLAG_ZERO then discarding
> +     * clusters could expose stale data from the backing file. */
> +    if (s->qcow_version < 3 && bs->backing) {
> +        return -ENOTSUP;
> +    }
> +
>      if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size)) {
>          assert(bytes < s->cluster_size);
>          /* Ignore partial clusters, except for the special case of the
> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> index 043f12904a..4a4fdfb1e1 100755
> --- a/tests/qemu-iotests/060
> +++ b/tests/qemu-iotests/060
> @@ -167,9 +167,10 @@ _make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G

More context: This image is created with -o 'compat=0.10', just because
a discard on such an image would result in the cluster being freed.  We
can drop that compat=0.10 bit now.

>  # Write two clusters, the second one enforces creation of an L2 table after
>  # the first data cluster.
>  $QEMU_IO -c 'write 0k 64k' -c 'write 512M 64k' "$TEST_IMG" | _filter_qemu_io
> -# Discard the first cluster. This cluster will soon enough be reallocated and
> +# Free the first cluster. This cluster will soon enough be reallocated and
>  # used for COW.
> -$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
> +poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x40000 
> - L2 entry
> +poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry
>  # Now, corrupt the image by marking the second L2 table cluster as free.
>  poke_file "$TEST_IMG" '131084' "\x00\x00" # 0x2000c
>  # Start a write operation requiring COW on the image stopping it right before

[...]

> diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
> new file mode 100755
> index 0000000000..13b4984721
> --- /dev/null
> +++ b/tests/qemu-iotests/289

[...]

> +_cleanup()
> +{
> +    _cleanup_test_img
> +    rm -f "$TEST_IMG.backing"

I’d call the image $TEST_IMG.base so _cleanup_test_img picks up on it.
(rm-ing test images is also wrong, because with external data files,
there will be more than one file.  It doesn’t matter here anyway because
this test doesn’t support external data files, but, well.)

> +}
> +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

I’d mark the compat option unsupported because this test will ignore it.
 Furthermore, the refcount_bits and data_file options are really
unsupported, because they won’t work with compat=0.10.

The test itself looks good.

Max

signature.asc
Description: OpenPGP digital signature