12:04 p.m.
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
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 (6):
util: Add parsing support for swtpm's cmdarg-migration capability
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 | 20 ++++---
src/qemu/qemu_process.c | 9 ++--
src/qemu/qemu_snapshot.c | 4 +-
src/qemu/qemu_tpm.c | 111 ++++++++++++++++++++++++++++++++++----
src/qemu/qemu_tpm.h | 12 ++++-
src/util/virtpm.c | 1 +
src/util/virtpm.h | 1 +
14 files changed, 318 insertions(+), 50 deletions(-)
--
2.37.3
12:04 p.m.
New subject: [PATCH v3 1/6] util: Add parsing support for swtpm's cmdarg-migration capability
Add support for parsing swtpm 'cmdarg-migration' capability (since v0.8).
Signed-off-by: Stefan Berger <stefanb(a)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
12:04 p.m.
New subject: [PATCH v3 2/6] qemu: tpm: Conditionally create storage on incoming migration
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(a)linux.ibm.com>
---
src/qemu/qemu_tpm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index dc09c94a4d..a45ad599aa 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -556,11 +556,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 &&
virFileIsSharedFS(tpm->data.emulator.storagepath))
+ create_storage = false;
+
+ if (create_storage &&
+ qemuTPMEmulatorCreateStorage(tpm, &created, swtpm_user, swtpm_group) < 0)
return NULL;
if (tpm->data.emulator.hassecretuuid)
--
2.37.3
5:55 a.m.
New subject: [PATCH v3 2/6] qemu: tpm: Conditionally create storage on incoming migration
On 10/18/22 19:04, Stefan Berger wrote:
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(a)linux.ibm.com>
---
src/qemu/qemu_tpm.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index dc09c94a4d..a45ad599aa 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -556,11 +556,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 &&
virFileIsSharedFS(tpm->data.emulator.storagepath))
Here and everywhere else, this needs to be virFileIsSharedFS() == 1,
because the function may return -1, 0, 1 and we do not want to treat -1
as 1.
+ create_storage = false;
+
+ if (create_storage &&
+ qemuTPMEmulatorCreateStorage(tpm, &created, swtpm_user, swtpm_group) <
0)
return NULL;
if (tpm->data.emulator.hassecretuuid)
Michal
8:23 a.m.
New subject: [PATCH v3 2/6] qemu: tpm: Conditionally create storage on incoming migration
On 10/21/22 06:55, Michal Prívozník wrote:
On 10/18/22 19:04, Stefan Berger wrote:
> 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(a)linux.ibm.com>
> ---
> src/qemu/qemu_tpm.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index dc09c94a4d..a45ad599aa 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -556,11 +556,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 &&
virFileIsSharedFS(tpm->data.emulator.storagepath))
Here and everywhere else, this needs to be virFileIsSharedFS() == 1,
because the function may return -1, 0, 1 and we do not want to treat -1
as 1.
Thanks, I will fix it.
> + create_storage = false;
> +
> + if (create_storage &&
> + qemuTPMEmulatorCreateStorage(tpm, &created, swtpm_user, swtpm_group)
< 0)
> return NULL;
>
> if (tpm->data.emulator.hassecretuuid)
Michal
Stefan
12:04 p.m.
New subject: [PATCH v3 3/6] qemu: tpm: Add support for storing private TPM-related data
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(a)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 4c14fc2aef..97c62e2c9e 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
@@ -3203,6 +3273,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 a22deaf113..e7d3e1be40 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -407,6 +407,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
5:55 a.m.
New subject: [PATCH v3 3/6] qemu: tpm: Add support for storing private TPM-related data
On 10/18/22 19:04, Stefan Berger wrote:
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(a)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(-)
I know I've pointed you in this direction, but now that I'm seeing the
actual I code I wonder whether we can do without private data. See my
other replies.
Michal
12:04 p.m.
New subject: [PATCH v3 4/6] qemu: tpm: Pass --migration option to swtpm if supported and needed
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(a)linux.ibm.com>
---
src/qemu/qemu_migration.c | 8 +++++
src/qemu/qemu_tpm.c | 66 ++++++++++++++++++++++++++++++++++++++-
src/qemu/qemu_tpm.h | 6 ++++
3 files changed, 79 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 33105cf07b..5b4f4615ee 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"
@@ -2789,6 +2790,13 @@ qemuMigrationSrcBegin(virConnectPtr conn,
goto cleanup;
}
+ if (qemuTPMHasSharedStorage(vm->def) &&
+ !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 a45ad599aa..7b0afe94ec 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,7 +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))
+ on_shared_storage = virFileIsSharedFS(tpm->data.emulator.storagepath);
+ if (incomingMigration && on_shared_storage)
create_storage = false;
if (create_storage &&
@@ -642,6 +644,31 @@ 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:
@@ -962,6 +989,43 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
}
+bool
+qemuTPMHasSharedStorage(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 virFileIsSharedFS(tpm->data.emulator.storagepath);
+ case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
+ case VIR_DOMAIN_TPM_TYPE_LAST:
+ }
+ }
+
+ return false;
+}
+
+
+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 f068f3ca5a..9daa3e14df 100644
--- a/src/qemu/qemu_tpm.h
+++ b/src/qemu/qemu_tpm.h
@@ -56,3 +56,9 @@ int qemuExtTPMSetupCgroup(virQEMUDriver *driver,
virCgroup *cgroup)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
G_GNUC_WARN_UNUSED_RESULT;
+
+bool qemuTPMHasSharedStorage(virDomainDef *def)
+ ATTRIBUTE_NONNULL(1);
+
+bool qemuTPMCanMigrateSharedStorage(virDomainDef *def)
+ ATTRIBUTE_NONNULL(1);
--
2.37.3
5:55 a.m.
New subject: [PATCH v3 4/6] qemu: tpm: Pass --migration option to swtpm if supported and needed
On 10/18/22 19:04, 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(a)linux.ibm.com>
---
src/qemu/qemu_migration.c | 8 +++++
src/qemu/qemu_tpm.c | 66 ++++++++++++++++++++++++++++++++++++++-
src/qemu/qemu_tpm.h | 6 ++++
3 files changed, 79 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 33105cf07b..5b4f4615ee 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"
@@ -2789,6 +2790,13 @@ qemuMigrationSrcBegin(virConnectPtr conn,
goto cleanup;
}
+ if (qemuTPMHasSharedStorage(vm->def) &&
+ !qemuTPMCanMigrateSharedStorage(vm->def)) {
This works, but only because qemuValidateDomainDefTPMs() denies two
emulated TPMs. I think we can call just qemuTPMCanMigrateSharedStorage()
since it already iterates over all TPMs and checks each one individually.
+ 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 a45ad599aa..7b0afe94ec 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,7 +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))
+ on_shared_storage = virFileIsSharedFS(tpm->data.emulator.storagepath);
+ if (incomingMigration && on_shared_storage)
create_storage = false;
if (create_storage &&
@@ -642,6 +644,31 @@ 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 we don't introduce private data, then this line should be deleted.
+ 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"),
Don't hesitate to put whole error message onto one line. It's way easier
to grep then.
+ swtpm);
+ goto error;
+ }
+ }
+
return g_steal_pointer(&cmd);
error:
@@ -962,6 +989,43 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
}
+bool
+qemuTPMHasSharedStorage(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 virFileIsSharedFS(tpm->data.emulator.storagepath);
+ case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
+ case VIR_DOMAIN_TPM_TYPE_LAST:
+ }
+ }
+
+ return false;
+}
+
+
+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;
If we don't introduce private data, then how about just:
if (virFileIsSharedFS(tpm->data.emulator.storagepath) == 1 &&
!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_MIGRATION))
return false;
+ 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 f068f3ca5a..9daa3e14df 100644
--- a/src/qemu/qemu_tpm.h
+++ b/src/qemu/qemu_tpm.h
@@ -56,3 +56,9 @@ int qemuExtTPMSetupCgroup(virQEMUDriver *driver,
virCgroup *cgroup)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
G_GNUC_WARN_UNUSED_RESULT;
+
+bool qemuTPMHasSharedStorage(virDomainDef *def)
+ ATTRIBUTE_NONNULL(1);
+
+bool qemuTPMCanMigrateSharedStorage(virDomainDef *def)
+ ATTRIBUTE_NONNULL(1);
Michal
6:36 a.m.
New subject: [PATCH v3 4/6] qemu: tpm: Pass --migration option to swtpm if supported and needed
On 10/21/22 06:55, Michal Prívozník wrote:
On 10/18/22 19:04, 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(a)linux.ibm.com>
> ---
> src/qemu/qemu_migration.c | 8 +++++
> src/qemu/qemu_tpm.c | 66 ++++++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_tpm.h | 6 ++++
> 3 files changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 33105cf07b..5b4f4615ee 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"
> @@ -2789,6 +2790,13 @@ qemuMigrationSrcBegin(virConnectPtr conn,
> goto cleanup;
> }
>
> + if (qemuTPMHasSharedStorage(vm->def) &&
> + !qemuTPMCanMigrateSharedStorage(vm->def)) {
This works, but only because qemuValidateDomainDefTPMs() denies two
emulated TPMs. I think we can call just qemuTPMCanMigrateSharedStorage()
since it already iterates over all TPMs and checks each one individually.
I am only starting swtpm with the necessary command line options for shared storage
migration if a) shared storage is detected at the time of the start of the VM and b) if
swtpm supports shared storage migration at all. If it doesn't support it I let you
start the VM but won't let you migrate.
qemuTPMCanMigrateSharedStorage() checks whether the running swtpm instance can do b). It
doesn't check the swtpm executable that you may have upgraded or downgraded in the
meantime. That's why I need the private data.
So the check above tests whether there is shared storage detected for the TPM_EMULATOR
(swtpm; only one allowed) and if so whether the *running swtpm instance* supports shared
storage migration. I think it is correct this way.
> + 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 a45ad599aa..7b0afe94ec 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,7 +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))
> + on_shared_storage = virFileIsSharedFS(tpm->data.emulator.storagepath);
> + if (incomingMigration && on_shared_storage)
> create_storage = false;
>
> if (create_storage &&
> @@ -642,6 +644,31 @@ 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 we don't introduce private data, then this line should be deleted.
> + 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"),
Don't hesitate to put whole error message onto one line. It's way easier
to grep then.
Yes, will change it
> + swtpm);
> + goto error;
> + }
> + }
> +
> return g_steal_pointer(&cmd);
>
> error:
> @@ -962,6 +989,43 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
> }
>
>
> +bool
> +qemuTPMHasSharedStorage(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 virFileIsSharedFS(tpm->data.emulator.storagepath);
> + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> + case VIR_DOMAIN_TPM_TYPE_LAST:
> + }
> + }
> +
> + return false;
> +}
> +
> +
> +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;
If we don't introduce private data, then how about just:
if (virFileIsSharedFS(tpm->data.emulator.storagepath) == 1 &&
!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_MIGRATION))
return false;
I'd rather not test the swtpm executable that may have been upgraded in the meantime
but go for checking the running swtpm instance...
Stefan
> + 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 f068f3ca5a..9daa3e14df 100644
> --- a/src/qemu/qemu_tpm.h
> +++ b/src/qemu/qemu_tpm.h
> @@ -56,3 +56,9 @@ int qemuExtTPMSetupCgroup(virQEMUDriver *driver,
> virCgroup *cgroup)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> G_GNUC_WARN_UNUSED_RESULT;
> +
> +bool qemuTPMHasSharedStorage(virDomainDef *def)
> + ATTRIBUTE_NONNULL(1);
> +
> +bool qemuTPMCanMigrateSharedStorage(virDomainDef *def)
> + ATTRIBUTE_NONNULL(1);
Michal
8:31 a.m.
New subject: [PATCH v3 4/6] qemu: tpm: Pass --migration option to swtpm if supported and needed
On 10/21/22 13:36, Stefan Berger wrote:
On 10/21/22 06:55, Michal Prívozník wrote:
> On 10/18/22 19:04, 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(a)linux.ibm.com>
>> ---
>> src/qemu/qemu_migration.c | 8 +++++
>> src/qemu/qemu_tpm.c | 66 ++++++++++++++++++++++++++++++++++++++-
>> src/qemu/qemu_tpm.h | 6 ++++
>> 3 files changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 33105cf07b..5b4f4615ee 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"
>> @@ -2789,6 +2790,13 @@ qemuMigrationSrcBegin(virConnectPtr conn,
>> goto cleanup;
>> }
>> + if (qemuTPMHasSharedStorage(vm->def) &&
>> + !qemuTPMCanMigrateSharedStorage(vm->def)) {
>
> This works, but only because qemuValidateDomainDefTPMs() denies two
> emulated TPMs. I think we can call just qemuTPMCanMigrateSharedStorage()
> since it already iterates over all TPMs and checks each one individually.
I am only starting swtpm with the necessary command line options for
shared storage migration if a) shared storage is detected at the time of
the start of the VM and b) if swtpm supports shared storage migration at
all. If it doesn't support it I let you start the VM but won't let you
migrate.
qemuTPMCanMigrateSharedStorage() checks whether the running swtpm
instance can do b). It doesn't check the swtpm executable that you may
have upgraded or downgraded in the meantime. That's why I need the
private data.
So the check above tests whether there is shared storage detected for
the TPM_EMULATOR (swtpm; only one allowed) and if so whether the
*running swtpm instance* supports shared storage migration. I think it
is correct this way.
Ah, good point. So let me fix the other small nits I've raised and merge
these.
Michal
9:57 a.m.
New subject: [PATCH v3 4/6] qemu: tpm: Pass --migration option to swtpm if supported and needed
On 10/21/22 15:31, Michal Prívozník wrote:
On 10/21/22 13:36, Stefan Berger wrote:
>
>
> On 10/21/22 06:55, Michal Prívozník wrote:
>> On 10/18/22 19:04, 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(a)linux.ibm.com>
>>> ---
>>> src/qemu/qemu_migration.c | 8 +++++
>>> src/qemu/qemu_tpm.c | 66 ++++++++++++++++++++++++++++++++++++++-
>>> src/qemu/qemu_tpm.h | 6 ++++
>>> 3 files changed, 79 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>>> index 33105cf07b..5b4f4615ee 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"
>>> @@ -2789,6 +2790,13 @@ qemuMigrationSrcBegin(virConnectPtr conn,
>>> goto cleanup;
>>> }
>>> + if (qemuTPMHasSharedStorage(vm->def) &&
>>> + !qemuTPMCanMigrateSharedStorage(vm->def)) {
>>
>> This works, but only because qemuValidateDomainDefTPMs() denies two
>> emulated TPMs. I think we can call just qemuTPMCanMigrateSharedStorage()
>> since it already iterates over all TPMs and checks each one individually.
>
> I am only starting swtpm with the necessary command line options for
> shared storage migration if a) shared storage is detected at the time of
> the start of the VM and b) if swtpm supports shared storage migration at
> all. If it doesn't support it I let you start the VM but won't let you
> migrate.
>
>
> qemuTPMCanMigrateSharedStorage() checks whether the running swtpm
> instance can do b). It doesn't check the swtpm executable that you may
> have upgraded or downgraded in the meantime. That's why I need the
> private data.
>
> So the check above tests whether there is shared storage detected for
> the TPM_EMULATOR (swtpm; only one allowed) and if so whether the
> *running swtpm instance* supports shared storage migration. I think it
> is correct this way.
Ah, good point. So let me fix the other small nits I've raised and merge
these.
Actually, I'll investigate whether we need the very last patch. I mean,
we definitely do not want to remove the swtpm state from the source, BUT
I wonder whether we can set @flags passed to
qemuDomainRemoveInactiveCommon() (e.g. to VIR_DOMAIN_UNDEFINE_KEEP_TPM)
instead of introducing another argument with basically the same
functionality.
Michal
12:04 p.m.
New subject: [PATCH v3 5/6] qemu: tpm: Avoid security labels on incoming migration with shared storage
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(a)linux.ibm.com>
---
src/qemu/qemu_tpm.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 7b0afe94ec..69410e36ff 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -933,10 +933,18 @@ 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)) {
+ /* 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
12:04 p.m.
New subject: [PATCH v3 6/6] qemu: tpm: Never remove state on outgoing migration and shared storage
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(a)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 | 21 ++++++++++++++++-----
src/qemu/qemu_tpm.h | 6 ++++--
10 files changed, 63 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 97c62e2c9e..20cc2409fc 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7245,7 +7245,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;
@@ -7271,7 +7272,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);
}
@@ -7283,14 +7284,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);
}
@@ -7312,7 +7314,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 e7d3e1be40..11ea52c32d 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -696,7 +696,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 5c75000742..017cda2a9c 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);
}
@@ -4053,7 +4053,7 @@ processMonitorEOFEvent(virQEMUDriver *driver,
virObjectEventStateQueue(driver->domainEventState, event);
endjob:
- qemuDomainRemoveInactive(driver, vm, 0);
+ qemuDomainRemoveInactive(driver, vm, 0, false);
virDomainObjEndJob(vm);
}
@@ -5985,7 +5985,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;
}
@@ -6675,7 +6675,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);
}
}
@@ -6824,7 +6824,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 5b4f4615ee..990c7e6829 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3399,7 +3399,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);
@@ -4044,7 +4044,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:
@@ -6047,7 +6047,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);
@@ -6174,7 +6174,7 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver,
}
if (!virDomainObjIsActive(vm))
- qemuDomainRemoveInactive(driver, vm, 0);
+ qemuDomainRemoveInactive(driver, vm, 0, true);
return ret;
}
@@ -6710,7 +6710,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;
@@ -6847,7 +6847,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 1a9175f40f..26b2edb05f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8073,6 +8073,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",
@@ -8170,7 +8171,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);
@@ -8436,7 +8439,7 @@ qemuProcessAutoDestroy(virDomainObj *dom,
VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
- qemuDomainRemoveInactive(driver, dom, 0);
+ qemuDomainRemoveInactive(driver, dom, 0, false);
virDomainObjEndJob(dom);
@@ -8899,7 +8902,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 69410e36ff..f7d1487111 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -729,13 +729,21 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm,
* qemuTPMEmulatorCleanupHost:
* @tpm: TPM definition
* @flags: flags indicating whether to keep or remove TPM persistent state
+ * @outgoingMigration: whether cleanup 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))
+ return;
+
/*
* remove TPM state if:
* - persistent_state flag is set and the UNDEFINE_TPM flag is set
@@ -1081,9 +1089,10 @@ qemuExtTPMPrepareHost(virQEMUDriver *driver,
void
qemuExtTPMCleanupHost(virDomainTPMDef *tpm,
- virDomainUndefineFlagsValues flags)
+ virDomainUndefineFlagsValues flags,
+ bool outgoingMigration)
{
- qemuTPMEmulatorCleanupHost(tpm, flags);
+ qemuTPMEmulatorCleanupHost(tpm, flags, outgoingMigration);
}
@@ -1104,7 +1113,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);
@@ -1113,7 +1123,8 @@ qemuExtTPMStop(virQEMUDriver *driver,
return;
qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName);
- qemuSecurityCleanupTPMEmulator(driver, vm);
+ if (!(outgoingMigration && qemuTPMHasSharedStorage(vm->def)))
+ qemuSecurityCleanupTPMEmulator(driver, vm);
}
diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h
index 9daa3e14df..53ff51f1d0 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
5:55 a.m.
On 10/18/22 19:04, 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
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 (6):
util: Add parsing support for swtpm's cmdarg-migration capability
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 | 20 ++++---
src/qemu/qemu_process.c | 9 ++--
src/qemu/qemu_snapshot.c | 4 +-
src/qemu/qemu_tpm.c | 111 ++++++++++++++++++++++++++++++++++----
src/qemu/qemu_tpm.h | 12 ++++-
src/util/virtpm.c | 1 +
src/util/virtpm.h | 1 +
14 files changed, 318 insertions(+), 50 deletions(-)
Patches look good. I did an experiment and tried to rework the code that
patch 3/6 is not needed. You can find it here:
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/tpm_migration_rework
If you don't like it, I can merge your patches as they are. Or, let me
know if you're okay with my changes. No need to resend either way.
Michal
10:27 a.m.
On 10/21/22 06:55, Michal Prívozník wrote:
On 10/18/22 19:04, 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
>
> 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 (6):
> util: Add parsing support for swtpm's cmdarg-migration capability
> 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 | 20 ++++---
> src/qemu/qemu_process.c | 9 ++--
> src/qemu/qemu_snapshot.c | 4 +-
> src/qemu/qemu_tpm.c | 111 ++++++++++++++++++++++++++++++++++----
> src/qemu/qemu_tpm.h | 12 ++++-
> src/util/virtpm.c | 1 +
> src/util/virtpm.h | 1 +
> 14 files changed, 318 insertions(+), 50 deletions(-)
>
Patches look good. I did an experiment and tried to rework the code that
patch 3/6 is not needed. You can find it here:
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/tpm_migration_rework
If you don't like it, I can merge your patches as they are. Or, let me
know if you're okay with my changes. No need to resend either way.
I think we need a v4. I have mine here:
https://github.com/stefanberger/libvirt-tpm/tree/swtpm_shared_storage.v4
Michal
3:55 a.m.
On 10/21/22 17:27, Stefan Berger wrote:
On 10/21/22 06:55, Michal Prívozník wrote:
> On 10/18/22 19:04, 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
>>
>> 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 (6):
>> util: Add parsing support for swtpm's cmdarg-migration capability
>> 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 | 20 ++++---
>> src/qemu/qemu_process.c | 9 ++--
>> src/qemu/qemu_snapshot.c | 4 +-
>> src/qemu/qemu_tpm.c | 111 ++++++++++++++++++++++++++++++++++----
>> src/qemu/qemu_tpm.h | 12 ++++-
>> src/util/virtpm.c | 1 +
>> src/util/virtpm.h | 1 +
>> 14 files changed, 318 insertions(+), 50 deletions(-)
>>
>
> Patches look good. I did an experiment and tried to rework the code that
> patch 3/6 is not needed. You can find it here:
>
> https://gitlab.com/MichalPrivoznik/libvirt/-/commits/tpm_migration_rework
>
> If you don't like it, I can merge your patches as they are. Or, let me
> know if you're okay with my changes. No need to resend either way.
I think we need a v4. I have mine here:
https://github.com/stefanberger/libvirt-tpm/tree/swtpm_shared_storage.v4
Yep. Also I realized that we need that extra argument to
qemuDomainRemoveInactive(), because in some cases we want to pass
VIR_DOMAIN_UNDEFINE_TPM flag, and yet not remove any TPM state if it's
on a shared storage.
Alright, I've uploaded my fixup commits here:
https://gitlab.com/MichalPrivoznik/libvirt/-/tree/tpm_migration_rework
Can you work in those fixup commits and resend?
Michal
763
days inactive
769
days old
16 comments
2 participants
participants (2)
-
Michal Prívozník -
Stefan Berger