11:10 a.m.
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.
So to me it looks like the only viable option is to not specify the
backing file info to QEMU at all.
> Now this adds an interresting dimension to this problem. If libvirt
> forces the users to specify the image format, and the users don't know
> it they will probe. So we are basically making this a problem of
> somebody else. [2] As you can see in that patch, it uses 'qemu-img'
> anyways and also additionally actually allows the chain to continue
> deeper! [3]
Yeah, this is a really bad situation given the difficulty in safely
using qemu-img, without also breaking valid usage.
We don't want to push this off to apps
> 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.