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@xxxxxxxxxx>
> ---
> 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);
> }
>
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
