[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing u
Daniel P . Berrangé
Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances
Tue, 25 Feb 2020 12:50:21 +0000
On Mon, Feb 24, 2020 at 06:10:46PM +0100, Peter Krempa wrote:
> 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.
Ok, that's not viable then. We can't switch one regression for a different
> 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.
True, but that is at least historically expected behaviour, which can
be fixed by setting <backingfile/> in the XML file IIUC.
> 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.
I'm a little lost as to exactly which scenarios are broken, and which
1. file:top.qcow2 -> file:base.raw
2. file:top.qcow2 -> file:base.qcow2
3. file:top.qcow2 -> file:middle.qcow2 -> file:base.raw
4. file:top.qcow2 -> file:middle.qcow2 -> file:base.qcow2
IIUC, (1) is working before and now, (2) is working before but
broken now, and (3) and (4) were broken before and now.
So (2) is the only one we /must/ to fix
Am I right that by doing probing in libvirt as per this patch,
as well as fixing (2) though, we'll accidentally fix (3) and (4)
even though they were always broken before ?
This all talks about qcow2 images on the file: protocol driver.
What is the situation for, say, the iscsi: protocol driver
1. iscsi:top.qcow2 -> iscsi:base.raw
2. iscsi:top.qcow2 -> iscsi:base.qcow2
3. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.raw
4. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.qcow2
What was the behaviour of this in the pre-blockdev days and
vs current git master ? Is it the same with (1) is working
before and now, (2) is working before but broken now, and
(3) and (4) were broken before and now.
I'm assuming the libvirt probing cannot fix any case other
than file: protocol, or host-device: protocol, since we're
unable to open any other type of storage.
> > > 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.
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|