Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

On Mon, Feb 24, 2020 at 14:24:15 +0000, Daniel Berrange wrote: > On Mon, Feb 24, 2020 at 02:34:16PM +0100, Peter Krempa wrote: > > On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote: [...] > > I'll reiterate the historical state of the problem because I think it's > > important: > > > > Pre-blockdev: > > - we internally assumed that if the image format of an backing image > > was not present in the overlay it is 'raw' > > - this influenced security labelling but not actually how qemu viewed > > or probed the file. If it was qcow2 probed as qcow2 qemu opened it > > as qcow2 possibly even including the backing file if selinux or > > other mechanism didn't prevent it. > > > > post-blockdev: > > - the assumption of 'raw' would now be expressed into the qemu > > configuration. This assumption turned into data corruption since we > > no longer allowed qemu to probe the format and forced it as raw. > > - fix was to always require the format to be recorded in the overlay > > - this made users unhappy who neglected to record the format into the > > overlay when creating it manually > > So the key problem we have is that with -blockdev we are always explicitly > telling QEMU what the backing file is for every image. > > Can we fix this to have the exact same behaviour as before by *not* telling > QEMU anything about the backing file when using -blockdev, if there is no > well defined backing format present. ie, use -blockdev, but let QEMU probe > just as it did in non-blockdev days. > > Would there be any downsides to this that did not already exist in the > non-blockdev days ? We can, but the price is that: 1) we won't allow blockjobs and anything blockdev-related because node name would be out of our control. This was possible in pre-blockdev era.

2) we will lose control of actually telling qemu to NOT open the backing file in that case. Distros using only unix permission still have arbitrary file access under permissions of the qemu process.

3) weird special-case code, because we need to keep some metadata about the image to do security labelling > I don't think we can solve the regressions in behaviour of backing files > by doing probing of the backing files in libvirt, because that only works > for the case where libvirt can actually open the file. ie a local file on > disk. We don't have logic for opening backing files on RBD, GlusterFS, > iSCSI, HTTP, SSH, etc, and nor do we want todo that. Now we are back in the teritory where we actually do match what would happen with previously. We don't specify these on the command line with ehaviour matching what's described above, with the caveats as above. I kept this behaviour because we couldn't do better. This is in place even now if the last introspectable image has valid format specified. We can reconsider how to approach this but ideally separately.

> > This boils down to whether we want to accept some possibility of image > > corruption in trade for avoiding regression of behaviour in the secure > > cases as well as management apps and users not having to re-invent when > > probing an image is actually safe. > > I feel like the risk of image corruption is pretty minor. Our probing > handles all normal cases the same way as QEMU and newly introduced > image formats are rare. Well, in this case I'm actually for re-considering the original patch discussed here. It uses image-format-probing code from libvirt, to allow the most common cases which were forbidden in a safe way. This means that as long as we can probe the image and the probed image does not have a backing file we allow the startup. It restores previous behaviour for valid cases including blockjobs, correctly revokes invalid cases (existing chain after image wihtout format, images impossible to introspect), is limited to the backing store walking code so can be contained and the price is doing the image format detection using libvirt's code.