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

[Max Reitz]

On 05.06.20 13:14, Kevin Wolf wrote:
> 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.

I’d take this patch anyway (because its motivation is just that other
programs might produce invalid images), and then not worry about the
case where we get an image produced by such another program (including
older versions of qemu) for which the user overrides the backing file at
runtime.

> The other one is that discard breaks preallocation,

The preallocation is about ensuring that there are no
fall-through-to-backing holes in the image.  Discarding doesn’t change that.

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

Perhaps.

But seeing I wondered whether I didn’t already propose this at some
point, there is another reason for preallocation:

https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00644.html
https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00329.html

All in all, I think data_file_raw should be interpretable as “You don’t
have to look at any metadata to know which data to read or write”.
(Maybe I’m wrong about that.)
Without any preallocation of metadata structure, it looks to me like we
break that promise.

(Yes, we could also force-zero the external data file during creation,
and blame users who put a backing file on images that don’t have one –
both of which are not unreasonable!  But we could also just preallocate
the metadata.)

Max

signature.asc
Description: OpenPGP digital signature