On 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.
