[PATCH 0/7] qemu: tpm: Add support for migration across shared storage
This series of patches adds support for migrating vTPMs across hosts whose storage has been set up to share the directory structure holding the state of the TPM (swtpm). The domain XML is extended with a shared_storage attribute that must be set to 'yes' when shared storage is used. It influences the management of the directory structure holding the TPM state, which for example is only to be removed when a domain is undefined (virsh undefine) and not when a VM is removed on the migration source host. Further, when shared storage is used security labeling on the destination side is skipped assuming that the labeling was already done on the source side. I have tested this with an NFS setup where I had to turn SELinux off on the hosts since the SELinux MLS range labeling is not supported. Share storage migration requires the upcoming swtpm v0.8 with the PR for shared storage merged: https://github.com/stefanberger/swtpm/pull/732 Stefan Stefan Berger (7): qemu: tpm: Pass parameter indicating reason for domain removal util: Add parsing support for swtpm's cmdarg-migration capability qemu: tpm: Conditionally create storage on incoming migration qemu: tpm: Pass --migration option to swtpm when using shared storage qemu: tpm: Avoid security labels on incoming migration with shared storage qemu: tpm: Remove TPM state files and directory only when undefining a VM qemu: config: Extend TPM domain XML with shared storage support docs/formatdomain.rst | 16 ++++++++ src/conf/domain_conf.c | 13 +++++++ src/conf/domain_conf.h | 1 + src/conf/schemas/domaincommon.rng | 5 +++ src/qemu/qemu_domain.c | 12 +++--- src/qemu/qemu_domain.h | 8 +++- src/qemu/qemu_driver.c | 20 +++++----- src/qemu/qemu_extdevice.c | 5 ++- src/qemu/qemu_extdevice.h | 3 +- src/qemu/qemu_migration.c | 13 ++++--- src/qemu/qemu_process.c | 4 +- src/qemu/qemu_snapshot.c | 4 +- src/qemu/qemu_tpm.c | 61 ++++++++++++++++++++++++++----- src/qemu/qemu_tpm.h | 3 +- src/util/virtpm.c | 1 + src/util/virtpm.h | 1 + 16 files changed, 131 insertions(+), 39 deletions(-) -- 2.37.1
Pass a parameter indicating the reason for the removal of a domain so that the TPM driver can determine whether to delete the TPM state directory structure. It may only do this when a domain is undefined as part of a command like 'virsh undefine' and it must not do this when a VM is migrated across a shared storage setup for the TPM and it is removed from the source host. Therefore, the call locations that correspond to a 'virsh undefine' pass the value 'QEMU_DOMAIN_UNDEFINE_DOMAIN' while all other ones pass the unspecific value 'QEMU_DOMAIN_UNDEFINE_UNSPEC'. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_domain.c | 12 +++++++----- src/qemu/qemu_domain.h | 8 +++++++- src/qemu/qemu_driver.c | 20 ++++++++++---------- src/qemu/qemu_extdevice.c | 5 +++-- src/qemu/qemu_extdevice.h | 3 ++- src/qemu/qemu_migration.c | 13 +++++++------ src/qemu/qemu_process.c | 4 ++-- src/qemu/qemu_snapshot.c | 4 ++-- src/qemu/qemu_tpm.c | 9 ++++++--- src/qemu/qemu_tpm.h | 3 ++- 10 files changed, 48 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 45f00e162d..ac8ab142b5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7143,7 +7143,8 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, static void qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, - virDomainObj *vm) + virDomainObj *vm, + qemuDomainUndefineReason undefReason) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *snapDir = NULL; @@ -7169,7 +7170,7 @@ qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, if (rmdir(chkDir) < 0 && errno != ENOENT) VIR_WARN("unable to remove checkpoint directory %s", chkDir); } - qemuExtDevicesCleanupHost(driver, vm->def); + qemuExtDevicesCleanupHost(driver, vm->def, undefReason); } @@ -7180,14 +7181,15 @@ qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, */ void qemuDomainRemoveInactive(virQEMUDriver *driver, - virDomainObj *vm) + virDomainObj *vm, + qemuDomainUndefineReason undefReason) { if (vm->persistent) { /* Short-circuit, we don't want to remove a persistent domain */ return; } - qemuDomainRemoveInactiveCommon(driver, vm); + qemuDomainRemoveInactiveCommon(driver, vm, undefReason); virDomainObjListRemove(driver->domains, vm); } @@ -7209,7 +7211,7 @@ qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, return; } - qemuDomainRemoveInactiveCommon(driver, vm); + qemuDomainRemoveInactiveCommon(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); virDomainObjListRemoveLocked(driver->domains, vm); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 592ee9805b..8e5dacf624 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -682,8 +682,14 @@ int qemuDomainMomentDiscardAll(void *payload, int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, virDomainObj *vm); +typedef enum { + QEMU_DOMAIN_UNDEFINE_UNSPEC = 0, + QEMU_DOMAIN_UNDEFINE_DOMAIN, /* virsh undefine type of command */ +} qemuDomainUndefineReason; + void qemuDomainRemoveInactive(virQEMUDriver *driver, - virDomainObj *vm); + virDomainObj *vm, + qemuDomainUndefineReason undefReason); void qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 637a748c85..fe5bbe2216 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1624,7 +1624,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, goto cleanup; if (qemuProcessBeginJob(vm, VIR_DOMAIN_JOB_OPERATION_START, flags) < 0) { - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); goto cleanup; } @@ -1633,7 +1633,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags) < 0) { virDomainAuditStart(vm, "booted", false); - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); qemuProcessEndJob(vm); goto cleanup; } @@ -2119,7 +2119,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, ret = 0; endjob: if (ret == 0) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_DOMAIN); qemuDomainObjEndJob(vm); cleanup: @@ -2738,7 +2738,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver, } qemuDomainObjEndAsyncJob(vm); if (ret == 0) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); cleanup: virQEMUSaveDataFree(data); @@ -3285,7 +3285,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, qemuDomainObjEndAsyncJob(vm); if (ret == 0 && flags & VIR_DUMP_CRASH) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); cleanup: virDomainObjEndAPI(&vm); @@ -3597,7 +3597,7 @@ processGuestPanicEvent(virQEMUDriver *driver, endjob: qemuDomainObjEndAsyncJob(vm); if (removeInactive) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); } @@ -4075,7 +4075,7 @@ processMonitorEOFEvent(virQEMUDriver *driver, virObjectEventStateQueue(driver->domainEventState, event); endjob: - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); qemuDomainObjEndJob(vm); } @@ -6005,7 +6005,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, virFileWrapperFdFree(wrapperFd); virQEMUSaveDataFree(data); if (vm && ret < 0) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); virDomainObjEndAPI(&vm); return ret; } @@ -6696,7 +6696,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, } else { /* Brand new domain. Remove it */ VIR_INFO("Deleting domain '%s'", vm->def->name); - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); } } @@ -6836,7 +6836,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, */ vm->persistent = 0; if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_DOMAIN); ret = 0; endjob: diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index b8e3c1000a..b5bfd6cfd2 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -151,7 +151,8 @@ qemuExtDevicesPrepareHost(virQEMUDriver *driver, void qemuExtDevicesCleanupHost(virQEMUDriver *driver, - virDomainDef *def) + virDomainDef *def, + qemuDomainUndefineReason undefReason) { size_t i; @@ -159,7 +160,7 @@ qemuExtDevicesCleanupHost(virQEMUDriver *driver, return; for (i = 0; i < def->ntpms; i++) { - qemuExtTPMCleanupHost(def->tpms[i]); + qemuExtTPMCleanupHost(def->tpms[i], undefReason); } } diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h index 43d2a4dfff..252238c47f 100644 --- a/src/qemu/qemu_extdevice.h +++ b/src/qemu/qemu_extdevice.h @@ -41,7 +41,8 @@ int qemuExtDevicesPrepareHost(virQEMUDriver *driver, G_GNUC_WARN_UNUSED_RESULT; void qemuExtDevicesCleanupHost(virQEMUDriver *driver, - virDomainDef *def) + virDomainDef *def, + qemuDomainUndefineReason undefReason) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuExtDevicesStart(virQEMUDriver *driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b3b25d78b4..b15c1ccc22 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3408,7 +3408,7 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, * and there is no 'goto cleanup;' in the middle of those */ VIR_FREE(priv->origname); virDomainObjRemoveTransientDef(vm); - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); } virDomainObjEndAPI(&vm); virErrorRestore(&origErr); @@ -4054,7 +4054,8 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver, virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); vm->persistent = 0; } - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, + QEMU_DOMAIN_UNDEFINE_UNSPEC); } cleanup: @@ -6003,7 +6004,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); vm->persistent = 0; } - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); } virErrorRestore(&orig_err); @@ -6130,7 +6131,7 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); return ret; } @@ -6667,7 +6668,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); virErrorRestore(&orig_err); return NULL; @@ -6805,7 +6806,7 @@ qemuMigrationProcessUnattended(virQEMUDriver *driver, qemuMigrationJobFinish(vm); if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5c8413a6b6..cd038195d3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8460,7 +8460,7 @@ qemuProcessAutoDestroy(virDomainObj *dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - qemuDomainRemoveInactive(driver, dom); + qemuDomainRemoveInactive(driver, dom, QEMU_DOMAIN_UNDEFINE_UNSPEC); qemuDomainObjEndJob(dom); @@ -8926,7 +8926,7 @@ qemuProcessReconnect(void *opaque) if (jobStarted) qemuDomainObjEndJob(obj); if (!virDomainObjIsActive(obj)) - qemuDomainRemoveInactive(driver, obj); + qemuDomainRemoveInactive(driver, obj, QEMU_DOMAIN_UNDEFINE_UNSPEC); virDomainObjEndAPI(&obj); virIdentitySetCurrent(NULL); return; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 6033deafed..2a4d2a9d9e 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2103,7 +2103,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, } if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); return -1; } @@ -2125,7 +2125,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) { - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); return -1; } detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 584c787b70..d2ae3b9824 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -701,11 +701,13 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm, /** * qemuTPMEmulatorCleanupHost: * @tpm: TPM definition + * @undefReason: The reason why the domain is removed * * Clean up persistent storage for the swtpm. */ static void -qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm) +qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, + qemuDomainUndefineReason undefReason G_GNUC_UNUSED) { if (!tpm->data.emulator.persistent_state) qemuTPMEmulatorDeleteStorage(tpm); @@ -1003,9 +1005,10 @@ qemuExtTPMPrepareHost(virQEMUDriver *driver, void -qemuExtTPMCleanupHost(virDomainTPMDef *tpm) +qemuExtTPMCleanupHost(virDomainTPMDef *tpm, + qemuDomainUndefineReason undefReason) { - qemuTPMEmulatorCleanupHost(tpm); + qemuTPMEmulatorCleanupHost(tpm, undefReason); } diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h index 9951f025a6..37c8e080d7 100644 --- a/src/qemu/qemu_tpm.h +++ b/src/qemu/qemu_tpm.h @@ -35,7 +35,8 @@ int qemuExtTPMPrepareHost(virQEMUDriver *driver, ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; -void qemuExtTPMCleanupHost(virDomainTPMDef *tpm) +void qemuExtTPMCleanupHost(virDomainTPMDef *tpm, + qemuDomainUndefineReason undefReason) ATTRIBUTE_NONNULL(1); int qemuExtTPMStart(virQEMUDriver *driver, -- 2.37.1
Add support for parsing swtpm 'cmdarg-migration' capability (since v0.8). Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/util/virtpm.c | 1 + src/util/virtpm.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/util/virtpm.c b/src/util/virtpm.c index 2f2b061fee..d85ab2bb97 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -39,6 +39,7 @@ VIR_LOG_INIT("util.tpm"); VIR_ENUM_IMPL(virTPMSwtpmFeature, VIR_TPM_SWTPM_FEATURE_LAST, "cmdarg-pwd-fd", + "cmdarg-migration", ); VIR_ENUM_IMPL(virTPMSwtpmSetupFeature, diff --git a/src/util/virtpm.h b/src/util/virtpm.h index a873881b23..fb330effa8 100644 --- a/src/util/virtpm.h +++ b/src/util/virtpm.h @@ -30,6 +30,7 @@ bool virTPMHasSwtpm(void); typedef enum { VIR_TPM_SWTPM_FEATURE_CMDARG_PWD_FD, + VIR_TPM_SWTPM_FEATURE_CMDARG_MIGRATION, VIR_TPM_SWTPM_FEATURE_LAST } virTPMSwtpmFeature; -- 2.37.1
Add a shared_storage field to the emulator part of the virDomainTPMDef used for indicating whether shared storage for TPM state is setup between hosts. Do not create storage if shared_storage flag is set and there's an incoming migration since the storage directory in this case must already exist. As a consequence also do not run swtpm_setup in this case. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/conf/domain_conf.h | 1 + src/qemu/qemu_tpm.c | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a1f6cf7a6f..29dc17a299 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1459,6 +1459,7 @@ struct _virDomainTPMDef { unsigned char secretuuid[VIR_UUID_BUFLEN]; bool hassecretuuid; bool persistent_state; + bool shared_storage; virBitmap *activePcrBanks; } emulator; } data; diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index d2ae3b9824..280307a14e 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -562,11 +562,19 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, int pwdfile_fd = -1; int migpwdfile_fd = -1; const unsigned char *secretuuid = NULL; + bool create_storage = true; if (!swtpm) return NULL; - if (qemuTPMEmulatorCreateStorage(tpm, &created, swtpm_user, swtpm_group) < 0) + /* Do not create storage and run swtpm_setup on incoming migration over + * shared storage + */ + if (incomingMigration && tpm->data.emulator.shared_storage) + create_storage = false; + + if (create_storage && + qemuTPMEmulatorCreateStorage(tpm, &created, swtpm_user, swtpm_group) < 0) return NULL; if (tpm->data.emulator.hassecretuuid) -- 2.37.1
when using shared storage pass the --migration option to swtpm, if swptm supports it (staring with v0.8). Always apply the 'release-lock-outgoing' parameter with this option and apply the 'incoming' parameter for incoming migration so that swtpm releases the file lock on the source side when the state is migrated and locks the file on the destination side when the state is received. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_tpm.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 280307a14e..8b3ef4e34e 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -650,6 +650,19 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc", migpwdfile_fd); } + if (tpm->data.emulator.shared_storage) { + if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_MIGRATION)) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("%s does not support the --migration option needed for shared storage"), + swtpm); + goto error; + } + + virCommandAddArg(cmd, "--migration"); + virCommandAddArgFormat(cmd, "release-lock-outgoing%s", + incomingMigration ? ",incoming": ""); + } + return g_steal_pointer(&cmd); error: -- 2.37.1
When using shared storage there is no need to apply security labels on the storage since the files have to have been labeled already on the source side and we must assume that the source and destination side have been setup to use the same uid and gid for running swtpm as well as share the same security labels. whether the security labels can be used at all depends on the shared storage and whether and how it supports them. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_tpm.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 8b3ef4e34e..20c7e92766 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -924,10 +924,19 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virCommandSetPidFile(cmd, pidfile); virCommandSetErrorFD(cmd, &errfd); - if (qemuSecurityStartTPMEmulator(driver, vm, cmd, - cfg->swtpm_user, cfg->swtpm_group, - NULL, &cmdret) < 0) - return -1; + if (incomingMigration && + tpm->data.emulator.shared_storage) { + /* security labels must have been set up on source already */ + if (qemuSecurityCommandRun(driver, vm, cmd, + cfg->swtpm_user, cfg->swtpm_group, + NULL, &cmdret) < 0) { + goto error; + } + } else if (qemuSecurityStartTPMEmulator(driver, vm, cmd, + cfg->swtpm_user, cfg->swtpm_group, + NULL, &cmdret) < 0) { + goto error; + } if (cmdret < 0) { /* virCommandRun() hidden in qemuSecurityStartTPMEmulator() -- 2.37.1
When share storage for the TPM state files has been setup betwen hosts then remove the TPM state files and directory only when undefining a VM and only if the attribute persistent_state is not set. Avoid removing the TPM state files and directory structure when a VM is migrated and shared storage is used since this would also remove those files and directory structure on the destination side. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_tpm.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 20c7e92766..d1639318e7 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -728,10 +728,20 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm, */ static void qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, - qemuDomainUndefineReason undefReason G_GNUC_UNUSED) + qemuDomainUndefineReason undefReason) { - if (!tpm->data.emulator.persistent_state) + if (tpm->data.emulator.shared_storage) { + /* When using shared storage remove the domain only if this is due to + * a 'virsh undefine' type of command and only if persistent_state == + * false. Avoid removal of the state files/directory during migration. + */ + if (undefReason == QEMU_DOMAIN_UNDEFINE_DOMAIN && + !tpm->data.emulator.persistent_state) { + qemuTPMEmulatorDeleteStorage(tpm); + } + } else if (!tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); + } } -- 2.37.1
On Mon, Aug 22, 2022 at 08:05:53AM -0400, Stefan Berger wrote:
When share storage for the TPM state files has been setup betwen hosts then remove the TPM state files and directory only when undefining a VM and only if the attribute persistent_state is not set. Avoid removing the TPM state files and directory structure when a VM is migrated and shared storage is used since this would also remove those files and directory structure on the destination side.
I think our current undefine behaviour is probably flawed. We go to the trouble of refusing to remove the firmware NVRAM when undefining because it contains important VM state, but then happily blow away the TPM state. Totally inconsistent behaviour :-( Its too late to change the default behaviour, but we likely ought to add a flag VIR_DOMAIN_UNDEFINE_KEEP_TPM and plumb that through the varius code paths, which would remove the need for this specific 'qemuDomainUndefineReason' enum.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_tpm.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 20c7e92766..d1639318e7 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -728,10 +728,20 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm, */ static void qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, - qemuDomainUndefineReason undefReason G_GNUC_UNUSED) + qemuDomainUndefineReason undefReason) { - if (!tpm->data.emulator.persistent_state) + if (tpm->data.emulator.shared_storage) { + /* When using shared storage remove the domain only if this is due to + * a 'virsh undefine' type of command and only if persistent_state == + * false. Avoid removal of the state files/directory during migration. + */ + if (undefReason == QEMU_DOMAIN_UNDEFINE_DOMAIN && + !tpm->data.emulator.persistent_state) { + qemuTPMEmulatorDeleteStorage(tpm); + } + } else if (!tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); + } }
-- 2.37.1
With regards, Daniel -- |: 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 :|
On 8/22/22 12:46, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 08:05:53AM -0400, Stefan Berger wrote:
When share storage for the TPM state files has been setup betwen hosts then remove the TPM state files and directory only when undefining a VM and only if the attribute persistent_state is not set. Avoid removing the TPM state files and directory structure when a VM is migrated and shared storage is used since this would also remove those files and directory structure on the destination side.
I think our current undefine behaviour is probably flawed. We go to the trouble of refusing to remove the firmware NVRAM when undefining because it contains important VM state, but then happily blow away the TPM state. Totally inconsistent behaviour :-( Its too late to change the default behaviour, but we likely ought to add a flag
VIR_DOMAIN_UNDEFINE_KEEP_TPM
and plumb that through the varius code paths, which would remove the need for this specific 'qemuDomainUndefineReason' enum.
I think the granularity encoded in the reason is necessary for the following patch I was going to post later on: Subject: [PATCH] qemu: tpm: Remove TPM state with non-shared storage This patch 'fixes' the behavior of the persistent_state TPM domain XML attribute that intends to preserve the state of the TPM but should not keep the state around on all the hosts a VM has been migrated to. It removes the TPM state directory structure from the source host upon successful migration when non-shared storage is used. Similarly, it removes it from the destination host upon migration failure when non-shared storage is used. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_migration.c | 4 ++-- src/qemu/qemu_tpm.c | 3 +++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8e5dacf624..4cf6a640c2 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -685,6 +685,8 @@ int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, typedef enum { QEMU_DOMAIN_UNDEFINE_UNSPEC = 0, QEMU_DOMAIN_UNDEFINE_DOMAIN, /* virsh undefine type of command */ + QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC, /* undefine due to successful migration on src */ + QEMU_DOMAIN_UNDEFINE_MIGRATION_DST, /* undefine due to failed migration on dst */ } qemuDomainUndefineReason; void qemuDomainRemoveInactive(virQEMUDriver *driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b15c1ccc22..f50a488072 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4055,7 +4055,7 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver, vm->persistent = 0; } qemuDomainRemoveInactive(driver, vm, - QEMU_DOMAIN_UNDEFINE_UNSPEC); + QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC); } cleanup: @@ -6668,7 +6668,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_MIGRATION_DST); virErrorRestore(&orig_err); return NULL; diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index d1639318e7..ad9bc9f1a5 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -739,6 +739,9 @@ qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, !tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } + } else if (undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC || + undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_DST) { + qemuTPMEmulatorDeleteStorage(tpm); } else if (!tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } -- 2.37.1 The complete qemuTPMEmulatorCleanupHost handling shared and non-shared storage along with the persistent_state attribute now looks like this: static void qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, qemuDomainUndefineReason undefReason) { if (tpm->data.emulator.shared_storage) { /* When using shared storage remove the domain only if this is due to * a 'virsh undefine' type of command and only if persistent_state == * false. Avoid removal of the state files/directory during migration. */ if (undefReason == QEMU_DOMAIN_UNDEFINE_DOMAIN && !tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } } else if (undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC || undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_DST) { qemuTPMEmulatorDeleteStorage(tpm); } else if (!tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } } Regards, Stefan
On Mon, Aug 22, 2022 at 03:17:34PM -0400, Stefan Berger wrote:
On 8/22/22 12:46, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 08:05:53AM -0400, Stefan Berger wrote:
When share storage for the TPM state files has been setup betwen hosts then remove the TPM state files and directory only when undefining a VM and only if the attribute persistent_state is not set. Avoid removing the TPM state files and directory structure when a VM is migrated and shared storage is used since this would also remove those files and directory structure on the destination side.
I think our current undefine behaviour is probably flawed. We go to the trouble of refusing to remove the firmware NVRAM when undefining because it contains important VM state, but then happily blow away the TPM state. Totally inconsistent behaviour :-( Its too late to change the default behaviour, but we likely ought to add a flag
VIR_DOMAIN_UNDEFINE_KEEP_TPM
and plumb that through the varius code paths, which would remove the need for this specific 'qemuDomainUndefineReason' enum.
I think the granularity encoded in the reason is necessary for the following patch I was going to post later on:
Subject: [PATCH] qemu: tpm: Remove TPM state with non-shared storage
This patch 'fixes' the behavior of the persistent_state TPM domain XML attribute that intends to preserve the state of the TPM but should not keep the state around on all the hosts a VM has been migrated to. It removes the TPM state directory structure from the source host upon successful migration when non-shared storage is used. Similarly, it removes it from the destination host upon migration failure when non-shared storage is used.
'persistent_state' is something that applies to transient VMs. What I'm suggesting is that we should have some control over this for persistent VMs, when calling 'undefine'. ie virsh undefine --keep-nvram --keep-tpm $VMNAME if we have this feature in the public API, then its impl should support both this feature and your future patch for 'persistent_state'.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_migration.c | 4 ++-- src/qemu/qemu_tpm.c | 3 +++ 3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8e5dacf624..4cf6a640c2 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -685,6 +685,8 @@ int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, typedef enum { QEMU_DOMAIN_UNDEFINE_UNSPEC = 0, QEMU_DOMAIN_UNDEFINE_DOMAIN, /* virsh undefine type of command */ + QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC, /* undefine due to successful migration on src */ + QEMU_DOMAIN_UNDEFINE_MIGRATION_DST, /* undefine due to failed migration on dst */ } qemuDomainUndefineReason;
void qemuDomainRemoveInactive(virQEMUDriver *driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b15c1ccc22..f50a488072 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4055,7 +4055,7 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver, vm->persistent = 0; } qemuDomainRemoveInactive(driver, vm, - QEMU_DOMAIN_UNDEFINE_UNSPEC); + QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC); }
cleanup: @@ -6668,7 +6668,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, }
if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_MIGRATION_DST);
virErrorRestore(&orig_err); return NULL; diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index d1639318e7..ad9bc9f1a5 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -739,6 +739,9 @@ qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, !tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } + } else if (undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC || + undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_DST) { + qemuTPMEmulatorDeleteStorage(tpm); } else if (!tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } -- 2.37.1
The complete qemuTPMEmulatorCleanupHost handling shared and non-shared storage along with the persistent_state attribute now looks like this:
static void qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, qemuDomainUndefineReason undefReason) { if (tpm->data.emulator.shared_storage) { /* When using shared storage remove the domain only if this is due to * a 'virsh undefine' type of command and only if persistent_state == * false. Avoid removal of the state files/directory during migration. */ if (undefReason == QEMU_DOMAIN_UNDEFINE_DOMAIN && !tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } } else if (undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC || undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_DST) { qemuTPMEmulatorDeleteStorage(tpm); } else if (!tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } }
Regards, Stefan
With regards, Daniel -- |: 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 :|
On 8/23/22 08:52, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 03:17:34PM -0400, Stefan Berger wrote:
On 8/22/22 12:46, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 08:05:53AM -0400, Stefan Berger wrote:
When share storage for the TPM state files has been setup betwen hosts then remove the TPM state files and directory only when undefining a VM and only if the attribute persistent_state is not set. Avoid removing the TPM state files and directory structure when a VM is migrated and shared storage is used since this would also remove those files and directory structure on the destination side.
I think our current undefine behaviour is probably flawed. We go to the trouble of refusing to remove the firmware NVRAM when undefining because it contains important VM state, but then happily blow away the TPM state. Totally inconsistent behaviour :-( Its too late to change the default behaviour, but we likely ought to add a flag
VIR_DOMAIN_UNDEFINE_KEEP_TPM
and plumb that through the varius code paths, which would remove the need for this specific 'qemuDomainUndefineReason' enum.
I think the granularity encoded in the reason is necessary for the following patch I was going to post later on:
Subject: [PATCH] qemu: tpm: Remove TPM state with non-shared storage
This patch 'fixes' the behavior of the persistent_state TPM domain XML attribute that intends to preserve the state of the TPM but should not keep the state around on all the hosts a VM has been migrated to. It removes the TPM state directory structure from the source host upon successful migration when non-shared storage is used. Similarly, it removes it from the destination host upon migration failure when non-shared storage is used.
'persistent_state' is something that applies to transient VMs.
It's an attribute set in the TPM's domain XML and affects the TPM state permanently: persistent_state The persistent_state attribute indicates whether 'swtpm' TPM state is kept or not when a transient domain is powered off or undefined. This option can be used for preserving TPM state. By default the value is no. This attribute only works with the emulator backend. The accepted values are yes and no. Since 7.0.0 The documentation may say transient but the code seems to just not remove the state at all once a user has set this in the domain XML: static void qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm) { if (!tpm->data.emulator.persistent_state) qemuTPMEmulatorDeleteStorage(tpm); } https://github.com/libvirt/libvirt/blob/master/src/qemu/qemu_tpm.c#L707 We cannot get rid of the persistent_state in the domain XML but with --keep-tpm we would have a second method to keep the state. My guess is the author of the persistent_state patch did not want to instrument callers but achieve the effect with just producing a different XML.
What I'm suggesting is that we should have some control over this for persistent VMs, when calling 'undefine'.
ie virsh undefine --keep-nvram --keep-tpm $VMNAME
if we have this feature in the public API, then its impl should support both this feature and your future patch for 'persistent_state'.
And how do we handle removal decisions on shared storage so that the directory structure is not removed when a VM is undefined on the source machine versus we want to have the directory structure removed on the source machine upon successful migration when no shared storage is used? This TPM-specific decision for removal seems better on the qemu_tpm.c level rather than having to decide whether to keep the TPM state around at all the call-sites that patch 1/7 instrumented with the reason parameter (albeit unspecific in most cases) and pass down a KEEP_TPM parameter instead. Getting the reason for the undefine on the qemu_tpm.c level lets us make this TPM-specific decision there rather than at all sites further up in the call stack. Regards, Stefan
On Tue, Aug 23, 2022 at 09:15:52AM -0400, Stefan Berger wrote:
On 8/23/22 08:52, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 03:17:34PM -0400, Stefan Berger wrote:
On 8/22/22 12:46, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 08:05:53AM -0400, Stefan Berger wrote:
When share storage for the TPM state files has been setup betwen hosts then remove the TPM state files and directory only when undefining a VM and only if the attribute persistent_state is not set. Avoid removing the TPM state files and directory structure when a VM is migrated and shared storage is used since this would also remove those files and directory structure on the destination side.
I think our current undefine behaviour is probably flawed. We go to the trouble of refusing to remove the firmware NVRAM when undefining because it contains important VM state, but then happily blow away the TPM state. Totally inconsistent behaviour :-( Its too late to change the default behaviour, but we likely ought to add a flag
VIR_DOMAIN_UNDEFINE_KEEP_TPM
and plumb that through the varius code paths, which would remove the need for this specific 'qemuDomainUndefineReason' enum.
I think the granularity encoded in the reason is necessary for the following patch I was going to post later on:
Subject: [PATCH] qemu: tpm: Remove TPM state with non-shared storage
This patch 'fixes' the behavior of the persistent_state TPM domain XML attribute that intends to preserve the state of the TPM but should not keep the state around on all the hosts a VM has been migrated to. It removes the TPM state directory structure from the source host upon successful migration when non-shared storage is used. Similarly, it removes it from the destination host upon migration failure when non-shared storage is used.
'persistent_state' is something that applies to transient VMs.
It's an attribute set in the TPM's domain XML and affects the TPM state permanently:
persistent_state
The persistent_state attribute indicates whether 'swtpm' TPM state is kept or not when a transient domain is powered off or undefined. This option can be used for preserving TPM state. By default the value is no. This attribute only works with the emulator backend. The accepted values are yes and no. Since 7.0.0
The documentation may say transient but the code seems to just not remove the state at all once a user has set this in the domain XML:
static void qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm) { if (!tpm->data.emulator.persistent_state) qemuTPMEmulatorDeleteStorage(tpm); }
https://github.com/libvirt/libvirt/blob/master/src/qemu/qemu_tpm.c#L707
We cannot get rid of the persistent_state in the domain XML but with --keep-tpm we would have a second method to keep the state. My guess is the author of the persistent_state patch did not want to instrument callers but achieve the effect with just producing a different XML.
Yeah, IMHO, the persistent_state attribute was a mistake, but we can't get rid of it due to back compatibility. The need of persistence is something that is contextually dependant, rather than a property of the VM you can know upfront. When undefining a VM there are reasons to both delete and preserve the TPM state that can only be known at the time the undefine operation is invoked. So we can't delete the XML attr, but we can add new flags to the undefine API and define how they interact wit the persistent_state attribute. * persistent_state=absent, flags=0 => delete * persistent_state=absent, flags=UNDEFINE_TPM => delete * persistent_state=absent, flags=UNDEFINE_KEEP_TPM => keep * persistent_state=yes, flags=0, => keep * persistent_state=yes, flags=UNDEFINE_TPM => delete * persistent_state=yes, flags=UNDEFINE_KEEP_TPM => keep * persistent_state=no, flags=0, => delete * persistent_state=no, flags=UNDEFINE_TPM => delete * persistent_state=no, flags=UNDEFINE_KEEP_TPM => keep
What I'm suggesting is that we should have some control over this for persistent VMs, when calling 'undefine'.
ie virsh undefine --keep-nvram --keep-tpm $VMNAME
if we have this feature in the public API, then its impl should support both this feature and your future patch for 'persistent_state'.
And how do we handle removal decisions on shared storage so that the directory structure is not removed when a VM is undefined on the source machine versus we want to have the directory structure removed on the source machine upon successful migration when no shared storage is used? This TPM-specific decision for removal seems better on the qemu_tpm.c level rather than having to decide whether to keep the TPM state around at all the call-sites that patch 1/7 instrumented with the reason parameter (albeit unspecific in most cases) and pass down a KEEP_TPM parameter instead.
No matter what, we need to plumb in an extra parameter across the call sites.
Getting the reason for the undefine on the qemu_tpm.c level lets us make this TPM-specific decision there rather than at all sites further up in the call stack.
When exposing the above mentioned flags to virDomainUndefine I don't think the "undefine reason" makes sense as a concept to pass around, as compared to a 'bool deleteTPMState' With regards, Daniel -- |: 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 :|
Extend the domain XML with a 'shared_storage' attribute for the TPM to support migration when the TPM's state directory is setup as shared storage between hosts. Document the shared_storage attribute. For libvirt to be able to correctly handle migration and the removal and security-labeling of TPM state files, it is necessary that the domain XML indicates whether shared stored has been set up for TPM state files. If shared storage is used the TPM domain XML must indicate this as follows: <tpm model='tpm-crb'> <backend type='emulator' version='2.0' shared_storage='yes'/> </tpm> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- docs/formatdomain.rst | 16 ++++++++++++++++ src/conf/domain_conf.c | 13 +++++++++++++ src/conf/schemas/domaincommon.rng | 5 +++++ 3 files changed, 34 insertions(+) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 212104fe1f..f6eb126617 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -7775,6 +7775,22 @@ Example: usage of the TPM Emulator This attribute only works with the ``emulator`` backend. The accepted values are ``yes`` and ``no``. :since:`Since 7.0.0` +``shared_storage`` + The ``shared_storage`` attribute indicates whether shared storage is + setup for storing 'swtpm' TPM state. It must be set to ``yes`` if shared + storage is used and must be omitted or set to ``no`` otherwise. The + default value is ``no``. This attribute is important for migrating + 'swtpm' state between hosts and managing the TPM state files. + :since:`Since 8.8.0` + + Note: All hosts sharing the storage must be configured to run swtpm + with the same account (see ``swtpm_user`` and ``swtpm_group`` in qemu.conf). + Further, any Linux security module used for file labeling, such as SELinux, + must be supported by the shared storage technology and be the same on all + hosts or otherwise may need to be turned off. For example, when NFS is used + for shared storage, SELinux must be turned off or put into permissive mode + since sVirt's MLS range labeling is not supported by NFS. + ``active_pcr_banks`` The ``active_pcr_banks`` node is used to define which of the PCR banks of a TPM 2.0 to activate. Valid names are for example sha1, sha256, sha384, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2fc94b40ef..9de23d6530 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10418,6 +10418,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, g_autofree char *path = NULL; g_autofree char *secretuuid = NULL; g_autofree char *persistent_state = NULL; + g_autofree char *shared_storage = NULL; g_autofree xmlNodePtr *backends = NULL; g_autofree xmlNodePtr *nodes = NULL; int bank; @@ -10492,6 +10493,16 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, } } + shared_storage = virXMLPropString(backends[0], "shared_storage"); + if (shared_storage) { + if (virStringParseYesNo(shared_storage, + &def->data.emulator.shared_storage) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid shared_storage value, either 'yes' or 'no'")); + goto error; + } + } + if ((nnodes = virXPathNodeSet("./backend/active_pcr_banks/*", ctxt, &nodes)) < 0) break; if (nnodes > 0) @@ -24301,6 +24312,8 @@ virDomainTPMDefFormat(virBuffer *buf, } if (def->data.emulator.persistent_state) virBufferAddLit(&backendAttrBuf, " persistent_state='yes'"); + if (def->data.emulator.shared_storage) + virBufferAddLit(&backendAttrBuf, " shared_storage='yes'"); if (def->data.emulator.hassecretuuid) { char uuidstr[VIR_UUID_STRING_BUFLEN]; diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 7f6ea1d888..27000670b1 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -5541,6 +5541,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="shared_storage"> + <ref name="virYesNo"/> + </attribute> + </optional> </group> </choice> <optional> -- 2.37.1
On Mon, Aug 22, 2022 at 08:05:47AM -0400, Stefan Berger wrote:
This series of patches adds support for migrating vTPMs across hosts whose storage has been set up to share the directory structure holding the state of the TPM (swtpm). The domain XML is extended with a shared_storage attribute that must be set to 'yes' when shared storage is used. It
We don't require any 'shared_storage' attribute for disk images - we just aim to "do the right thing" automatically. If we want to support shared storage for TPM, then IMHO it should likewise be made transparent. What's the thinking behind putting the TPM on shared storage though ? The state is tiny so you're not going to notice it being transferred in the regular migration stream, so there's no performance benefit here. Meanwhile it hurts security because we can't do SELinux isolation on many NFS install, and the complexity of the dance around locking makes the migration process more fragile. The pain we've had dealing with NFS makes it really unappealing
influences the management of the directory structure holding the TPM state, which for example is only to be removed when a domain is undefined (virsh undefine) and not when a VM is removed on the migration source host. Further, when shared storage is used security labeling on the destination side is skipped assuming that the labeling was already done on the source side.
I have tested this with an NFS setup where I had to turn SELinux off on the hosts since the SELinux MLS range labeling is not supported.
Have you tested in systems where 'root_squash' is enabled on the NFS server, such that libvirt can't create the files as 'root' and then chown them to 'qemu' later ? That's been a major source of pain related to NFS for disks and snapshots historically.
Share storage migration requires the upcoming swtpm v0.8 with the PR for shared storage merged: https://github.com/stefanberger/swtpm/pull/732
Stefan
Stefan Berger (7): qemu: tpm: Pass parameter indicating reason for domain removal util: Add parsing support for swtpm's cmdarg-migration capability qemu: tpm: Conditionally create storage on incoming migration qemu: tpm: Pass --migration option to swtpm when using shared storage qemu: tpm: Avoid security labels on incoming migration with shared storage qemu: tpm: Remove TPM state files and directory only when undefining a VM qemu: config: Extend TPM domain XML with shared storage support
docs/formatdomain.rst | 16 ++++++++ src/conf/domain_conf.c | 13 +++++++ src/conf/domain_conf.h | 1 + src/conf/schemas/domaincommon.rng | 5 +++ src/qemu/qemu_domain.c | 12 +++--- src/qemu/qemu_domain.h | 8 +++- src/qemu/qemu_driver.c | 20 +++++----- src/qemu/qemu_extdevice.c | 5 ++- src/qemu/qemu_extdevice.h | 3 +- src/qemu/qemu_migration.c | 13 ++++--- src/qemu/qemu_process.c | 4 +- src/qemu/qemu_snapshot.c | 4 +- src/qemu/qemu_tpm.c | 61 ++++++++++++++++++++++++++----- src/qemu/qemu_tpm.h | 3 +- src/util/virtpm.c | 1 + src/util/virtpm.h | 1 + 16 files changed, 131 insertions(+), 39 deletions(-)
-- 2.37.1
With regards, Daniel -- |: 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 :|
On 8/22/22 12:35, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 08:05:47AM -0400, Stefan Berger wrote:
This series of patches adds support for migrating vTPMs across hosts whose storage has been set up to share the directory structure holding the state of the TPM (swtpm). The domain XML is extended with a shared_storage attribute that must be set to 'yes' when shared storage is used. It
We don't require any 'shared_storage' attribute for disk images - we just aim to "do the right thing" automatically. If we want to support shared storage for TPM, then IMHO it should likewise be made transparent.
What's the thinking behind putting the TPM on shared storage though ?
It's drive by swtpm user(s): https://github.com/stefanberger/swtpm/pull/732 The driving force is having the state available on the destination to restart a VM there if the original host failed. Allegedly all hosts in their setup would share the necessary storage to be able to do that with TPM state but then presumably also with the disk image(s).
The state is tiny so you're not going to notice it being transferred
Tiny is relative to disk sizes. It can become ~260kb or so, depending on how much is stored in the NVRAM areas, but yes, it's comparably small.
in the regular migration stream, so there's no performance benefit here. Meanwhile it hurts security because we can't do SELinux isolation on many NFS install, and the complexity of the dance around locking makes the migration process more fragile. The pain we've had dealing with NFS makes it really unappealing
Others would want to use CephFS for it I heard and I am not sure what the pain points with this shared storage tech are.
influences the management of the directory structure holding the TPM state, which for example is only to be removed when a domain is undefined (virsh undefine) and not when a VM is removed on the migration source host. Further, when shared storage is used security labeling on the destination side is skipped assuming that the labeling was already done on the source side.
I have tested this with an NFS setup where I had to turn SELinux off on the hosts since the SELinux MLS range labeling is not supported.
Have you tested in systems where 'root_squash' is enabled on the NFS server, such that libvirt can't create the files as 'root' and then chown them to 'qemu' later ? That's been a major source of pain related to NFS for disks and snapshots historically.
Yes, that doesn't seem to work. I had used it with no_root_squash before. # virsh start testVM error: Failed to start domain 'testVM' error: internal error: Could not create directory /var/lib/libvirt/swtpm/ecc221c4-6bb9-423f-ac31-72244fdbb1a1/tpm2 as 59:59 Stefan
On Mon, Aug 22, 2022 at 02:52:26PM -0400, Stefan Berger wrote:
On 8/22/22 12:35, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 08:05:47AM -0400, Stefan Berger wrote:
This series of patches adds support for migrating vTPMs across hosts whose storage has been set up to share the directory structure holding the state of the TPM (swtpm). The domain XML is extended with a shared_storage attribute that must be set to 'yes' when shared storage is used. It
We don't require any 'shared_storage' attribute for disk images - we just aim to "do the right thing" automatically. If we want to support shared storage for TPM, then IMHO it should likewise be made transparent.
What's the thinking behind putting the TPM on shared storage though ?
It's drive by swtpm user(s):
https://github.com/stefanberger/swtpm/pull/732
The driving force is having the state available on the destination to restart a VM there if the original host failed. Allegedly all hosts in their setup would share the necessary storage to be able to do that with TPM state but then presumably also with the disk image(s).
Ok, so use case is resilience rather than specifically live migration.
The state is tiny so you're not going to notice it being transferred
Tiny is relative to disk sizes. It can become ~260kb or so, depending on how much is stored in the NVRAM areas, but yes, it's comparably small.
I meant 'tiny' in the sense that the time required to live migration it is not measureably significant. Compared with say migrating disk storage, which could add hours to a live migration if it wasn't on shared storage.
in the regular migration stream, so there's no performance benefit here. Meanwhile it hurts security because we can't do SELinux isolation on many NFS install, and the complexity of the dance around locking makes the migration process more fragile. The pain we've had dealing with NFS makes it really unappealing
Others would want to use CephFS for it I heard and I am not sure what the pain points with this shared storage tech are.
influences the management of the directory structure holding the TPM state, which for example is only to be removed when a domain is undefined (virsh undefine) and not when a VM is removed on the migration source host. Further, when shared storage is used security labeling on the destination side is skipped assuming that the labeling was already done on the source side.
I have tested this with an NFS setup where I had to turn SELinux off on the hosts since the SELinux MLS range labeling is not supported.
Have you tested in systems where 'root_squash' is enabled on the NFS server, such that libvirt can't create the files as 'root' and then chown them to 'qemu' later ? That's been a major source of pain related to NFS for disks and snapshots historically.
Yes, that doesn't seem to work. I had used it with no_root_squash before.
# virsh start testVM error: Failed to start domain 'testVM' error: internal error: Could not create directory /var/lib/libvirt/swtpm/ecc221c4-6bb9-423f-ac31-72244fdbb1a1/tpm2 as 59:59
Stefan
With regards, Daniel -- |: 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 :|
On 8/23/22 08:55, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 02:52:26PM -0400, Stefan Berger wrote:
On 8/22/22 12:35, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 08:05:47AM -0400, Stefan Berger wrote:
This series of patches adds support for migrating vTPMs across hosts whose storage has been set up to share the directory structure holding the state of the TPM (swtpm). The domain XML is extended with a shared_storage attribute that must be set to 'yes' when shared storage is used. It
We don't require any 'shared_storage' attribute for disk images - we just aim to "do the right thing" automatically. If we want to support shared storage for TPM, then IMHO it should likewise be made transparent.
What's the thinking behind putting the TPM on shared storage though ?
It's drive by swtpm user(s):
https://github.com/stefanberger/swtpm/pull/732
The driving force is having the state available on the destination to restart a VM there if the original host failed. Allegedly all hosts in their setup would share the necessary storage to be able to do that with TPM state but then presumably also with the disk image(s).
Ok, so use case is resilience rather than specifically live migration.
Yes, support for HA.
The state is tiny so you're not going to notice it being transferred
Tiny is relative to disk sizes. It can become ~260kb or so, depending on how much is stored in the NVRAM areas, but yes, it's comparably small.
I meant 'tiny' in the sense that the time required to live migration it is not measureably significant. Compared with say migrating disk storage, which could add hours to a live migration if it wasn't on shared storage.
I won't change QEMU for support of shared storage setups. It will continue migrating the TPM state. However, shared storage setups need special management support, which this series tries to address.
participants (2)
-
Daniel P. Berrangé -
Stefan Berger