6:18 a.m.
On 01/09/2017 07:58 AM, Michal Privoznik wrote:
So far if qemu is spawned under separate mount namespace in order
to relabel everything it needs an access to the security driver
is run in that namespace too. This has a very nasty down side -
s/is/to/
it is being run in a separate process, so any internal state
transition is NOT reflected in the dameon. This can lead to many
s/dameon/daemon
sleepless nights. Therefore, use the transaction APIs so that
libvirt developers can sleep tight again.
Having trouble sleeping lately? ;-)
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_security.c | 100 ++++++++++++++---------------------------------
1 file changed, 30 insertions(+), 70 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 9ab91e9f2..544feeb4a 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -40,66 +40,31 @@ struct qemuSecuritySetRestoreAllLabelData {
};
-static int
-qemuSecuritySetRestoreAllLabelHelper(pid_t pid,
- void *opaque)
-{
- struct qemuSecuritySetRestoreAllLabelData *data = opaque;
-
- virSecurityManagerPostFork(data->driver->securityManager);
-
- if (data->set) {
- VIR_DEBUG("Setting up security labels inside namespace pid=%lld",
- (long long) pid);
- if (virSecurityManagerSetAllLabel(data->driver->securityManager,
- data->vm->def,
- data->stdin_path) < 0)
- return -1;
- } else {
- VIR_DEBUG("Restoring security labels inside namespace pid=%lld",
- (long long) pid);
- if (virSecurityManagerRestoreAllLabel(data->driver->securityManager,
- data->vm->def,
- data->migrated) < 0)
- return -1;
- }
-
- return 0;
-}
-
-
int
qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
const char *stdin_path)
{
- struct qemuSecuritySetRestoreAllLabelData data;
+ int ret = -1;
- memset(&data, 0, sizeof(data));
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ goto cleanup;
- data.set = true;
- data.driver = driver;
- data.vm = vm;
- data.stdin_path = stdin_path;
+ if (virSecurityManagerSetAllLabel(driver->securityManager,
+ vm->def,
+ stdin_path) < 0)
+ goto cleanup;
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
- if (virSecurityManagerPreFork(driver->securityManager) < 0)
- return -1;
Both paths have removed the PreFork/PostFork processing... Is that then
no longer required? This is/was the only PreFork caller I think.
- if (virProcessRunInMountNamespace(vm->pid,
- qemuSecuritySetRestoreAllLabelHelper,
- &data) < 0) {
- virSecurityManagerPostFork(driver->securityManager);
- return -1;
- }
- virSecurityManagerPostFork(driver->securityManager);
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionCommit(driver->securityManager,
+ vm->pid) < 0)
+ goto cleanup;
Once we've entered Commit - calling Abort is a noop since Commit takes
the 'list', clears the thread local storage, and will free the list on exit.
- } else {
- if (virSecurityManagerSetAllLabel(driver->securityManager,
- vm->def,
- stdin_path) < 0)
- return -1;
- }
- return 0;
+ ret = 0;
+ cleanup:
+ virSecurityManagerTransactionAbort(driver->securityManager);
+ return ret;
}
@@ -108,27 +73,22 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
bool migrated)
{
- struct qemuSecuritySetRestoreAllLabelData data;
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ goto cleanup;
- memset(&data, 0, sizeof(data));
-
- data.driver = driver;
- data.vm = vm;
- data.migrated = migrated;
-
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
- if (virSecurityManagerPreFork(driver->securityManager) < 0)
- return;
-
- virProcessRunInMountNamespace(vm->pid,
- qemuSecuritySetRestoreAllLabelHelper,
- &data);
- virSecurityManagerPostFork(driver->securityManager);
- } else {
- virSecurityManagerRestoreAllLabel(driver->securityManager,
+ if (virSecurityManagerRestoreAllLabel(driver->securityManager,
vm->def,
- migrated);
- }
+ migrated) < 0)
+ goto cleanup;
This would seemingly be the only place where goto cleanup is necessary
John
+
+ if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+ virSecurityManagerTransactionCommit(driver->securityManager,
+ vm->pid) < 0)
+ goto cleanup;
+
+ cleanup:
+ virSecurityManagerTransactionAbort(driver->securityManager);
}