[libvirt] [PATCH v3 0/3] qemu: Forbid internal snapshots with pflash-based firmware (OVMF)
Patches 1 and 2 are new in the series and add an option to override the check so that users can use the feature despite being unsafe. See patch 3/3 for clarification of the motiovation to do this. Peter Krempa (3): virerror: Introduce VIR_ERR_OPERATION_UNSAFE snapshot: Introduce flag VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE qemu: snapshot: Forbid internal snapshots with pflash firmware include/libvirt/libvirt-domain-snapshot.h | 3 +++ include/libvirt/virterror.h | 1 + src/qemu/qemu_driver.c | 18 +++++++++++++++++- src/util/virerror.c | 5 +++++ tools/virsh-snapshot.c | 6 ++++++ tools/virsh.pod | 6 +++++- 6 files changed, 37 insertions(+), 2 deletions(-) -- 2.12.1
Similarly to VIR_ERR_MIGRATION_UNSAFE add a error code which will be reported when an operation is possible with the hypervisor but may lead to data loss or other problems in certain cases. This error code notifies the user that the operation can be forced using a specific flag. --- include/libvirt/virterror.h | 1 + src/util/virerror.c | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 2efee8f0c..04fea2e34 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -319,6 +319,7 @@ typedef enum { VIR_ERR_AGENT_UNSYNCED = 97, /* guest agent replies with wrong id to guest-sync command */ VIR_ERR_LIBSSH = 98, /* error in libssh transport driver */ + VIR_ERR_OPERATION_UNSAFE = 99, /* unsafe operation requiring override */ } virErrorNumber; /** diff --git a/src/util/virerror.c b/src/util/virerror.c index ef17fb5e6..1437cf02b 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1407,6 +1407,11 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("libssh transport error: %s"); break; + case VIR_ERR_OPERATION_UNSAFE: + if (info == NULL) + errmsg = _("operation unsafe"); + else + errmsg = _("operation unsafe: %s"); } return errmsg; } -- 2.12.1
Introduce a flag that will allow to override safety checks in certain snapshot configurations. --- include/libvirt/libvirt-domain-snapshot.h | 3 +++ src/qemu/qemu_driver.c | 3 ++- tools/virsh-snapshot.c | 6 ++++++ tools/virsh.pod | 6 +++++- 4 files changed, 16 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h index 0f73f24b2..9d1da8710 100644 --- a/include/libvirt/libvirt-domain-snapshot.h +++ b/include/libvirt/libvirt-domain-snapshot.h @@ -70,6 +70,9 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_CREATE_LIVE = (1 << 8), /* create the snapshot while the guest is running */ + VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE = (1 << 9), /* override safety checks + for certain + configurations */ } virDomainSnapshotCreateFlags; /* Take a snapshot of the current VM state */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 676295208..02cdd2f6b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14431,7 +14431,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE | + VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE, NULL); VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 5c844a5ea..48ad6f2ef 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -364,6 +364,10 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { .flags = VSH_OFLAG_REQ_OPT, .help = N_("memory attributes: [file=]name[,snapshot=type]") }, + {.name = "unsafe", + .type = VSH_OT_BOOL, + .help = N_("override unsafe operation") + }, {.name = "diskspec", .type = VSH_OT_ARGV, .help = N_("disk attributes: disk[,snapshot=type][,driver=type][,file=name]") @@ -404,6 +408,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; if (vshCommandOptBool(cmd, "live")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE; + if (vshCommandOptBool(cmd, "unsafe")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; diff --git a/tools/virsh.pod b/tools/virsh.pod index ee7904611..62bca4b45 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -4104,7 +4104,7 @@ used to represent properties of snapshots. =item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]] | [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>] -[I<--quiesce>] [I<--atomic>] [I<--live>]} +[I<--quiesce>] [I<--atomic>] [I<--live>] [I<--unsafe>]} Create a snapshot for domain I<domain> with the properties specified in I<xmlfile>. Normally, the only properties settable for a domain snapshot @@ -4163,6 +4163,10 @@ the guest is running. Both disk snapshot and domain memory snapshot are taken. This increases the size of the memory image of the external checkpoint. This is currently supported only for external checkpoints. +Certain snapshot configurations may be unsafe but historically used to work. You +can specify the I<--unsafe> flag if a snapshot operation is forbidden due to +being unsafe. + Existence of snapshot metadata will prevent attempts to B<undefine> a persistent domain. However, for transient domains, snapshot metadata is silently lost when the domain quits running (whether -- 2.12.1
If the variable store (<nvram>) file is raw qemu can't do a snapshot of it and thus the snapshot would be incomplete. QEMU does no reject such snapshot. Additionally allowing to use a qcow2 variable store backing file would solve this issue but then it would become eligible to become target of the memory dump. Offline internal snapshot would be incomplete too with either storage format since libvirt does not handle the pflash file in this case. Forbid such snapshot so that we can avoid problems. --- Notes: v3: - allow overriding of the check by specifying VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE - report VIR_ERR_OPERATION_UNSAFE (instead of VIR_ERR_OPERATION_UNSUPPORTED) - tweaked commend in code (since it's not forbidden completely) - tweaked error message v2: - changed error code to OPERATION_UNSUPPORTED (from CONFIG_UNSUPPORTED) - dropped mention of QEMU from the error message - dropped mentions of OVMF or the firmware itself altoghether, the culprit is the pflash device regardless of the software it contains - mentioned all the stuff in the commit message and comment We also will need to introduce a way to snapshot the pflash for external snapshots which is currently impossible as well, but fortunately does not have inherent drawbacks as internal snapshots. src/qemu/qemu_driver.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 02cdd2f6b..2ca839f1c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13754,6 +13754,7 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, bool active = virDomainObjIsActive(vm); bool reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; bool atomic = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; + bool unsafe = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE) != 0; bool found_internal = false; bool forbid_internal = false; int external = 0; @@ -13873,6 +13874,20 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, goto cleanup; } + /* internal snapshots + pflash based loader have the following problems: + * - if the variable store is raw, the snapshot is incomplete + * - alowing a qcow2 image as the varstore would make it eligible to receive + * the vmstate dump, which would make it huge + * - offline snapshot would not snapshot the varstore at all + */ + if (!unsafe && found_internal && + vm->def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH) { + virReportError(VIR_ERR_OPERATION_UNSAFE, "%s", + _("internal snapshots of a VM with pflash based " + "firmware can corrupt the nvram data")); + goto cleanup; + } + /* Alter flags to let later users know what we learned. */ if (external && !active) *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; -- 2.12.1
On Fri, Mar 24, 2017 at 14:05:39 +0100, Peter Krempa wrote:
Patches 1 and 2 are new in the series and add an option to override the check so that users can use the feature despite being unsafe.
See patch 3/3 for clarification of the motiovation to do this.
Peter Krempa (3): virerror: Introduce VIR_ERR_OPERATION_UNSAFE snapshot: Introduce flag VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE qemu: snapshot: Forbid internal snapshots with pflash firmware
Self NACK series. Turns out that our HMP error detection for 'savevm' sucks. Snapshots with PFLASH are not supported at all, but libvirt thinks they are. V2 is the way to go with mentioning that actually it's a libvirt bug.
participants (1)
-
Peter Krempa