[PATCH v2 0/5] Fix crash when reverting/deleting external snapshots
v2: - modified explanation of why virStorageSourceIsSameLocation was called with NULL - fixed the call to virStorageSourceGetMetadata which caused the NULL deref - fixed permission issue when calling qemu-img if files are owned by root:root after restoring seclabels - improved error message when generating snapshot path fails (not related to original bug, just annoying when I've had a messed up VM) Peter Krempa (5): qemuSnapshotDiskHasBackingDisk: Avoid call of virStorageSourceIsSameLocation with NULL argument qemuSnapshotUpdateBackingStore: Remove stale comment qemuSnapshotDiskHasBackingDisk: Use proper 'max_depth' when calling 'virStorageSourceGetMetadata' virDomainSnapshotDefAssignExternalNames: Improve error message qemuSnapshotUpdateBackingStore: Retry as curent user if qemu-img fails src/conf/snapshot_conf.c | 4 +-- src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_snapshot.c | 61 ++++++++++++++++++++++++++++------------ 4 files changed, 46 insertions(+), 22 deletions(-) -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> When the 'backingStore' pointer is not populated the function calls 'virStorageSourceGetMetadata' to try to populate it but if the on-disk metadata doesn't have a backing image (e.g. if it's the 'base' image of the chain) the 'backingStore' or the metadata fetcher fails the pointer will still be NULL. The function then calls 'virStorageSourceIsSameLocation' but the internal functions for dealing with storage sources don't handle NULL gracefully. Since the code calling 'qemu-img' based on the data detected here doesn't actually raise errors if the operations fail there's no point in raising errors here either. Closes: https://gitlab.com/libvirt/libvirt/-/issues/844 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index e738afffc3..8f58df3b45 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3147,7 +3147,8 @@ qemuSnapshotDiskHasBackingDisk(void *payload, if (!disk->src->backingStore) ignore_value(virStorageSourceGetMetadata(disk->src, uid, gid, 1, false)); - if (virStorageSourceIsSameLocation(disk->src->backingStore, iterdata->diskSrc)) { + if (disk->src->backingStore && + virStorageSourceIsSameLocation(disk->src->backingStore, iterdata->diskSrc)) { struct _qemuSnapshotDisksWithBackingStoreData *data = g_new0(struct _qemuSnapshotDisksWithBackingStoreData, 1); -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> The code does a 'qemu-img rebase' rather than a 'qemu-img create' what the commit suggests. Since we enumerate all arguments right below, there's no need for a comment. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 8f58df3b45..19bb6f8b37 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3710,7 +3710,6 @@ qemuSnapshotUpdateBackingStore(qemuSnapshotDeleteExternalData *data) struct _qemuSnapshotDisksWithBackingStoreData *backingData = cur->data; g_autoptr(virCommand) cmd = NULL; - /* creates cmd line args: qemu-img create -f qcow2 -o */ if (!(cmd = virCommandNewArgList("qemu-img", "rebase", "-u", -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> The 'max_depth' argument of 'virStorageSourceGetMetadata' doesn't just limit how far the function goes but also fails completely if the chain is deeper than the passed value. In 'qemuSnapshotDiskHasBackingDisk' we only care about finding the backing image, so just one level below, the passed path, but due to the above setting '1' as max_depth will make the function simply fail every time. Extract and reuse QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH as the detection depth. While '200' layers is overkill for this code, we also start a full qemu instance just to delete an snapshot so this doens't matter and still protects from self-referential images. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_snapshot.c | 4 +++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ac56fc7cb4..486a0e7913 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6297,8 +6297,6 @@ qemuDomainStorageAlias(const char *device, int depth) } -#define QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH 200 - /** * qemuDomainStorageSourceValidateDepth: * @src: storage source chain to validate diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 3396f929fd..b9bb338682 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -706,6 +706,7 @@ int qemuDomainCheckDiskStartupPolicy(virQEMUDriver *driver, size_t diskIndex, bool cold_boot); +#define QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH 200 int qemuDomainStorageSourceValidateDepth(virStorageSource *src, int add, const char *diskdst); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 19bb6f8b37..1628c33865 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3145,7 +3145,9 @@ qemuSnapshotDiskHasBackingDisk(void *payload, NULL, &uid, &gid); if (!disk->src->backingStore) - ignore_value(virStorageSourceGetMetadata(disk->src, uid, gid, 1, false)); + ignore_value(virStorageSourceGetMetadata(disk->src, uid, gid, + QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH, + false)); if (disk->src->backingStore && virStorageSourceIsSameLocation(disk->src->backingStore, iterdata->diskSrc)) { -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> Mention the 'path' where the detection failed as well as include the possibility that the 'path' doesn't exist in the message itself. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 039ed77b84..4309667a34 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -541,8 +541,8 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def, if (stat(origpath, &sb) < 0 || !S_ISREG(sb.st_mode)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("source for disk '%1$s' is not a regular file; refusing to generate external snapshot name"), - disk->name); + _("source for disk '%1$s' (%2$s) doesn't exist or is not a regular file; refusing to generate external snapshot name"), + disk->name, origpath); return -1; } -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> The code calls 'qemu-img rebase' to fix the backing store references. The 'qemu-img' process here is run as the 'qemu' user or whatever the defaults and domain XML resolve to. Since this, in certain cases, works also on images which are not part of the backing chain and in privileged deployments thus can be owned by 'root:root' the update may fail (silently). To preserver root-squash deployments but fix also the above case, retry the operation on failure as current user. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 53 ++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1628c33865..70e4c37144 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3710,25 +3710,48 @@ qemuSnapshotUpdateBackingStore(qemuSnapshotDeleteExternalData *data) for (cur = data->disksWithBacking; cur; cur = g_slist_next(cur)) { struct _qemuSnapshotDisksWithBackingStoreData *backingData = cur->data; - g_autoptr(virCommand) cmd = NULL; + /* Try to run the command first as the appropriate user based on the + * domain definition and config. If error is returned retry as current + * (possibly privileged) user for cases where seclabels were reset + * to the default */ + g_autoptr(virCommand) cmd_user_qemu = NULL; + g_autoptr(virCommand) cmd_user_curr = NULL; + + if (!(cmd_user_qemu = virCommandNewArgList("qemu-img", + "rebase", + "-u", + "-F", + virStorageFileFormatTypeToString(data->parentDiskSrc->format), + "-f", + virStorageFileFormatTypeToString(backingData->diskSrc->format), + "-b", + data->parentDiskSrc->path, + backingData->diskSrc->path, + NULL))) + continue; - if (!(cmd = virCommandNewArgList("qemu-img", - "rebase", - "-u", - "-F", - virStorageFileFormatTypeToString(data->parentDiskSrc->format), - "-f", - virStorageFileFormatTypeToString(backingData->diskSrc->format), - "-b", - data->parentDiskSrc->path, - backingData->diskSrc->path, - NULL))) + virCommandSetUID(cmd_user_qemu, backingData->uid); + virCommandSetGID(cmd_user_qemu, backingData->gid); + + /* done on success */ + if (virCommandRun(cmd_user_qemu, NULL) == 0) continue; - virCommandSetUID(cmd, backingData->uid); - virCommandSetGID(cmd, backingData->gid); + /* retry as current user */ + if (!(cmd_user_curr = virCommandNewArgList("qemu-img", + "rebase", + "-u", + "-F", + virStorageFileFormatTypeToString(data->parentDiskSrc->format), + "-f", + virStorageFileFormatTypeToString(backingData->diskSrc->format), + "-b", + data->parentDiskSrc->path, + backingData->diskSrc->path, + NULL))) + continue; - ignore_value(virCommandRun(cmd, NULL)); + ignore_value(virCommandRun(cmd_user_curr, NULL)); } } -- 2.52.0
On Mon, Jan 26, 2026 at 05:06:48PM +0100, Peter Krempa via Devel wrote:
v2: - modified explanation of why virStorageSourceIsSameLocation was called with NULL - fixed the call to virStorageSourceGetMetadata which caused the NULL deref - fixed permission issue when calling qemu-img if files are owned by root:root after restoring seclabels - improved error message when generating snapshot path fails (not related to original bug, just annoying when I've had a messed up VM)
Peter Krempa (5): qemuSnapshotDiskHasBackingDisk: Avoid call of virStorageSourceIsSameLocation with NULL argument qemuSnapshotUpdateBackingStore: Remove stale comment qemuSnapshotDiskHasBackingDisk: Use proper 'max_depth' when calling 'virStorageSourceGetMetadata' virDomainSnapshotDefAssignExternalNames: Improve error message qemuSnapshotUpdateBackingStore: Retry as curent user if qemu-img fails
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Pavel Hrdina -
Peter Krempa