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

[Eric Blake]

On 3/23/20 2:44 PM, 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.
>
> 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

The actual fix is much smaller than the iotest fallout ;)

> +++ b/tests/qemu-iotests/060
> @@ -167,9 +167,10 @@ _make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G
>   # 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

Instead of writing '262144' ... # 0x40000, you could write $((0x40000)) 
in-place.  Similarly for 131082 vs. 0x2000a.

Also, Max has pending patches for adding poke_file_be; if those land 
first, this becomes simpler as:

poke_file_be "$TEST_IMG" $((0x40000)) 8 0 # L2 entry
poke_file_be "$TEST_IMG" $((0x2000a)) 2 0 # Refcount entry

> +++ b/tests/qemu-iotests/289
> @@ -0,0 +1,90 @@
> +#!/usr/bin/env bash
> +#
> +# Test how 'qemu-io -c discard' behaves on v2 and v3 qcow2 images
At any rate, the new test looks reasonable to me. I see you have other 
review comments for improving it, with thos in, you can add

Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org