[PATCH v4 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 existence of share storage influences the management of the directory structure holding the TPM state, which for example is only removed when a domain is undefined and not when a VM is removed on the migration source host. Further, when shared storage is used then 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 by NFS. For shared storage support to work properly both sides of the migration need to be clients of the shared storage setup, meaning that they both have to have /var/lib/libvirt/swtpm mounted as shared storage, because other- wise the virFileIsSharedFS() may not detect shared storage and in the worst case may cause the TPM emulator (swtpm) to malfunction if for example the source side removed the TPM state directory structure. Shared storage migration requires (upcoming) swtpm v0.8. Stefan v4: - Fixed long-standing bug regarding offline migration that now blocks if no shared storage is set up - Addressed Michal's concerns v3: - Relying entirely on virFileIsSharedFS() on migration source and destination sides to detect whether shared storage is set up between hosts; no more hint about shared storage from user via flag - Added support for virDomainTPMPrivate structure to store and persist TPM-related private data Stefan Berger (7): util: Add parsing support for swtpm's cmdarg-migration capability qemu: tpm: Allow offline migration with TPM_EMULATOR only with shared storage qemu: tpm: Conditionally create storage on incoming migration qemu: tpm: Add support for storing private TPM-related data qemu: tpm: Pass --migration option to swtpm if supported and needed qemu: tpm: Avoid security labels on incoming migration with shared storage qemu: tpm: Never remove state on outgoing migration and shared storage src/conf/domain_conf.c | 63 ++++++++++++++++++-- src/conf/domain_conf.h | 9 +++ src/qemu/qemu_domain.c | 85 ++++++++++++++++++++++++-- src/qemu/qemu_domain.h | 17 +++++- src/qemu/qemu_driver.c | 20 +++---- src/qemu/qemu_extdevice.c | 10 ++-- src/qemu/qemu_extdevice.h | 6 +- src/qemu/qemu_migration.c | 28 +++++++-- src/qemu/qemu_process.c | 9 ++- src/qemu/qemu_snapshot.c | 4 +- src/qemu/qemu_tpm.c | 122 ++++++++++++++++++++++++++++++++++---- src/qemu/qemu_tpm.h | 14 ++++- src/util/virtpm.c | 1 + src/util/virtpm.h | 1 + 14 files changed, 339 insertions(+), 50 deletions(-) -- 2.37.3
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 91db0f31eb..19850de1c8 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.3
Allow migration with TPM_EMULATOR (swtpm) only if shared storage has been set up for the TPM state directory. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_migration.c | 6 ++++++ src/qemu/qemu_tpm.c | 28 ++++++++++++++++++++++++++++ src/qemu/qemu_tpm.h | 5 +++++ 3 files changed, 39 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 33105cf07b..16bf7ac178 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -38,6 +38,7 @@ #include "qemu_security.h" #include "qemu_slirp.h" #include "qemu_block.h" +#include "qemu_tpm.h" #include "domain_audit.h" #include "virlog.h" @@ -2579,6 +2580,11 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, _("tunnelled offline migration does not make sense")); return NULL; } + if (qemuTPMHasSharedStorage(driver, vm->def) != 1) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("offline migration requires TPM state directory to be on shared storage")); + return NULL; + } } if (flags & VIR_MIGRATE_ZEROCOPY && !(flags & VIR_MIGRATE_PARALLEL)) { diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index dc09c94a4d..5f89a6bb18 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -954,6 +954,34 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, } +int +qemuTPMHasSharedStorage(virQEMUDriver *driver, + virDomainDef *def) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + size_t i; + + for (i = 0; i < def->ntpms; i++) { + virDomainTPMDef *tpm = def->tpms[i]; + + switch (tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + if (qemuTPMEmulatorInitPaths(tpm, + cfg->swtpmStorageDir, + cfg->swtpmLogDir, + def->name, + def->uuid) < 0) + return -1; + return virFileIsSharedFS(tpm->data.emulator.storagepath); + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + } + } + + return 0; +} + + /* --------------------- * Module entry points * --------------------- diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h index f068f3ca5a..531d93846b 100644 --- a/src/qemu/qemu_tpm.h +++ b/src/qemu/qemu_tpm.h @@ -56,3 +56,8 @@ int qemuExtTPMSetupCgroup(virQEMUDriver *driver, virCgroup *cgroup) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; + +int qemuTPMHasSharedStorage(virQEMUDriver *driver, + virDomainDef *def) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) + G_GNUC_WARN_UNUSED_RESULT; -- 2.37.3
On 10/24/22 12:28, Stefan Berger wrote:
Allow migration with TPM_EMULATOR (swtpm) only if shared storage has been set up for the TPM state directory.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_migration.c | 6 ++++++ src/qemu/qemu_tpm.c | 28 ++++++++++++++++++++++++++++ src/qemu/qemu_tpm.h | 5 +++++ 3 files changed, 39 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 33105cf07b..16bf7ac178 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -38,6 +38,7 @@ #include "qemu_security.h" #include "qemu_slirp.h" #include "qemu_block.h" +#include "qemu_tpm.h"
#include "domain_audit.h" #include "virlog.h" @@ -2579,6 +2580,11 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, _("tunnelled offline migration does not make sense")); return NULL; } + if (qemuTPMHasSharedStorage(driver, vm->def) != 1) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("offline migration requires TPM state directory to be on shared storage"));
Does it though? We don't have such requirement for say domain disks. I believe the point of --offline is to merely just get domain XML an define it on the destination. That's why --offline requires --persistent and why a lot of checks are skipped when --offline. I mean, for disks we just assume users will copy them onto destination (or use a shared FS). We can assume the same for TPM, can't we? This would also allow us to simplify qemuTPMHasSharedStorage() because it no longer needs to call qemuTPMEmulatorInitPaths() because for running guests the paths were already initialized. And if we chose to keep this check in, then I think it should live in qemuMigrationSrcIsAllowed() which is the places where similar checks are performed. Michal
On 11/7/22 03:31, Michal Prívozník wrote:
On 10/24/22 12:28, Stefan Berger wrote:
Allow migration with TPM_EMULATOR (swtpm) only if shared storage has been set up for the TPM state directory.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_migration.c | 6 ++++++ src/qemu/qemu_tpm.c | 28 ++++++++++++++++++++++++++++ src/qemu/qemu_tpm.h | 5 +++++ 3 files changed, 39 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 33105cf07b..16bf7ac178 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -38,6 +38,7 @@ #include "qemu_security.h" #include "qemu_slirp.h" #include "qemu_block.h" +#include "qemu_tpm.h"
#include "domain_audit.h" #include "virlog.h" @@ -2579,6 +2580,11 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, _("tunnelled offline migration does not make sense")); return NULL; } + if (qemuTPMHasSharedStorage(driver, vm->def) != 1) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("offline migration requires TPM state directory to be on shared storage"));
Does it though? We don't have such requirement for say domain disks. I believe the point of --offline is to merely just get domain XML an define it on the destination. That's why --offline requires --persistent and why a lot of checks are skipped when --offline.
I mean, for disks we just assume users will copy them onto destination (or use a shared FS). We can assume the same for TPM, can't we? This would also allow us to simplify qemuTPMHasSharedStorage() because it no longer needs to call qemuTPMEmulatorInitPaths() because for running guests the paths were already initialized.
And if we chose to keep this check in, then I think it should live in qemuMigrationSrcIsAllowed() which is the places where similar checks are performed.
I don't mind dropping the patch then. Stefan
Michal
Do not create storage if the TPM state files are on shared storage and there's an incoming migration since in this case the storage directory must already exist. Also do not run swtpm_setup in this case. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_tpm.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 5f89a6bb18..79d7a0e671 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -556,11 +556,20 @@ 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 && + virFileIsSharedFS(tpm->data.emulator.storagepath) == 1) + create_storage = false; + + if (create_storage && + qemuTPMEmulatorCreateStorage(tpm, &created, swtpm_user, swtpm_group) < 0) return NULL; if (tpm->data.emulator.hassecretuuid) -- 2.37.3
Add support for storing private TPM-related data. The first private data will be related to the capability of the started swtpm indicating whether it is capable of migration with a shared storage setup since that requires support for certain command line flags that were only becoming available in v0.8. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/conf/domain_conf.c | 63 +++++++++++++++++++++++++++++++++--- src/conf/domain_conf.h | 9 ++++++ src/qemu/qemu_domain.c | 73 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 14 ++++++++ 4 files changed, 154 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7dba65cfeb..4178583950 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3276,6 +3276,22 @@ void virDomainHostdevDefClear(virDomainHostdevDef *def) } } +static virDomainTPMDef * +virDomainTPMDefNew(virDomainXMLOption *xmlopt) +{ + virDomainTPMDef *def; + + def = g_new0(virDomainTPMDef, 1); + + if (xmlopt && xmlopt->privateData.tpmNew && + !(def->privateData = xmlopt->privateData.tpmNew())) { + VIR_FREE(def); + return NULL; + } + + return def; +} + void virDomainTPMDefFree(virDomainTPMDef *def) { if (!def) @@ -3296,6 +3312,7 @@ void virDomainTPMDefFree(virDomainTPMDef *def) } virDomainDeviceInfoClear(&def->info); + virObjectUnref(def->privateData); g_free(def); } @@ -10238,7 +10255,8 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, g_autofree xmlNodePtr *nodes = NULL; int bank; - def = g_new0(virDomainTPMDef, 1); + if (!(def = virDomainTPMDefNew(xmlopt))) + return NULL; if (virXMLPropEnum(node, "model", virDomainTPMModelTypeFromString, @@ -10329,6 +10347,14 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt, &def->info, flags) < 0) goto error; + if (flags & VIR_DOMAIN_DEF_PARSE_STATUS && + xmlopt && xmlopt->privateData.tpmParse) { + if ((ctxt->node = virXPathNode("./privateData", ctxt))) { + if (xmlopt->privateData.tpmParse(ctxt, def) < 0) + goto error; + } + } + return def; error: @@ -24049,10 +24075,32 @@ virDomainSoundCodecDefFormat(virBuffer *buf, return 0; } -static void +static int +virDomainTPMDefFormatPrivateData(virBuffer *buf, + const virDomainTPMDef *tpm, + unsigned int flags, + virDomainXMLOption *xmlopt) +{ + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + + if (!(flags & VIR_DOMAIN_DEF_FORMAT_STATUS) || + !xmlopt || + !xmlopt->privateData.tpmFormat) + return 0; + + if (xmlopt->privateData.tpmFormat(tpm, &childBuf) < 0) + return -1; + + virXMLFormatElement(buf, "privateData", NULL, &childBuf); + return 0; +} + + +static int virDomainTPMDefFormat(virBuffer *buf, const virDomainTPMDef *def, - unsigned int flags) + unsigned int flags, + virDomainXMLOption *xmlopt) { g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); @@ -24101,8 +24149,12 @@ virDomainTPMDefFormat(virBuffer *buf, virXMLFormatElement(&childBuf, "backend", &backendAttrBuf, &backendChildBuf); virDomainDeviceInfoFormat(&childBuf, &def->info, flags); + if (virDomainTPMDefFormatPrivateData(&childBuf, def, flags, xmlopt) < 0) + return -1; virXMLFormatElement(buf, "tpm", &attrBuf, &childBuf); + + return 0; } @@ -27188,7 +27240,8 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, } for (n = 0; n < def->ntpms; n++) { - virDomainTPMDefFormat(buf, def->tpms[n], flags); + if (virDomainTPMDefFormat(buf, def->tpms[n], flags, xmlopt) < 0) + return -1; } for (n = 0; n < def->ngraphics; n++) { @@ -28454,7 +28507,7 @@ virDomainDeviceDefCopy(virDomainDeviceDef *src, rc = virDomainChrDefFormat(&buf, src->data.chr, flags); break; case VIR_DOMAIN_DEVICE_TPM: - virDomainTPMDefFormat(&buf, src->data.tpm, flags); + virDomainTPMDefFormat(&buf, src->data.tpm, flags, xmlopt); rc = 0; break; case VIR_DOMAIN_DEVICE_PANIC: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8f8a54bc41..82f71f8853 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1445,6 +1445,8 @@ typedef enum { #define VIR_DOMAIN_TPM_DEFAULT_DEVICE "/dev/tpm0" struct _virDomainTPMDef { + virObject *privateData; + virDomainTPMModel model; virDomainTPMBackendType type; virDomainDeviceInfo info; @@ -3248,6 +3250,10 @@ typedef int (*virDomainXMLPrivateDataStorageSourceParseFunc)(xmlXPathContextPtr typedef int (*virDomainXMLPrivateDataStorageSourceFormatFunc)(virStorageSource *src, virBuffer *buf); +typedef int (*virDomainXMLPrivateDataTPMParseFunc)(xmlXPathContextPtr ctxt, + virDomainTPMDef *disk); +typedef int (*virDomainXMLPrivateDataTPMFormatFunc)(const virDomainTPMDef *tpm, + virBuffer *buf); struct _virDomainXMLPrivateDataCallbacks { virDomainXMLPrivateDataAllocFunc alloc; @@ -3264,6 +3270,9 @@ struct _virDomainXMLPrivateDataCallbacks { virDomainXMLPrivateDataNewFunc networkNew; virDomainXMLPrivateDataNewFunc videoNew; virDomainXMLPrivateDataNewFunc fsNew; + virDomainXMLPrivateDataTPMParseFunc tpmParse; + virDomainXMLPrivateDataTPMFormatFunc tpmFormat; + virDomainXMLPrivateDataNewFunc tpmNew; virDomainXMLPrivateDataFormatFunc format; virDomainXMLPrivateDataParseFunc parse; /* following function shall return a pointer which will be used as the diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9ef6c8bb64..41333f1725 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1139,6 +1139,76 @@ qemuDomainVideoPrivateDispose(void *obj) } +static virClass *qemuDomainTPMPrivateClass; +static void qemuDomainTPMPrivateDispose(void *obj); + + +static int +qemuDomainTPMPrivateOnceInit(void) +{ + if (!VIR_CLASS_NEW(qemuDomainTPMPrivate, virClassForObject())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(qemuDomainTPMPrivate); + + +static virObject * +qemuDomainTPMPrivateNew(void) +{ + qemuDomainTPMPrivate *priv; + + if (qemuDomainTPMPrivateInitialize() < 0) + return NULL; + + if (!(priv = virObjectNew(qemuDomainTPMPrivateClass))) + return NULL; + + return (virObject *) priv; +} + + +static void +qemuDomainTPMPrivateDispose(void *obj G_GNUC_UNUSED) +{ +} + + +static int +qemuDomainTPMPrivateParse(xmlXPathContextPtr ctxt, + virDomainTPMDef *tpm) +{ + qemuDomainTPMPrivate *priv = QEMU_DOMAIN_TPM_PRIVATE(tpm); + + priv->swtpm.can_migrate_shared_storage = + virXPathBoolean("string(./swtpm/@can_migrate_shared_storage)", ctxt); + + return 0; +} + + +static int +qemuDomainTPMPrivateFormat(const virDomainTPMDef *tpm, + virBuffer *buf) +{ + qemuDomainTPMPrivate *priv = QEMU_DOMAIN_TPM_PRIVATE(tpm); + + switch (tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + if (priv->swtpm.can_migrate_shared_storage) + virBufferAddLit(buf, "<swtpm can_migrate_shared_storage='yes'/>\n"); + break; + + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + } + + return 0; +} + + /* qemuDomainSecretInfoSetup: * @priv: pointer to domain private object * @alias: alias of the secret @@ -3215,6 +3285,9 @@ virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = { .graphicsNew = qemuDomainGraphicsPrivateNew, .networkNew = qemuDomainNetworkPrivateNew, .videoNew = qemuDomainVideoPrivateNew, + .tpmNew = qemuDomainTPMPrivateNew, + .tpmParse = qemuDomainTPMPrivateParse, + .tpmFormat = qemuDomainTPMPrivateFormat, .parse = qemuDomainObjPrivateXMLParse, .format = qemuDomainObjPrivateXMLFormat, .getParseOpaque = qemuDomainObjPrivateXMLGetParseOpaque, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2bbd492d62..919ce16097 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -414,6 +414,20 @@ struct _qemuDomainNetworkPrivate { qemuFDPass *vdpafd; }; + +#define QEMU_DOMAIN_TPM_PRIVATE(dev) \ + ((qemuDomainTPMPrivate *) (dev)->privateData) + +typedef struct _qemuDomainTPMPrivate qemuDomainTPMPrivate; +struct _qemuDomainTPMPrivate { + virObject parent; + + struct { + bool can_migrate_shared_storage; + } swtpm; +}; + + void qemuDomainNetworkPrivateClearFDs(qemuDomainNetworkPrivate *priv); -- 2.37.3
Pass the --migration option to swtpm if swptm supports it (starting with v0.8) and if the TPM's state is written on shared storage. If this is the case 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. If a started swtpm instance is running with the necessary options of migrating with share storage then remember this with a flag in the virDomainTPMPrivateDef. Report an error if swtpm does not support the --migration option and an incoming migration across shared storage is requested. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_migration.c | 10 ++++++++ src/qemu/qemu_tpm.c | 48 +++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_tpm.h | 3 +++ 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 16bf7ac178..2aa0b6e89e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2786,6 +2786,7 @@ qemuMigrationSrcBegin(virConnectPtr conn, g_autofree char *xml = NULL; char *ret = NULL; virDomainAsyncJob asyncJob; + int rc; if (cfg->migrateTLSForce && !(flags & VIR_MIGRATE_TUNNELLED) && @@ -2795,6 +2796,15 @@ qemuMigrationSrcBegin(virConnectPtr conn, goto cleanup; } + rc = qemuTPMHasSharedStorage(driver, vm->def); + if (rc < 0) + goto cleanup; + if (rc == 1 && !qemuTPMCanMigrateSharedStorage(vm->def)) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("the running swtpm does not support migration with shared storage")); + goto cleanup; + } + if (flags & VIR_MIGRATE_POSTCOPY_RESUME) { ret = qemuMigrationSrcBeginResumePhase(conn, driver, vm, xmlin, cookieout, cookieoutlen, flags); diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 79d7a0e671..cffa77cfa3 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -557,6 +557,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, int migpwdfile_fd = -1; const unsigned char *secretuuid = NULL; bool create_storage = true; + bool on_shared_storage; if (!swtpm) return NULL; @@ -564,8 +565,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, /* Do not create storage and run swtpm_setup on incoming migration over * shared storage */ - if (incomingMigration && - virFileIsSharedFS(tpm->data.emulator.storagepath) == 1) + on_shared_storage = virFileIsSharedFS(tpm->data.emulator.storagepath) == 1; + if (incomingMigration && on_shared_storage) create_storage = false; if (create_storage && @@ -643,6 +644,30 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc", migpwdfile_fd); } + /* If swtpm supports it and the TPM state is stored on shared storage, + * start swtpm with --migration release-lock-outgoing so it can migrate + * across shared storage if needed. + */ + QEMU_DOMAIN_TPM_PRIVATE(tpm)->swtpm.can_migrate_shared_storage = false; + if (on_shared_storage && + virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_MIGRATION)) { + + virCommandAddArg(cmd, "--migration"); + virCommandAddArgFormat(cmd, "release-lock-outgoing%s", + incomingMigration ? ",incoming": ""); + QEMU_DOMAIN_TPM_PRIVATE(tpm)->swtpm.can_migrate_shared_storage = true; + } else { + /* Report an error if there's an incoming migration across shared + * storage and swtpm does not support the --migration option. + */ + if (incomingMigration && on_shared_storage) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("%s (on destination side) does not support the --migration option needed for migration with shared storage"), + swtpm); + goto error; + } + } + return g_steal_pointer(&cmd); error: @@ -991,6 +1016,25 @@ qemuTPMHasSharedStorage(virQEMUDriver *driver, } +bool +qemuTPMCanMigrateSharedStorage(virDomainDef *def) +{ + size_t i; + + for (i = 0; i < def->ntpms; i++) { + virDomainTPMDef *tpm = def->tpms[i]; + switch (tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + return QEMU_DOMAIN_TPM_PRIVATE(tpm)-> + swtpm.can_migrate_shared_storage; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + } + } + return true; +} + + /* --------------------- * Module entry points * --------------------- diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h index 531d93846b..e6e32a0c4a 100644 --- a/src/qemu/qemu_tpm.h +++ b/src/qemu/qemu_tpm.h @@ -61,3 +61,6 @@ int qemuTPMHasSharedStorage(virQEMUDriver *driver, virDomainDef *def) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; + +bool qemuTPMCanMigrateSharedStorage(virDomainDef *def) + ATTRIBUTE_NONNULL(1); -- 2.37.3
On 10/24/22 12:28, Stefan Berger wrote:
Pass the --migration option to swtpm if swptm supports it (starting with v0.8) and if the TPM's state is written on shared storage. If this is the case 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.
If a started swtpm instance is running with the necessary options of migrating with share storage then remember this with a flag in the virDomainTPMPrivateDef.
Report an error if swtpm does not support the --migration option and an incoming migration across shared storage is requested.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_migration.c | 10 ++++++++ src/qemu/qemu_tpm.c | 48 +++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_tpm.h | 3 +++ 3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 16bf7ac178..2aa0b6e89e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2786,6 +2786,7 @@ qemuMigrationSrcBegin(virConnectPtr conn, g_autofree char *xml = NULL; char *ret = NULL; virDomainAsyncJob asyncJob; + int rc;
if (cfg->migrateTLSForce && !(flags & VIR_MIGRATE_TUNNELLED) && @@ -2795,6 +2796,15 @@ qemuMigrationSrcBegin(virConnectPtr conn, goto cleanup; }
+ rc = qemuTPMHasSharedStorage(driver, vm->def); + if (rc < 0) + goto cleanup; + if (rc == 1 && !qemuTPMCanMigrateSharedStorage(vm->def)) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("the running swtpm does not support migration with shared storage")); + goto cleanup; + } +
This check is correct, but as I said in my other reply, I think it should live in qemuMigrationSrcIsAllowed(). Michal
On 11/7/22 03:31, Michal Prívozník wrote:
On 10/24/22 12:28, Stefan Berger wrote:
Pass the --migration option to swtpm if swptm supports it (starting with v0.8) and if the TPM's state is written on shared storage. If this is the case 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.
If a started swtpm instance is running with the necessary options of migrating with share storage then remember this with a flag in the virDomainTPMPrivateDef.
Report an error if swtpm does not support the --migration option and an incoming migration across shared storage is requested.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_migration.c | 10 ++++++++ src/qemu/qemu_tpm.c | 48 +++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_tpm.h | 3 +++ 3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 16bf7ac178..2aa0b6e89e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2786,6 +2786,7 @@ qemuMigrationSrcBegin(virConnectPtr conn, g_autofree char *xml = NULL; char *ret = NULL; virDomainAsyncJob asyncJob; + int rc;
if (cfg->migrateTLSForce && !(flags & VIR_MIGRATE_TUNNELLED) && @@ -2795,6 +2796,15 @@ qemuMigrationSrcBegin(virConnectPtr conn, goto cleanup; }
+ rc = qemuTPMHasSharedStorage(driver, vm->def); + if (rc < 0) + goto cleanup; + if (rc == 1 && !qemuTPMCanMigrateSharedStorage(vm->def)) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("the running swtpm does not support migration with shared storage")); + goto cleanup; + } +
This check is correct, but as I said in my other reply, I think it should live in qemuMigrationSrcIsAllowed().
I can move it. Stefan
Michal
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 cffa77cfa3..5a0d298052 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -932,10 +932,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 && + virFileIsSharedFS(tpm->data.emulator.storagepath) == 1) { + /* 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.3
On 10/24/22 12:28, Stefan Berger wrote:
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 cffa77cfa3..5a0d298052 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -932,10 +932,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 && + virFileIsSharedFS(tpm->data.emulator.storagepath) == 1) { + /* 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()
Turns out, this is patch is problematic with SELinux [1]. Couple of problems here: 1) When a domain is being started up, libvirt creates an unique SELinux label. This is not migrated onto the destination - here another unique label is created on incoming migration. This means that the state might be not available to the destination swtpm binary. 2) the log file should be relabelled regardless - it's not on the shared volume. And since its label is set in qemuSecurityStartTPMEmulator(), it won't be set on the destination. While I could deal with 2), I am not sure what to do with 1). Because when I tried to set the label (basically by reverting this patch), the destination was unhappy as it could not lock the .lock file. Stefan, do you have any bright idea how to fix this? 1: https://bugzilla.redhat.com/show_bug.cgi?id=2130192 Michal
On 11/29/22 09:04, Michal Prívozník wrote:
On 10/24/22 12:28, Stefan Berger wrote:
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 cffa77cfa3..5a0d298052 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -932,10 +932,19 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virCommandSetPidFile(cmd, pidfile); virCommandSetErrorFD(cmd, &errfd);
- if (qsudo tpm2_nvdefine -C o -a "nt=extend|ownerread|ownerwrite|policywrite|writedefine|orderly" 2(driver, vm, cmd, - cfg->swtpm_user, cfg->swtpm_group, - NULL, &cmdret) < 0) - return -1; + if (incomingMigration && + virFileIsSharedFS(tpm->data.emulator.storagepath) == 1) { + /* 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()
Turns out, this is patch is problematic with SELinux [1]. Couple of problems here:
1) When a domain is being started up, libvirt creates an unique SELinux label. This is not migrated onto the destination - here another unique label is created on incoming migration. This means that the state might be not available to the destination swtpm binary.
2) the log file should be relabelled regardless - it's not on the shared volume. And since its label is set in qemuSecurityStartTPMEmulator(), it won't be set on the destination.
While I could deal with 2), I am not sure what to do with 1). Because when I tried to set the label (basically by reverting this patch), the destination was unhappy as it could not lock the .lock file.
Stefan, do you have any bright idea how to fix this?
Sorry, no, unfortunately I don't. SELinux and NFS had some issues and due to other reasons as well I had to run my machine in permissive mode but I don't remember the exact details. What I at least was not able to do was to run qemuSecurityStartTPMEmulator on the destination side - this didn't work just to begin with. Also, NFS is one example of a shared filesystem and I am not sure what issues other shared filesystems may or may not have when SELinux is used -- could a shared filesystem entertain local SELinux labels and not have the issue that NFS has with SELinux labels? In the worst case we may have to put a blocker into the code if SELinux(+NFS) is being used. Migration across shared storage shouldn't be a problem with AppArmor at least. Stefan
1: https://bugzilla.redhat.com/show_bug.cgi?id=2130192
Michal
Never remove the TPM state on outgoing migration if the storage setup has shared storage for the TPM state files. Also, do not do the security cleanup on outgoing migration if shared storage is detected. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_domain.c | 12 +++++++----- src/qemu/qemu_domain.h | 3 ++- src/qemu/qemu_driver.c | 20 ++++++++++---------- src/qemu/qemu_extdevice.c | 10 ++++++---- src/qemu/qemu_extdevice.h | 6 ++++-- src/qemu/qemu_migration.c | 12 ++++++------ src/qemu/qemu_process.c | 9 ++++++--- src/qemu/qemu_snapshot.c | 4 ++-- src/qemu/qemu_tpm.c | 22 +++++++++++++++++----- src/qemu/qemu_tpm.h | 6 ++++-- 10 files changed, 64 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 41333f1725..acfa60bc2c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7257,7 +7257,8 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, static void qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, virDomainObj *vm, - virDomainUndefineFlagsValues flags) + virDomainUndefineFlagsValues flags, + bool outgoingMigration) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *snapDir = NULL; @@ -7283,7 +7284,7 @@ qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, if (rmdir(chkDir) < 0 && errno != ENOENT) VIR_WARN("unable to remove checkpoint directory %s", chkDir); } - qemuExtDevicesCleanupHost(driver, vm->def, flags); + qemuExtDevicesCleanupHost(driver, vm->def, flags, outgoingMigration); } @@ -7295,14 +7296,15 @@ qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, void qemuDomainRemoveInactive(virQEMUDriver *driver, virDomainObj *vm, - virDomainUndefineFlagsValues flags) + virDomainUndefineFlagsValues flags, + bool outgoingMigration) { if (vm->persistent) { /* Short-circuit, we don't want to remove a persistent domain */ return; } - qemuDomainRemoveInactiveCommon(driver, vm, flags); + qemuDomainRemoveInactiveCommon(driver, vm, flags, outgoingMigration); virDomainObjListRemove(driver->domains, vm); } @@ -7324,7 +7326,7 @@ qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, return; } - qemuDomainRemoveInactiveCommon(driver, vm, 0); + qemuDomainRemoveInactiveCommon(driver, vm, 0, false); virDomainObjListRemoveLocked(driver->domains, vm); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 919ce16097..7950c4c2da 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -703,7 +703,8 @@ int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, void qemuDomainRemoveInactive(virQEMUDriver *driver, virDomainObj *vm, - virDomainUndefineFlagsValues flags); + virDomainUndefineFlagsValues flags, + bool outgoingMigration); void qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 59a3b37b98..a4a5970b8c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1611,7 +1611,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, goto cleanup; if (qemuProcessBeginJob(vm, VIR_DOMAIN_JOB_OPERATION_START, flags) < 0) { - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); goto cleanup; } @@ -1620,7 +1620,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags) < 0) { virDomainAuditStart(vm, "booted", false); - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); qemuProcessEndJob(vm); goto cleanup; } @@ -2103,7 +2103,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, ret = 0; endjob: if (ret == 0) - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); virDomainObjEndJob(vm); cleanup: @@ -2723,7 +2723,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver, } virDomainObjEndAsyncJob(vm); if (ret == 0) - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); cleanup: virQEMUSaveDataFree(data); @@ -3263,7 +3263,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, virDomainObjEndAsyncJob(vm); if (ret == 0 && flags & VIR_DUMP_CRASH) - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); cleanup: virDomainObjEndAPI(&vm); @@ -3575,7 +3575,7 @@ processGuestPanicEvent(virQEMUDriver *driver, endjob: virDomainObjEndAsyncJob(vm); if (removeInactive) - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); } @@ -3809,7 +3809,7 @@ processMonitorEOFEvent(virQEMUDriver *driver, virObjectEventStateQueue(driver->domainEventState, event); endjob: - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); virDomainObjEndJob(vm); } @@ -5741,7 +5741,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, virFileWrapperFdFree(wrapperFd); virQEMUSaveDataFree(data); if (vm && ret < 0) - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); virDomainObjEndAPI(&vm); return ret; } @@ -6431,7 +6431,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, } else { /* Brand new domain. Remove it */ VIR_INFO("Deleting domain '%s'", vm->def->name); - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); } } @@ -6580,7 +6580,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, */ vm->persistent = 0; if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm, flags); + qemuDomainRemoveInactive(driver, vm, flags, false); ret = 0; endjob: diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index 24a57b0f74..3eaf6571a2 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -152,7 +152,8 @@ qemuExtDevicesPrepareHost(virQEMUDriver *driver, void qemuExtDevicesCleanupHost(virQEMUDriver *driver, virDomainDef *def, - virDomainUndefineFlagsValues flags) + virDomainUndefineFlagsValues flags, + bool outgoingMigration) { size_t i; @@ -160,7 +161,7 @@ qemuExtDevicesCleanupHost(virQEMUDriver *driver, return; for (i = 0; i < def->ntpms; i++) { - qemuExtTPMCleanupHost(def->tpms[i], flags); + qemuExtTPMCleanupHost(def->tpms[i], flags, outgoingMigration); } } @@ -225,7 +226,8 @@ qemuExtDevicesStart(virQEMUDriver *driver, void qemuExtDevicesStop(virQEMUDriver *driver, - virDomainObj *vm) + virDomainObj *vm, + bool outgoingMigration) { virDomainDef *def = vm->def; size_t i; @@ -242,7 +244,7 @@ qemuExtDevicesStop(virQEMUDriver *driver, for (i = 0; i < def->ntpms; i++) { if (def->tpms[i]->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) - qemuExtTPMStop(driver, vm); + qemuExtTPMStop(driver, vm, outgoingMigration); } for (i = 0; i < def->nnets; i++) { diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h index 6b05b59cd6..86e7133a2a 100644 --- a/src/qemu/qemu_extdevice.h +++ b/src/qemu/qemu_extdevice.h @@ -42,7 +42,8 @@ int qemuExtDevicesPrepareHost(virQEMUDriver *driver, void qemuExtDevicesCleanupHost(virQEMUDriver *driver, virDomainDef *def, - virDomainUndefineFlagsValues flags) + virDomainUndefineFlagsValues flags, + bool outgoingMigration) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuExtDevicesStart(virQEMUDriver *driver, @@ -52,7 +53,8 @@ int qemuExtDevicesStart(virQEMUDriver *driver, G_GNUC_WARN_UNUSED_RESULT; void qemuExtDevicesStop(virQEMUDriver *driver, - virDomainObj *vm) + virDomainObj *vm, + bool outgoingMigration) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); bool qemuExtDevicesHasDevice(virDomainDef *def); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2aa0b6e89e..c47fdce253 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3407,7 +3407,7 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, * and there is no 'goto cleanup;' in the middle of those */ VIR_FREE(priv->origname); virDomainObjRemoveTransientDef(vm); - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); } virDomainObjEndAPI(&vm); virErrorRestore(&origErr); @@ -4052,7 +4052,7 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver, virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); vm->persistent = 0; } - qemuDomainRemoveInactive(driver, vm, VIR_DOMAIN_UNDEFINE_TPM); + qemuDomainRemoveInactive(driver, vm, VIR_DOMAIN_UNDEFINE_TPM, true); } cleanup: @@ -6055,7 +6055,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); vm->persistent = 0; } - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, true); } virErrorRestore(&orig_err); @@ -6182,7 +6182,7 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, true); return ret; } @@ -6718,7 +6718,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm, VIR_DOMAIN_UNDEFINE_TPM); + qemuDomainRemoveInactive(driver, vm, VIR_DOMAIN_UNDEFINE_TPM, false); virErrorRestore(&orig_err); return NULL; @@ -6855,7 +6855,7 @@ qemuMigrationProcessUnattended(virQEMUDriver *driver, qemuMigrationJobFinish(vm); if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f405326312..14adba255b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8209,6 +8209,7 @@ void qemuProcessStop(virQEMUDriver *driver, g_autofree char *timestamp = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virConnect) conn = NULL; + bool outgoingMigration; VIR_DEBUG("Shutting down vm=%p name=%s id=%d pid=%lld, " "reason=%s, asyncJob=%s, flags=0x%x", @@ -8306,7 +8307,9 @@ void qemuProcessStop(virQEMUDriver *driver, qemuDomainCleanupRun(driver, vm); - qemuExtDevicesStop(driver, vm); + outgoingMigration = (flags & VIR_QEMU_PROCESS_STOP_MIGRATED) && + (asyncJob != VIR_ASYNC_JOB_MIGRATION_IN); + qemuExtDevicesStop(driver, vm, outgoingMigration); qemuDBusStop(driver, vm); @@ -8572,7 +8575,7 @@ qemuProcessAutoDestroy(virDomainObj *dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - qemuDomainRemoveInactive(driver, dom, 0); + qemuDomainRemoveInactive(driver, dom, 0, false); virDomainObjEndJob(dom); @@ -9038,7 +9041,7 @@ qemuProcessReconnect(void *opaque) if (jobStarted) virDomainObjEndJob(obj); if (!virDomainObjIsActive(obj)) - qemuDomainRemoveInactive(driver, obj, 0); + qemuDomainRemoveInactive(driver, obj, 0, false); virDomainObjEndAPI(&obj); virIdentitySetCurrent(NULL); return; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 06b5c180ff..d7983c134f 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, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); return -1; } @@ -2125,7 +2125,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) { - qemuDomainRemoveInactive(driver, vm, 0); + qemuDomainRemoveInactive(driver, vm, 0, false); return -1; } detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 5a0d298052..ec78697c38 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -728,13 +728,22 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm, * qemuTPMEmulatorCleanupHost: * @tpm: TPM definition * @flags: flags indicating whether to keep or remove TPM persistent state + * @outgoingMigration: whether cleanup is due to an outgoing migration * * Clean up persistent storage for the swtpm. */ static void qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, - virDomainUndefineFlagsValues flags) + virDomainUndefineFlagsValues flags, + bool outgoingMigration) { + /* Never remove the state in case of outgoing migration with shared + * storage. + */ + if (outgoingMigration && + virFileIsSharedFS(tpm->data.emulator.storagepath) == 1) + return; + /* * remove TPM state if: * - persistent_state flag is set and the UNDEFINE_TPM flag is set @@ -1091,9 +1100,10 @@ qemuExtTPMPrepareHost(virQEMUDriver *driver, void qemuExtTPMCleanupHost(virDomainTPMDef *tpm, - virDomainUndefineFlagsValues flags) + virDomainUndefineFlagsValues flags, + bool outgoingMigration) { - qemuTPMEmulatorCleanupHost(tpm, flags); + qemuTPMEmulatorCleanupHost(tpm, flags, outgoingMigration); } @@ -1114,7 +1124,8 @@ qemuExtTPMStart(virQEMUDriver *driver, void qemuExtTPMStop(virQEMUDriver *driver, - virDomainObj *vm) + virDomainObj *vm, + bool outgoingMigration) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *shortName = virDomainDefGetShortName(vm->def); @@ -1123,7 +1134,8 @@ qemuExtTPMStop(virQEMUDriver *driver, return; qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName); - qemuSecurityCleanupTPMEmulator(driver, vm); + if (!(outgoingMigration && qemuTPMHasSharedStorage(driver, vm->def) == 1)) + qemuSecurityCleanupTPMEmulator(driver, vm); } diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h index e6e32a0c4a..0e99cfb3e6 100644 --- a/src/qemu/qemu_tpm.h +++ b/src/qemu/qemu_tpm.h @@ -36,7 +36,8 @@ int qemuExtTPMPrepareHost(virQEMUDriver *driver, G_GNUC_WARN_UNUSED_RESULT; void qemuExtTPMCleanupHost(virDomainTPMDef *tpm, - virDomainUndefineFlagsValues flags) + virDomainUndefineFlagsValues flags, + bool outgoingMigration) ATTRIBUTE_NONNULL(1); int qemuExtTPMStart(virQEMUDriver *driver, @@ -48,7 +49,8 @@ int qemuExtTPMStart(virQEMUDriver *driver, G_GNUC_WARN_UNUSED_RESULT; void qemuExtTPMStop(virQEMUDriver *driver, - virDomainObj *vm) + virDomainObj *vm, + bool outgoingMigration) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuExtTPMSetupCgroup(virQEMUDriver *driver, -- 2.37.3
On 10/24/22 12:28, 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 existence of share storage influences the management of the directory structure holding the TPM state, which for example is only removed when a domain is undefined and not when a VM is removed on the migration source host. Further, when shared storage is used then 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 by NFS.
For shared storage support to work properly both sides of the migration need to be clients of the shared storage setup, meaning that they both have to have /var/lib/libvirt/swtpm mounted as shared storage, because other- wise the virFileIsSharedFS() may not detect shared storage and in the worst case may cause the TPM emulator (swtpm) to malfunction if for example the source side removed the TPM state directory structure.
Shared storage migration requires (upcoming) swtpm v0.8.
Stefan
v4: - Fixed long-standing bug regarding offline migration that now blocks if no shared storage is set up - Addressed Michal's concerns
v3: - Relying entirely on virFileIsSharedFS() on migration source and destination sides to detect whether shared storage is set up between hosts; no more hint about shared storage from user via flag - Added support for virDomainTPMPrivate structure to store and persist TPM-related private data
Stefan Berger (7): util: Add parsing support for swtpm's cmdarg-migration capability qemu: tpm: Allow offline migration with TPM_EMULATOR only with shared storage qemu: tpm: Conditionally create storage on incoming migration qemu: tpm: Add support for storing private TPM-related data qemu: tpm: Pass --migration option to swtpm if supported and needed qemu: tpm: Avoid security labels on incoming migration with shared storage qemu: tpm: Never remove state on outgoing migration and shared storage
src/conf/domain_conf.c | 63 ++++++++++++++++++-- src/conf/domain_conf.h | 9 +++ src/qemu/qemu_domain.c | 85 ++++++++++++++++++++++++-- src/qemu/qemu_domain.h | 17 +++++- src/qemu/qemu_driver.c | 20 +++---- src/qemu/qemu_extdevice.c | 10 ++-- src/qemu/qemu_extdevice.h | 6 +- src/qemu/qemu_migration.c | 28 +++++++-- src/qemu/qemu_process.c | 9 ++- src/qemu/qemu_snapshot.c | 4 +- src/qemu/qemu_tpm.c | 122 ++++++++++++++++++++++++++++++++++---- src/qemu/qemu_tpm.h | 14 ++++- src/util/virtpm.c | 1 + src/util/virtpm.h | 1 + 14 files changed, 339 insertions(+), 50 deletions(-)
Hey, sorry for late review. I just have to small nits. I surely want to avoid making you send another version. My suggestions are trivial (moving a few lines of code into a different function or dropping it) and as such I can squash them in before pushing. Tentatively-ACKed-by: me Michal
On 11/7/22 03:31, Michal Prívozník wrote:
On 10/24/22 12:28, 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 existence of share storage influences the management of the directory structure holding the TPM state, which for example is only removed when a domain is undefined and not when a VM is removed on the migration source host. Further, when shared storage is used then 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 by NFS.
For shared storage support to work properly both sides of the migration need to be clients of the shared storage setup, meaning that they both have to have /var/lib/libvirt/swtpm mounted as shared storage, because other- wise the virFileIsSharedFS() may not detect shared storage and in the worst case may cause the TPM emulator (swtpm) to malfunction if for example the source side removed the TPM state directory structure.
Shared storage migration requires (upcoming) swtpm v0.8.
Stefan
v4: - Fixed long-standing bug regarding offline migration that now blocks if no shared storage is set up - Addressed Michal's concerns
v3: - Relying entirely on virFileIsSharedFS() on migration source and destination sides to detect whether shared storage is set up between hosts; no more hint about shared storage from user via flag - Added support for virDomainTPMPrivate structure to store and persist TPM-related private data
Stefan Berger (7): util: Add parsing support for swtpm's cmdarg-migration capability qemu: tpm: Allow offline migration with TPM_EMULATOR only with shared storage qemu: tpm: Conditionally create storage on incoming migration qemu: tpm: Add support for storing private TPM-related data qemu: tpm: Pass --migration option to swtpm if supported and needed qemu: tpm: Avoid security labels on incoming migration with shared storage qemu: tpm: Never remove state on outgoing migration and shared storage
src/conf/domain_conf.c | 63 ++++++++++++++++++-- src/conf/domain_conf.h | 9 +++ src/qemu/qemu_domain.c | 85 ++++++++++++++++++++++++-- src/qemu/qemu_domain.h | 17 +++++- src/qemu/qemu_driver.c | 20 +++---- src/qemu/qemu_extdevice.c | 10 ++-- src/qemu/qemu_extdevice.h | 6 +- src/qemu/qemu_migration.c | 28 +++++++-- src/qemu/qemu_process.c | 9 ++- src/qemu/qemu_snapshot.c | 4 +- src/qemu/qemu_tpm.c | 122 ++++++++++++++++++++++++++++++++++---- src/qemu/qemu_tpm.h | 14 ++++- src/util/virtpm.c | 1 + src/util/virtpm.h | 1 + 14 files changed, 339 insertions(+), 50 deletions(-)
Hey, sorry for late review. I just have to small nits. I surely want to avoid making you send another version. My suggestions are trivial
How is it going to work without another version. I am fine with you making the changes but I can also send another version... Stefan
(moving a few lines of code into a different function or dropping it) and as such I can squash them in before pushing.
Tentatively-ACKed-by: me
Michal
On 11/8/22 15:10, Stefan Berger wrote:
On 11/7/22 03:31, Michal Prívozník wrote:
On 10/24/22 12:28, 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 existence of share storage influences the management of the directory structure holding the TPM state, which for example is only removed when a domain is undefined and not when a VM is removed on the migration source host. Further, when shared storage is used then 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 by NFS.
For shared storage support to work properly both sides of the migration need to be clients of the shared storage setup, meaning that they both have to have /var/lib/libvirt/swtpm mounted as shared storage, because other- wise the virFileIsSharedFS() may not detect shared storage and in the worst case may cause the TPM emulator (swtpm) to malfunction if for example the source side removed the TPM state directory structure.
Shared storage migration requires (upcoming) swtpm v0.8.
Stefan
v4: - Fixed long-standing bug regarding offline migration that now blocks if no shared storage is set up - Addressed Michal's concerns
v3: - Relying entirely on virFileIsSharedFS() on migration source and destination sides to detect whether shared storage is set up between hosts; no more hint about shared storage from user via flag - Added support for virDomainTPMPrivate structure to store and persist TPM-related private data
Stefan Berger (7): util: Add parsing support for swtpm's cmdarg-migration capability qemu: tpm: Allow offline migration with TPM_EMULATOR only with shared storage qemu: tpm: Conditionally create storage on incoming migration qemu: tpm: Add support for storing private TPM-related data qemu: tpm: Pass --migration option to swtpm if supported and needed qemu: tpm: Avoid security labels on incoming migration with shared storage qemu: tpm: Never remove state on outgoing migration and shared storage
src/conf/domain_conf.c | 63 ++++++++++++++++++-- src/conf/domain_conf.h | 9 +++ src/qemu/qemu_domain.c | 85 ++++++++++++++++++++++++-- src/qemu/qemu_domain.h | 17 +++++- src/qemu/qemu_driver.c | 20 +++---- src/qemu/qemu_extdevice.c | 10 ++-- src/qemu/qemu_extdevice.h | 6 +- src/qemu/qemu_migration.c | 28 +++++++-- src/qemu/qemu_process.c | 9 ++- src/qemu/qemu_snapshot.c | 4 +- src/qemu/qemu_tpm.c | 122 ++++++++++++++++++++++++++++++++++---- src/qemu/qemu_tpm.h | 14 ++++- src/util/virtpm.c | 1 + src/util/virtpm.h | 1 + 14 files changed, 339 insertions(+), 50 deletions(-)
Hey, sorry for late review. I just have to small nits. I surely want to avoid making you send another version. My suggestions are trivial
How is it going to work without another version. I am fine with you making the changes but I can also send another version...
I'll do the changes just before pushing. Fortunately, we are not doing merge requests where this would be more complicated. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Thank you for your patience. BTW: do you have any plans of making a release of swtpm in near future? I think it may be worth it so that distros can pick up this new feature. Michal
On 11/9/22 06:38, Michal Prívozník wrote:
On 11/8/22 15:10, Stefan Berger wrote:
Hey, sorry for late review. I just have to small nits. I surely want to avoid making you send another version. My suggestions are trivial
How is it going to work without another version. I am fine with you making the changes but I can also send another version...
I'll do the changes just before pushing. Fortunately, we are not doing merge requests where this would be more complicated.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
and pushed. Thank you for your patience.
BTW: do you have any plans of making a release of swtpm in near future? I think it may be worth it so that distros can pick up this new feature.
I was going to tag it with v0.8.0 tomorrow or on Friday. I supposed you tried the shared storage migration a bit as well? Stefan
Michal
On 11/9/22 12:59, Stefan Berger wrote:
On 11/9/22 06:38, Michal Prívozník wrote:
On 11/8/22 15:10, Stefan Berger wrote:
Hey, sorry for late review. I just have to small nits. I surely want to avoid making you send another version. My suggestions are trivial
How is it going to work without another version. I am fine with you making the changes but I can also send another version...
I'll do the changes just before pushing. Fortunately, we are not doing merge requests where this would be more complicated.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
and pushed. Thank you for your patience.
BTW: do you have any plans of making a release of swtpm in near future? I think it may be worth it so that distros can pick up this new feature.
I was going to tag it with v0.8.0 tomorrow or on Friday.
Perfect!
I supposed you tried the shared storage migration a bit as well?
Yes, I grabbed master and build swtpm from it. Worked in my testing. Michal
participants (2)
-
Michal Prívozník -
Stefan Berger