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

[Eric Blake]

On 3/27/20 11:48 AM, 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>
> ---

> +++ b/tests/qemu-iotests/290

> +
> +echo
> +echo "### Test 'qemu-io -c discard' on a QCOW2 image without a backing file"
> +echo
> +for qcow2_compat in 0.10 1.1; do
> +    echo "# Create an image with compat=$qcow2_compat without a backing file"
> +    _make_test_img -o "compat=$qcow2_compat" 128k
> +
> +    echo "# Fill all clusters with data and then discard them"
> +    $QEMU_IO -c 'write -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
> +    $QEMU_IO -c 'discard 0 128k' "$TEST_IMG" | _filter_qemu_io
> +
> +    echo "# Read the data from the discarded clusters"
> +    $QEMU_IO -c 'read -P 0x00 0 128k' "$TEST_IMG" | _filter_qemu_io
> +done

Should this loop also inspect qemu-img map output?

> +
> +echo
> +echo "### Test 'qemu-io -c discard' on a QCOW2 image with a backing file"
> +echo
> +
> +echo "# Create a backing image and fill it with data"
> +BACKING_IMG="$TEST_IMG.base"
> +TEST_IMG="$BACKING_IMG" _make_test_img 128k
> +$QEMU_IO -c 'write -P 0xff 0 128k' "$BACKING_IMG" | _filter_qemu_io
> +
> +for qcow2_compat in 0.10 1.1; do
> +    echo "# Create an image with compat=$qcow2_compat and a backing file"
> +    _make_test_img -o "compat=$qcow2_compat" -b "$BACKING_IMG"
> +
> +    echo "# Fill all clusters with data and then discard them"
> +    $QEMU_IO -c 'write -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
> +    $QEMU_IO -c 'discard 0 128k' "$TEST_IMG" | _filter_qemu_io
> +
> +    echo "# Read the data from the discarded clusters"
> +    if [ "$qcow2_compat" = "1.1" ]; then
> +        # In qcow2 v3 clusters are zeroed (with QCOW_OFLAG_ZERO)
> +        $QEMU_IO -c 'read -P 0x00 0 128k' "$TEST_IMG" | _filter_qemu_io
> +    else
> +        # In qcow2 v2 if there's a backing image we cannot zero the clusters
> +        # without exposing the backing file data so discard does nothing
> +        $QEMU_IO -c 'read -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
> +    fi
> +
> +    echo "# Output of qemu-img map"
> +    $QEMU_IMG map "$TEST_IMG" | _filter_testdir
> +done

But I agree this was the more interesting one, so we at least have 
decent coverage of the change itself.

Reviewed-by: Eric Blake <address@hidden>

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