Re: [PATCH for-5.1] qcow2: Don't open images with a backing file and the data-file-raw bit

[Max Reitz]

On 15.04.20 21:02, Alberto Garcia wrote:
> Although we cannot create these images with qemu-img it is still
> possible to do it using an external tool. QEMU should refuse to open
> them until the data-file-raw bit is cleared with 'qemu-img check'.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2.c              | 39 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/244     | 13 +++++++++++++
>  tests/qemu-iotests/244.out | 14 ++++++++++++++
>  3 files changed, 66 insertions(+)

Sorry for the long delay. :/

The patch itself looks good, but I’m not sure whether it is extensive
enough.  Let me just jump straight to the problem:

$ ./qemu-img create -f qcow2 \
    -o data_file=foo.qcow2.raw,data_file_raw=on \
    foo.qcow2 64M
(Create some file empty foo.qcow2 with external data file that’s raw)

$ ./qemu-img create -f qcow2 backing.qcow2 64M
$ ./qemu-io -c 'write -P 42 0 64M' backing.qcow2
(Create some file filled with 42s)

$ ./qemu-img compare foo.qcow2 foo.qcow2.raw
Images are identical.
(As expected, foo.qcow2 is identical to its raw data file)

$ ./qemu-img compare --image-opts \
    file.filename=foo.qcow2,backing.file.filename=backing.qcow2 \
    file.filename=foo.qcow2.raw
Content mismatch at offset 0!
(Oops.)

So when the user manually gives a backing file without one having been
given by the image file, we run into the same problem.  Now I’m not
quite sure what the problem is here.  We could make this patch more
extensive and also forbid this case.

But I think there actually shouldn’t be a problem.  The qcow2 driver
shouldn’t fall back to a backing file for raw external data files.  But
how exactly should that be implemented?  I think the correct way would
be to preallocate all metadata whenever data_file_raw=on – the qcow2
spec doesn’t say to ignore the metadata with data_file_raw=on, it just
says that the data read from the qcow2 file must match that read from
the external data file.
(I seem to remember I proposed this before, but I don’t know exactly...)

(In contrast, I don’t think it would be correct to just treat
unallocated clusters as zero whenever data_file_raw=on.)

What do you think?  Should we force preallocation with data_file_raw=on,
and then just take this patch, even though it still lets users give
backing files to a qcow2 file at runtime without error?  (Except the
backing file wouldn’t have an effect, then.)

signature.asc
Description: OpenPGP digital signature