[libvirt] [PATCH 0/4] qemu: Error out if backing image format is not recorded in image metadata
See patch 3/4 for explanation. Peter Krempa (4): tests: storage: Use strict version of virStorageFileGetMetadata tests: storage: Remove unused test modes util: storage: Don't treat files with missing backing store format as 'raw' kbase: Add document outlining backing chain XML config and troubleshooting docs/kbase.html.in | 4 + docs/kbase/backing_chains.rst | 185 ++++++++++++++++++++++++++++++++++ src/util/virstoragefile.c | 21 +++- tests/virstoragetest.c | 42 +++----- 4 files changed, 221 insertions(+), 31 deletions(-) create mode 100644 docs/kbase/backing_chains.rst -- 2.23.0
Pass in 'true' as '@report_broken' of virStorageFileGetMetadata to make it fail in the tests. The most important code paths (when starting the VM) expect this function to fail rather than silently return partial data. Switch the test to exercise this more important code path. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virstoragetest.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 407378bab4..73717b0460 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -103,7 +103,7 @@ testStorageFileGetMetadata(const char *path, def->path = g_strdup(path); - if (virStorageFileGetMetadata(def, uid, gid, false) < 0) + if (virStorageFileGetMetadata(def, uid, gid, true) < 0) return NULL; return g_steal_pointer(&def); @@ -775,7 +775,7 @@ mymain(void) qcow2.expBackingStoreRaw = datadir "/bogus"; /* Qcow2 file with missing backing file but specified type */ - TEST_CHAIN(absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_WARN); + TEST_CHAIN(absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_FAIL); /* Rewrite qcow2 to a missing backing file, without backing type */ virCommandFree(cmd); @@ -785,7 +785,7 @@ mymain(void) ret = -1; /* Qcow2 file with missing backing file and no specified type */ - TEST_CHAIN(absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_WARN); + TEST_CHAIN(absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_FAIL); /* Rewrite qcow2 to use an nbd: protocol as backend */ virCommandFree(cmd); @@ -916,7 +916,7 @@ mymain(void) qcow2.expBackingStoreRaw = "qcow2"; /* Behavior of an infinite loop chain */ - TEST_CHAIN(absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_WARN); + TEST_CHAIN(absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_FAIL); /* Rewrite wrap and qcow2 to be mutually-referential loop */ virCommandFree(cmd); @@ -933,7 +933,7 @@ mymain(void) qcow2.expBackingStoreRaw = "wrap"; /* Behavior of an infinite loop chain */ - TEST_CHAIN(abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2), EXP_WARN); + TEST_CHAIN(abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2), EXP_FAIL); /* Rewrite qcow2 to use an rbd: protocol as backend */ virCommandFree(cmd); -- 2.23.0
EXP_WARN and ALLOW_PROBE flags for the testStorageChain cases are no longer used so we can remove them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virstoragetest.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 73717b0460..93f3d1604a 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -242,8 +242,6 @@ struct _testFileData enum { EXP_PASS = 0, EXP_FAIL = 1, - EXP_WARN = 2, - ALLOW_PROBE = 4, }; struct testChainData @@ -288,25 +286,15 @@ testStorageChain(const void *args) fprintf(stderr, "call should have failed\n"); return -1; } - if (data->flags & EXP_WARN) { - if (virGetLastErrorCode() == VIR_ERR_OK) { - fprintf(stderr, "call should have warned\n"); - return -1; - } - virResetLastError(); - if (virStorageFileChainGetBroken(meta, &broken) || !broken) { - fprintf(stderr, "call should identify broken part of chain\n"); - return -1; - } - } else { - if (virGetLastErrorCode()) { - fprintf(stderr, "call should not have warned\n"); - return -1; - } - if (virStorageFileChainGetBroken(meta, &broken) || broken) { - fprintf(stderr, "chain should not be identified as broken\n"); - return -1; - } + + if (virGetLastErrorCode()) { + fprintf(stderr, "call should not have reported error\n"); + return -1; + } + + if (virStorageFileChainGetBroken(meta, &broken) || broken) { + fprintf(stderr, "chain should not be identified as broken\n"); + return -1; } elt = meta; -- 2.23.0
Assuming that the backing image format is raw is wrong when doing image detection: 1) In -drive mode qemu will still probe the image format of the backing image. This means it will try to open a backing file of the image which will fail if a more advanced security model is in use. 2) In blockdev mode the image will be opened as raw actually which is wrong since it might be qcow. Not opening the backing images will also end up in the guest seeing corrupted data. Rather than attempt to solve various corner cases when us assuming the storage file being raw and actually being right forbid startup when the guest image doesn't have the format specified in the metadata. https://bugzilla.redhat.com/show_bug.cgi?id=1588373 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 21 +++++++++++++++++---- tests/virstoragetest.c | 2 +- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3a3de314b8..e280a646b7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4981,12 +4981,25 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, goto cleanup; } - if (backingFormat == VIR_STORAGE_FILE_AUTO) + backingStore->format = backingFormat; + + if (backingStore->format == VIR_STORAGE_FILE_AUTO) { + /* Assuming the backing store to be raw can lead to failures. We do + * it only when we must not report an error to prevent losing VMs. + * Otherwise report an error. + */ + if (report_broken) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("format of backing image '%s' of image '%s' was not specified in the image metadata"), + src->backingStoreRaw, NULLSTR(src->path)); + return -1; + } + backingStore->format = VIR_STORAGE_FILE_RAW; - else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) + } + + if (backingStore->format == VIR_STORAGE_FILE_AUTO_SAFE) backingStore->format = VIR_STORAGE_FILE_AUTO; - else - backingStore->format = backingFormat; if ((ret = virStorageFileGetMetadataRecurse(backingStore, parent, uid, gid, diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 93f3d1604a..b9d4a45cdd 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -751,7 +751,7 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; TEST_CHAIN(abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap_as_raw, &qcow2_as_raw), EXP_PASS); + (&wrap_as_raw, &qcow2_as_raw), EXP_FAIL); /* Rewrite qcow2 to a missing backing file, with backing type */ virCommandFree(cmd); -- 2.23.0
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/kbase.html.in | 4 + docs/kbase/backing_chains.rst | 185 ++++++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+) create mode 100644 docs/kbase/backing_chains.rst diff --git a/docs/kbase.html.in b/docs/kbase.html.in index a5504a540f..c156414c41 100644 --- a/docs/kbase.html.in +++ b/docs/kbase.html.in @@ -25,6 +25,10 @@ <dt><a href="kbase/rpm-deployment.html">RPM deployment</a></dt> <dd>Explanation of the different RPM packages and illustration of which to pick for installation</dd> + + <dt><a href="kbase/backing_chains.html">Backing chain management</a></dt> + <dd>Explanation of how disk backing chain specification impacts libvirt's + behaviour and basic troubleshooting steps of disk problems.</dd> </dl> </div> diff --git a/docs/kbase/backing_chains.rst b/docs/kbase/backing_chains.rst new file mode 100644 index 0000000000..ec8f1b33b8 --- /dev/null +++ b/docs/kbase/backing_chains.rst @@ -0,0 +1,185 @@ +================= +Disk image chains +================= + +Modern disk image formats allow users to create an overlay on top of an +existing image which will be the target of the new guest writes. This allows us +to do snapshots of the disk state of a VM efficiently. The following text +describes how libvirt manages such image chains and some problems which can +occur. Note that many of the cases mentioned below are currently only relevant +for the qemu driver. + +.. contents:: + +Domain XML image and chain specification +======================================== + +Disk image chains can be partially or fully configured in the domain XML. The +basic approach is to use the ``<backingStore>`` elements in the configuration. + +The ``<backingStore>`` elements present in the live VM xml represent the actual +topology that libvirt knows of. + +Basic disk setup +---------------- + +Any default configuration or example usually refers only to the top (active) +image of the backing chain. + +:: + + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/tmp/pull4.qcow2'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + </disk> + +This configuration will prompt libvirt to detect the backing image of the source +image and recursively do the same thing until the end of the chain. + +Importance of properly setup backing chain +------------------------------------------ + +The disk image locations are used by libvirt to properly setup the security +system used on the host so that the hypervisor can access the files and possibly +also directly to configure the hypervisor to use the appropriate images. Thus +it's important to properly setup the formats and paths of the backing images. + +Image detection caveats +----------------------- + +Detection of the backing chain requires libvirt to read and understand the +``backing file`` field recorded in the image metadata and also being able to +recurse and read the backing file. Due to security implications libvirt +will not attempt to detect the format of the backing image if the image metadata +don't contain it. + +Libvirt also may not support a network disk storage technology and thus may be +unable to visit and detect backing chains on such storage. This may result in +the backing chain present in the live XML to look incomplete or some operations +not being possible. To prevent this it's possible to specify the image metadata +explicitly in the XML. + +Advanced backing chain specifications +------------------------------------- + +To specify the topology of disk images explicitly the following XML +specification can be used: + +:: + + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/tmp/pull4.qcow2'/> + <backingStore type='file'> + <format type='qcow2'/> + <source file='/tmp/pull3.qcow2'/> + <backingStore type='file'> + <format type='qcow2'/> + <source file='/tmp/pull2.qcow2'/> + <backingStore type='file'> + <format type='qcow2'/> + <source file='/tmp/pull1.qcow2'/> + <backingStore type='file'> + <format type='qcow2'/> + <source file='/tmp/pull0.qcow2'/> + <backingStore/> + </backingStore> + </backingStore> + </backingStore> + </backingStore> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + </disk> + +This makes libvirt follow the settings as configured in the XML. Note that this +is supported only when the https://libvirt.org/formatdomaincaps.html#featureBackingStoreInput +capability is present. + +An empty ``<backingStore/>`` element signals the end of the chain. Using this +will prevent libvirt or qemu from probing the backing chain. + +Note that it's also possible to partially specify the chain in the XML but omit +the terminating element. This will result into probing from the last specified +``<backingStore>`` + + +Manual image creation +===================== + +When creating disk images manually outside of libvirt it's important to create +them properly so that they work with libvirt as expected. The created disk +images must contain the format of the backing image in the metadata. This +means that the **-F** parameter of ``qemu-img`` must always be used. + +Troubleshooting +=============== + +Few common problems which occur when managing chains of disk images. + +VM refuses to start due to misconfigured backing store format +------------------------------------------------------------- + +This problem happens on VMs where the backing chain was created manually outside +of libvirt and can have multiple symptoms: + +- permission denied error reported on a backing image +- ``format of backing image '%s' of image '%s' was not specified in the image metadata`` error reported +- disk image looking corrupt inside the guest + +The cause of the above problem is that the image metadata does not record the +format of the backing image along with the location of the image. When the +format is not specified libvirt or qemu would have to do image format probing +which is insecure to do as a mallicious guest could rewrite the header of the +disk leading to access of host files. Libvirt thus does not try to assume +the format of the backing image. The following command can be used to check if +the image has backing image format specified: + +:: + + $ qemu-img info /tmp/copy4.qcow2 + image: /tmp/copy4.qcow2 + file format: qcow2 + virtual size: 10 MiB (10485760 bytes) + disk size: 196 KiB + cluster_size: 65536 + backing file: copy3.qcow2 (actual path: /tmp/copy3.qcow2) + backing file format: qcow2 + Format specific information: + compat: 1.1 + lazy refcounts: false + refcount bits: 16 + corrupt: false + +If the ``backing file format:`` field is missing above the format was not +specified properly. The image can be fixed by the following command: + +:: + + qemu-img rebase -f $IMAGE_FORMAT -F $BACKING_IMAGE_FORMAT -b $BACKING_IMAGE_PATH $IMAGE_PATH + +It is important to fill out ``$BACKING_IMAGE_FORMAT`` and ``$IMAGE_FORMAT`` +properly. ``$BACKING_IMAGE_PATH`` should be specified as a full absolute path. +If relative referencing of the backing image is desired, the path must be +relative to the location of image described by ``$IMAGE_PATH``. + +Missing images reported after after moving disk images into a different path +---------------------------------------------------------------------------- + +The path to the backing image which is recorded in the image metadata often +contains a full path to the backing image. This is the default libvirt-created +image configuration. When such images are moved to a different location the +top image will no longer point to the correct image. + +To fix such issue you can either fully specify the image chain in the domain XML +as pointed out above or the following ``qemu-img`` command can be used: + +:: + + qemu-img rebase -u -f $IMAGE_FORMAT -F $BACKING_IMAGE_FORMAT -b $BACKING_IMAGE_PATH $IMAGE_PATH + +It is important to fill out ``$BACKING_IMAGE_FORMAT`` and ``$IMAGE_FORMAT`` +properly. ``$BACKING_IMAGE_PATH`` should be specified as a full absolute path. +If relative referencing of the backing image is desired, the path must be +relative to the location of image described by ``$IMAGE_PATH``. -- 2.23.0
On 12/17/19 12:35 PM, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
+Image detection caveats +----------------------- + +Detection of the backing chain requires libvirt to read and understand the +``backing file`` field recorded in the image metadata and also being able to +recurse and read the backing file. Due to security implications libvirt +will not attempt to detect the format of the backing image if the image metadata +don't contain it.
s/don't/doesn't/
+ +Libvirt also may not support a network disk storage technology and thus may be
s/also may not support/also might lack support for/
+unable to visit and detect backing chains on such storage. This may result in +the backing chain present in the live XML to look incomplete or some operations +not being possible. To prevent this it's possible to specify the image metadata +explicitly in the XML. +
+Troubleshooting +=============== + +Few common problems which occur when managing chains of disk images.
s/Few/A few/
+ +VM refuses to start due to misconfigured backing store format +------------------------------------------------------------- + +This problem happens on VMs where the backing chain was created manually outside +of libvirt and can have multiple symptoms: + +- permission denied error reported on a backing image +- ``format of backing image '%s' of image '%s' was not specified in the image metadata`` error reported +- disk image looking corrupt inside the guest + +The cause of the above problem is that the image metadata does not record the +format of the backing image along with the location of the image. When the +format is not specified libvirt or qemu would have to do image format probing +which is insecure to do as a mallicious guest could rewrite the header of the
s/mallicious/malicious/
+disk leading to access of host files. Libvirt thus does not try to assume +the format of the backing image. The following command can be used to check if +the image has backing image format specified:
s/has/has a/
+ +:: + + $ qemu-img info /tmp/copy4.qcow2 + image: /tmp/copy4.qcow2 + file format: qcow2 + virtual size: 10 MiB (10485760 bytes) + disk size: 196 KiB + cluster_size: 65536 + backing file: copy3.qcow2 (actual path: /tmp/copy3.qcow2) + backing file format: qcow2 + Format specific information: + compat: 1.1 + lazy refcounts: false + refcount bits: 16 + corrupt: false + +If the ``backing file format:`` field is missing above the format was not +specified properly. The image can be fixed by the following command: + +:: + + qemu-img rebase -f $IMAGE_FORMAT -F $BACKING_IMAGE_FORMAT -b $BACKING_IMAGE_PATH $IMAGE_PATH
Adding -u can make this operation faster (blindly update the image metadata, rather than actually crawling the entire image to perform a data operation in the process). But I'm not sure whether we want to document that here.
+ +It is important to fill out ``$BACKING_IMAGE_FORMAT`` and ``$IMAGE_FORMAT`` +properly. ``$BACKING_IMAGE_PATH`` should be specified as a full absolute path. +If relative referencing of the backing image is desired, the path must be +relative to the location of image described by ``$IMAGE_PATH``. + +Missing images reported after after moving disk images into a different path +---------------------------------------------------------------------------- + +The path to the backing image which is recorded in the image metadata often +contains a full path to the backing image. This is the default libvirt-created +image configuration. When such images are moved to a different location the +top image will no longer point to the correct image. + +To fix such issue you can either fully specify the image chain in the domain XML +as pointed out above or the following ``qemu-img`` command can be used: + +:: + + qemu-img rebase -u -f $IMAGE_FORMAT -F $BACKING_IMAGE_FORMAT -b $BACKING_IMAGE_PATH $IMAGE_PATH +
Odd to mention -u here but not above. Should we be consistent between the two examples?
+It is important to fill out ``$BACKING_IMAGE_FORMAT`` and ``$IMAGE_FORMAT`` +properly. ``$BACKING_IMAGE_PATH`` should be specified as a full absolute path. +If relative referencing of the backing image is desired, the path must be +relative to the location of image described by ``$IMAGE_PATH``.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Dec 17, 2019 at 13:17:22 -0600, Eric Blake wrote:
On 12/17/19 12:35 PM, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
[...]
+If the ``backing file format:`` field is missing above the format was not +specified properly. The image can be fixed by the following command: + +:: + + qemu-img rebase -f $IMAGE_FORMAT -F $BACKING_IMAGE_FORMAT -b $BACKING_IMAGE_PATH $IMAGE_PATH
Adding -u can make this operation faster (blindly update the image metadata, rather than actually crawling the entire image to perform a data operation in the process). But I'm not sure whether we want to document that here.
I thought qemu would skip the data verification if the backing image is not being changed. I'm not sure about the -u here though.
+It is important to fill out ``$BACKING_IMAGE_FORMAT`` and ``$IMAGE_FORMAT`` +properly. ``$BACKING_IMAGE_PATH`` should be specified as a full absolute path. +If relative referencing of the backing image is desired, the path must be +relative to the location of image described by ``$IMAGE_PATH``. + +Missing images reported after after moving disk images into a different path +---------------------------------------------------------------------------- + +The path to the backing image which is recorded in the image metadata often +contains a full path to the backing image. This is the default libvirt-created +image configuration. When such images are moved to a different location the +top image will no longer point to the correct image. + +To fix such issue you can either fully specify the image chain in the domain XML +as pointed out above or the following ``qemu-img`` command can be used: + +:: + + qemu-img rebase -u -f $IMAGE_FORMAT -F $BACKING_IMAGE_FORMAT -b $BACKING_IMAGE_PATH $IMAGE_PATH +
Odd to mention -u here but not above. Should we be consistent between the two examples?
You must use it here since qemu-img will fail to locate the old backing file as the example is fixing a image chain using absolute paths moved to a different location.
On 12/17/19 7:35 PM, Peter Krempa wrote:
See patch 3/4 for explanation.
Peter Krempa (4): tests: storage: Use strict version of virStorageFileGetMetadata tests: storage: Remove unused test modes util: storage: Don't treat files with missing backing store format as 'raw' kbase: Add document outlining backing chain XML config and troubleshooting
docs/kbase.html.in | 4 + docs/kbase/backing_chains.rst | 185 ++++++++++++++++++++++++++++++++++ src/util/virstoragefile.c | 21 +++- tests/virstoragetest.c | 42 +++----- 4 files changed, 221 insertions(+), 31 deletions(-) create mode 100644 docs/kbase/backing_chains.rst
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Eric Blake -
Michal Prívozník -
Peter Krempa