[libvirt] [PATCH v2 5/5] qemu: Use transactions from security driver

[John Ferlan]

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 at 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);
>  }
>  
>  
>