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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5483 bytes --]

Am 05.06.2020 um 14:11 hat Max Reitz geschrieben:
> 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.)

Makes sense. I'm not against preallocation during image creation. I just
think it shouldn't play a role in deciding whether an image is valid or
not.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]