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

[Kevin Wolf]

Am 03.06.2020 um 15:53 hat Max Reitz geschrieben:
> 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.

I guess what we should really be checking is that bs->backing is NULL
after the node is fully opened. The challenging part is that the backing
child isn't managed by the block driver, but by the generic block layer,
and .brv_open() comes first. So we don't really have a place to check
this. (And there is also the case that the image is originally opened
with BDRV_O_NO_BACKING and the later bdrv_open_backing_file().)

> 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...)

I don't find preallocation convincing, mostly for two reasons.

First is, old images or images created by another program could miss the
preallocation, but we still shouldn't access the backing file.

The other one is that discard breaks preallocation, so we would also
have to make sure to have a special case in every operation that could
end up discarding clusters (and to add it to every future operation we
might add).

It just sounds very brittle.

> (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.)

Honestly, maybe passing a backing file at runtime to an image that
doesn't logically have one is just a case of "then don't do that".

Kevin

signature.asc
Description: PGP signature