10:41 a.m.
This is needed when migrating a guest that has persistent TPM
state: relabeling (which implies locking) needs to happen
before the swtpm process is started on the destination host,
but the lock file won't be released by the swtpm process
running on the source host before a handshake with the target
process has happened, creating a catch-22 scenario.
In order to make migration possible, make it so that locking
for lock files can be explicitly skipped. All other state
files are handled as usual.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_security.c | 56 ++++++++++++++++++++++-----------
src/security/security_dac.c | 12 +++++--
src/security/security_driver.h | 3 +-
src/security/security_manager.c | 21 +++++++++++--
src/security/security_manager.h | 6 ++--
src/security/security_selinux.c | 12 +++++--
src/security/security_stack.c | 6 ++--
7 files changed, 83 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 996c95acc0..5e815ba2a0 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -57,7 +57,8 @@ qemuSecuritySetAllLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -94,7 +95,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriver *driver,
if (transactionStarted &&
virSecurityManagerTransactionCommit(driver->securityManager,
- -1, priv->rememberOwner) < 0)
+ -1, priv->rememberOwner,
+ false) < 0)
VIR_WARN("Unable to run security manager transaction");
virSecurityManagerTransactionAbort(driver->securityManager);
@@ -133,7 +135,8 @@ qemuSecuritySetImageLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -171,7 +174,8 @@ qemuSecurityRestoreImageLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -227,7 +231,8 @@ qemuSecuritySetHostdevLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -261,7 +266,8 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -294,7 +300,8 @@ qemuSecuritySetMemoryLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -327,7 +334,8 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -360,7 +368,8 @@ qemuSecuritySetInputLabel(virDomainObj *vm,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -393,7 +402,8 @@ qemuSecurityRestoreInputLabel(virDomainObj *vm,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -427,7 +437,8 @@ qemuSecuritySetChardevLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -461,7 +472,8 @@ qemuSecurityRestoreChardevLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -492,7 +504,8 @@ qemuSecuritySetNetdevLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -524,7 +537,8 @@ qemuSecurityRestoreNetdevLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -552,7 +566,8 @@ qemuSecuritySetTPMLabels(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- -1, priv->rememberOwner) < 0)
+ -1, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -580,7 +595,8 @@ qemuSecurityRestoreTPMLabels(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- -1, priv->rememberOwner) < 0)
+ -1, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -615,7 +631,8 @@ qemuSecurityDomainSetPathLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -648,7 +665,8 @@ qemuSecurityDomainRestorePathLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- pid, priv->rememberOwner) < 0)
+ pid, priv->rememberOwner,
+ false) < 0)
goto cleanup;
ret = 0;
@@ -694,7 +712,7 @@ qemuSecurityDomainSetMountNSPathLabel(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- vm->pid, false) < 0)
+ vm->pid, false, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index fdc11876c9..a179378a78 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -83,6 +83,7 @@ struct _virSecurityDACChownList {
virSecurityDACChownItem **items;
size_t nItems;
bool lock;
+ bool lockMetadataException;
};
@@ -232,7 +233,8 @@ virSecurityDACTransactionRun(pid_t pid G_GNUC_UNUSED,
if (!(state = virSecurityManagerMetadataLock(list->manager,
list->sharedFilesystems,
- paths, npaths)))
+ paths, npaths,
+ list->lockMetadataException)))
return -1;
for (i = 0; i < list->nItems; i++) {
@@ -580,6 +582,7 @@ virSecurityDACTransactionStart(virSecurityManager *mgr,
* @mgr: security manager
* @pid: domain's PID
* @lock: lock and unlock paths that are relabeled
+ * @lockMetadataException: don't lock metadata for lock files
*
* If @pid is not -1 then enter the @pid namespace (usually @pid refers
* to a domain) and perform all the chown()-s on the list. If @pid is -1
@@ -599,7 +602,8 @@ virSecurityDACTransactionStart(virSecurityManager *mgr,
static int
virSecurityDACTransactionCommit(virSecurityManager *mgr G_GNUC_UNUSED,
pid_t pid,
- bool lock)
+ bool lock,
+ bool lockMetadataException)
{
g_autoptr(virSecurityDACChownList) list = NULL;
int rc;
@@ -618,6 +622,7 @@ virSecurityDACTransactionCommit(virSecurityManager *mgr
G_GNUC_UNUSED,
}
list->lock = lock;
+ list->lockMetadataException = lockMetadataException;
if (pid != -1) {
rc = virProcessRunInMountNamespace(pid,
@@ -1084,7 +1089,8 @@ virSecurityDACMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED,
if (!(state = virSecurityManagerMetadataLock(data->mgr,
data->sharedFilesystems,
- paths, G_N_ELEMENTS(paths))))
+ paths, G_N_ELEMENTS(paths),
+ false)))
return -1;
ret = virSecurityMoveRememberedLabel(SECURITY_DAC_NAME, data->src, data->dst);
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 2956e002ff..5ab4d6ca1e 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -50,7 +50,8 @@ typedef int (*virSecurityDriverTransactionStart) (virSecurityManager
*mgr,
char *const *sharedFilesystems);
typedef int (*virSecurityDriverTransactionCommit) (virSecurityManager *mgr,
pid_t pid,
- bool lock);
+ bool lock,
+ bool lockMetadataException);
typedef void (*virSecurityDriverTransactionAbort) (virSecurityManager *mgr);
typedef int (*virSecurityDomainSetDaemonSocketLabel)(virSecurityManager *mgr,
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 65b173e670..c2460eae37 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -31,6 +31,7 @@
#include "virobject.h"
#include "virlog.h"
#include "virfile.h"
+#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
@@ -270,6 +271,7 @@ virSecurityManagerTransactionStart(virSecurityManager *mgr,
* @mgr: security manager
* @pid: domain's PID
* @lock: lock and unlock paths that are relabeled
+ * @lockMetadataException: don't lock metadata for lock files
*
* If @pid is not -1 then enter the @pid namespace (usually @pid refers
* to a domain) and perform all the operations on the transaction list.
@@ -290,14 +292,15 @@ virSecurityManagerTransactionStart(virSecurityManager *mgr,
int
virSecurityManagerTransactionCommit(virSecurityManager *mgr,
pid_t pid,
- bool lock)
+ bool lock,
+ bool lockMetadataException)
{
VIR_LOCK_GUARD lockguard = virObjectLockGuard(mgr);
if (!mgr->drv->transactionCommit)
return 0;
- return mgr->drv->transactionCommit(mgr, pid, lock);
+ return mgr->drv->transactionCommit(mgr, pid, lock, lockMetadataException);
}
@@ -1310,6 +1313,7 @@ cmpstringp(const void *p1,
* @sharedFilesystems: list of filesystem to consider shared
* @paths: paths to lock
* @npaths: number of items in @paths array
+ * @lockMetadataException: don't lock metadata for lock files
*
* Lock passed @paths for metadata change. The returned state
* should be passed to virSecurityManagerMetadataUnlock.
@@ -1325,7 +1329,8 @@ virSecurityManagerMetadataLockState *
virSecurityManagerMetadataLock(virSecurityManager *mgr G_GNUC_UNUSED,
char *const *sharedFilesystems,
const char **paths,
- size_t npaths)
+ size_t npaths,
+ bool lockMetadataException)
{
size_t i = 0;
size_t nfds = 0;
@@ -1371,6 +1376,16 @@ virSecurityManagerMetadataLock(virSecurityManager *mgr
G_GNUC_UNUSED,
if (i != j)
continue;
+ /* Any attempt to lock a lock file is likely to go very
+ * poorly. This is a problem for TPM persistent storage,
+ * since on migration we need to relabel it while the swtpm
+ * process is still holding on to its lock file. As a way to
+ * resolve the conundrum, skip metadata locking for paths
+ * that look like they might be referring to lock files, if
+ * we have also been explicitly asked to make this exception */
+ if (lockMetadataException && virStringHasSuffix(p, ".lock"))
+ continue;
+
if (stat(p, &sb) < 0)
continue;
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index bf0059b2e0..068ca4e290 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -85,7 +85,8 @@ int virSecurityManagerTransactionStart(virSecurityManager *mgr,
char *const *sharedFilesystems);
int virSecurityManagerTransactionCommit(virSecurityManager *mgr,
pid_t pid,
- bool lock);
+ bool lock,
+ bool lockMetadataException);
void virSecurityManagerTransactionAbort(virSecurityManager *mgr);
void *virSecurityManagerGetPrivateData(virSecurityManager *mgr);
@@ -254,7 +255,8 @@ virSecurityManagerMetadataLockState *
virSecurityManagerMetadataLock(virSecurityManager *mgr,
char *const *sharedFilesystems,
const char **paths,
- size_t npaths);
+ size_t npaths,
+ bool lockMetadataException);
void
virSecurityManagerMetadataUnlock(virSecurityManager *mgr,
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 05f54cc9f5..8f567d5488 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -81,6 +81,7 @@ struct _virSecuritySELinuxContextList {
virSecuritySELinuxContextItem **items;
size_t nItems;
bool lock;
+ bool lockMetadataException;
};
#define SECURITY_SELINUX_VOID_DOI "0"
@@ -300,7 +301,8 @@ virSecuritySELinuxTransactionRun(pid_t pid G_GNUC_UNUSED,
if (!(state = virSecurityManagerMetadataLock(list->manager,
list->sharedFilesystems,
- paths, npaths)))
+ paths, npaths,
+ list->lockMetadataException)))
goto cleanup;
for (i = 0; i < list->nItems; i++) {
@@ -1193,6 +1195,7 @@ virSecuritySELinuxTransactionStart(virSecurityManager *mgr,
* @mgr: security manager
* @pid: domain's PID
* @lock: lock and unlock paths that are relabeled
+ * @lockMetadataException: don't lock metadata for lock files
*
* If @pis is not -1 then enter the @pid namespace (usually @pid refers
* to a domain) and perform all the sefilecon()-s on the list. If @pid
@@ -1213,7 +1216,8 @@ virSecuritySELinuxTransactionStart(virSecurityManager *mgr,
static int
virSecuritySELinuxTransactionCommit(virSecurityManager *mgr G_GNUC_UNUSED,
pid_t pid,
- bool lock)
+ bool lock,
+ bool lockMetadataException)
{
virSecuritySELinuxContextList *list;
int rc;
@@ -1233,6 +1237,7 @@ virSecuritySELinuxTransactionCommit(virSecurityManager *mgr
G_GNUC_UNUSED,
}
list->lock = lock;
+ list->lockMetadataException = lockMetadataException;
if (pid != -1) {
rc = virProcessRunInMountNamespace(pid,
@@ -2091,7 +2096,8 @@ virSecuritySELinuxMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED,
if (!(state = virSecurityManagerMetadataLock(data->mgr,
data->sharedFilesystems,
- paths, G_N_ELEMENTS(paths))))
+ paths, G_N_ELEMENTS(paths),
+ false)))
return -1;
ret = virSecurityMoveRememberedLabel(SECURITY_SELINUX_NAME, data->src,
data->dst);
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 11800535b9..99a68a6053 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -164,13 +164,15 @@ virSecurityStackTransactionStart(virSecurityManager *mgr,
static int
virSecurityStackTransactionCommit(virSecurityManager *mgr,
pid_t pid,
- bool lock)
+ bool lock,
+ bool lockMetadataException)
{
virSecurityStackData *priv = virSecurityManagerGetPrivateData(mgr);
virSecurityStackItem *item = priv->itemsHead;
for (; item; item = item->next) {
- if (virSecurityManagerTransactionCommit(item->securityManager, pid, lock) <
0)
+ if (virSecurityManagerTransactionCommit(item->securityManager, pid,
+ lock, lockMetadataException) < 0)
goto rollback;
}
--
2.46.2