We will never be able to acquire the lock on the destination
host because the swtpm process that's running on the source
host is still holding on to it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/qemu/qemu_security.c | 10 ++++++----
src/qemu/qemu_security.h | 6 ++++--
src/qemu/qemu_tpm.c | 20 +++++++++++++++++---
3 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 996c95acc0..11af9100fb 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -537,7 +537,8 @@ qemuSecurityRestoreNetdevLabel(virQEMUDriver *driver,
int
qemuSecuritySetTPMLabels(virQEMUDriver *driver,
virDomainObj *vm,
- bool setTPMStateLabel)
+ bool setTPMStateLabel,
+ bool attemptLocking)
{
qemuDomainObjPrivate *priv = vm->privateData;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
@@ -552,7 +553,7 @@ qemuSecuritySetTPMLabels(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- -1, priv->rememberOwner) < 0)
+ -1, priv->rememberOwner && attemptLocking) < 0)
goto cleanup;
ret = 0;
@@ -565,7 +566,8 @@ qemuSecuritySetTPMLabels(virQEMUDriver *driver,
int
qemuSecurityRestoreTPMLabels(virQEMUDriver *driver,
virDomainObj *vm,
- bool restoreTPMStateLabel)
+ bool restoreTPMStateLabel,
+ bool attemptLocking)
{
qemuDomainObjPrivate *priv = vm->privateData;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
@@ -580,7 +582,7 @@ qemuSecurityRestoreTPMLabels(virQEMUDriver *driver,
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
- -1, priv->rememberOwner) < 0)
+ -1, priv->rememberOwner && attemptLocking) < 0)
goto cleanup;
ret = 0;
diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
index 32f29bc210..4e3186a2af 100644
--- a/src/qemu/qemu_security.h
+++ b/src/qemu/qemu_security.h
@@ -87,11 +87,13 @@ int qemuSecurityRestoreNetdevLabel(virQEMUDriver *driver,
int qemuSecuritySetTPMLabels(virQEMUDriver *driver,
virDomainObj *vm,
- bool setTPMStateLabel);
+ bool setTPMStateLabel,
+ bool attemptLocking);
int qemuSecurityRestoreTPMLabels(virQEMUDriver *driver,
virDomainObj *vm,
- bool restoreTPMStateLabel);
+ bool restoreTPMStateLabel,
+ bool attemptLocking);
int qemuSecuritySetSavedStateLabel(virQEMUDriver *driver,
virDomainObj *vm,
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 55927b4582..6ab4fc9d01 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -934,6 +934,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
virTimeBackOffVar timebackoff;
const unsigned long long timeout = 1000; /* ms */
pid_t pid = -1;
+ bool attemptLocking = true;
cfg = virQEMUDriverGetConfig(driver);
@@ -959,7 +960,20 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
virCommandSetPidFile(cmd, pidfile);
virCommandSetErrorFD(cmd, &errfd);
- if (qemuSecuritySetTPMLabels(driver, vm, true) < 0)
+ if (incomingMigration && qemuTPMHasSharedStorage(driver, vm->def)) {
+ /* If the TPM is being migrated over shared storage, we can't
+ * lock the files before labeling them: the source swtpm
+ * process is still holding a lock on some of them, and it
+ * will only release it after negotiation with the target
+ * swtpm process, which we can't start until labeling has
+ * been performed.
+ *
+ * So we run with fewer guarantees in this specific, narrow
+ * scenario in order to make the migration possible at all */
+ attemptLocking = false;
+ }
+
+ if (qemuSecuritySetTPMLabels(driver, vm, true, attemptLocking) < 0)
return -1;
if (qemuSecurityCommandRun(driver, vm, cmd, cfg->swtpm_user,
@@ -1008,7 +1022,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
virProcessKillPainfully(pid, true);
if (pidfile)
unlink(pidfile);
- qemuSecurityRestoreTPMLabels(driver, vm, true);
+ qemuSecurityRestoreTPMLabels(driver, vm, true, attemptLocking);
return -1;
}
@@ -1144,7 +1158,7 @@ qemuExtTPMStop(virQEMUDriver *driver,
if (outgoingMigration && qemuTPMHasSharedStorage(driver, vm->def))
restoreTPMStateLabel = false;
- if (qemuSecurityRestoreTPMLabels(driver, vm, restoreTPMStateLabel) < 0)
+ if (qemuSecurityRestoreTPMLabels(driver, vm, restoreTPMStateLabel, true) < 0)
VIR_WARN("Unable to restore labels on TPM state and/or log file");
}
--
2.46.0On Fri, Aug 30, 2024 at 17:13:45 +0200, Andrea Bolognani wrote:
> We will never be able to acquire the lock on the destination
> host because the swtpm process that's running on the source
> host is still holding on to it.
So this commit message is too vague ...
[...]
> @@ -552,7 +553,7 @@ qemuSecuritySetTPMLabels(virQEMUDriver *driver,
> goto cleanup;
>
> if (virSecurityManagerTransactionCommit(driver->securityManager,
> - -1, priv->rememberOwner) < 0)
> + -1, priv->rememberOwner && attemptLocking) < 0)
... fix too drastic ...
[...]
> @@ -959,7 +960,20 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
> virCommandSetPidFile(cmd, pidfile);
> virCommandSetErrorFD(cmd, &errfd);
>
> - if (qemuSecuritySetTPMLabels(driver, vm, true) < 0)
> + if (incomingMigration && qemuTPMHasSharedStorage(driver, vm->def)) {
> + /* If the TPM is being migrated over shared storage, we can't
> + * lock the files before labeling them: the source swtpm
> + * process is still holding a lock on some of them, and it
> + * will only release it after negotiation with the target
> + * swtpm process, which we can't start until labeling has
> + * been performed.
> + *
> + * So we run with fewer guarantees in this specific, narrow
> + * scenario in order to make the migration possible at all */
... and the explanation is not explaining the root cause for this.
The libvirt security manager has the following logic:
#define METADATA_OFFSET 1
#define METADATA_LEN 1
if (virFileLock(fd, false,
METADATA_OFFSET, METADATA_LEN, false) < 0) {
int virFileLock(int fd, bool shared, off_t start, off_t len, bool waitForLock)
{
struct flock fl = {
.l_type = shared ? F_RDLCK : F_WRLCK,
.l_whence = SEEK_SET,
.l_start = start,
.l_len = len,
};
int cmd = waitForLock ? F_SETLKW : F_SETLK;
if (fcntl(fd, cmd, &fl) < 0)
return -errno;
return 0;
}
Which in virSecuritySELinuxTransactionRun is applied to any file whose
selinux context is being changed:
if (list->lock) {
paths = g_new0(const char *, list->nItems);
for (i = 0; i < list->nItems; i++) {
virSecuritySELinuxContextItem *item = list->items[i];
const char *p = item->path;
if (item->remember)
VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
}
SWTPM on the other hand uses locks as follows:
SWTPM_NVRAM_Lock_Dir(const char *backend_uri, unsigned int retries)
{
const char *tpm_state_path;
TPM_RESULT rc = 0;
char *lockfile = NULL;
struct flock flock = {
.l_type = F_WRLCK,
.l_whence = SEEK_SET,
.l_start = 0,
.l_len = 0,
};
if (lock_fd >= 0)
return 0;
tpm_state_path = SWTPM_NVRAM_Uri_to_Dir(backend_uri);
if (asprintf(&lockfile, "%s/.lock", tpm_state_path) < 0) {
logprintf(STDERR_FILENO,
"SWTPM_NVRAM_Lock_Dir: Could not asprintf lock filename\n");
return TPM_FAIL;
}
lock_fd = open(lockfile, O_WRONLY|O_CREAT|O_TRUNC|O_NOFOLLOW, 0660);
Thus it's locking just the '.lock' file in the directory but it's
locking it completely.
Thus the libvirt locks is colliding with the swtpm lock.
Since 'swtpm' is locking just the lock file IMO a reasonable solution
for libvirt would be to simply disable seclabel remembering for the
.lock file in the swtpm data directory (which would be one STREQ)
I didn't find any other use of locking for the other backends (linear,
and linear_file).
> + attemptLocking = false;
> + }
> +
> + if (qemuSecuritySetTPMLabels(driver, vm, true, attemptLocking) < 0)
> return -1;
>
> if (qemuSecurityCommandRun(driver, vm, cmd, cfg->swtpm_user,
> @@ -1008,7 +1022,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
> virProcessKillPainfully(pid, true);
> if (pidfile)
> unlink(pidfile);
> - qemuSecurityRestoreTPMLabels(driver, vm, true);
> + qemuSecurityRestoreTPMLabels(driver, vm, true, attemptLocking);
> return -1;
> }
>
> @@ -1144,7 +1158,7 @@ qemuExtTPMStop(virQEMUDriver *driver,
> if (outgoingMigration && qemuTPMHasSharedStorage(driver, vm->def))
> restoreTPMStateLabel = false;
>
> - if (qemuSecurityRestoreTPMLabels(driver, vm, restoreTPMStateLabel) < 0)
> + if (qemuSecurityRestoreTPMLabels(driver, vm, restoreTPMStateLabel, true) < 0)
> VIR_WARN("Unable to restore labels on TPM state and/or log file");
Removing the need for these changes.
© 2016 - 2024 Red Hat, Inc.