[libvirt] [PATCH v3 15/17] qemu_hotplug: Relabel memdev

[John Ferlan]

On 03/09/2017 11:06 AM, Michal Privoznik wrote:
> Now that we have APIs for relabel memdevs on hotplug, fill in the
> missing implementation in qemu hotplug code.
> 
> The qemuSecurity wrappers might look like overkill for now,
> because qemu namespace code does not deal with the nvdimms yet.
> Nor does our cgroup code.  But hey, there's cgroup_device_acl
> variable in qemu.conf. If users add their /dev/pmem* device in
> there, the device is allowed in cgroups and created in the
> namespace so they can successfully passthrough it to the domain.
> It doesn't look like overkill after all, does it?
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_hotplug.c  | 13 +++++++++++
>  src/qemu/qemu_security.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_security.h |  8 +++++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 4e416b12e..7e19d2f82 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2215,6 +2215,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>      char *objalias = NULL;
>      const char *backendType;
>      bool objAdded = false;
> +    bool teardownlabel = false;
>      virJSONValuePtr props = NULL;
>      virObjectEventPtr event;
>      int id;
> @@ -2244,6 +2245,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>                                    priv->qemuCaps, vm->def, mem, NULL, true) < 0)
>          goto cleanup;
>  
> +    if (qemuSecuritySetMemoryLabel(driver, vm, mem) < 0)
> +        goto cleanup;
> +    teardownlabel = true;
> +

@props will be leaked (found by Coverity)

If the code would follow other models, then virJSONValueFree(props)
would be in the cleanup label.  Since props = NULL is done after the
AddObject that should be safe.  Of course that means a couple of
existing virJSONValueFree(props) would be removed and just goto cleanup
left. Whether those are done inline here or as a patch stuffed in before
this one - doesn't matter to me...


>      if (virDomainMemoryInsert(vm->def, mem) < 0) {
>          virJSONValueFree(props);
>          goto cleanup;
> @@ -2288,6 +2293,11 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>   audit:
>      virDomainAuditMemory(vm, oldmem, newmem, "update", ret == 0);
>   cleanup:
> +    if (mem && ret < 0) {

Given this requirement for 'mem' to still be valid one wonders if rather
than goto cleanup, maybe the goto should be some label below removedef
and above goto audit... Furthermore whether that should also be wrapped
in a virSaveLastError...

 restoredevice:
    if (!mem) {
       VIR_WARN("Unable to restore host device la
       goto audit;
    }

    orig_err = virSaveLastError();
    if (teardownlabel &&
        qemuSecurityRestoreMemoryLabel(driver, vm, mem) < 0)
        VIR_WARN("Unable to restore security label on memdev");
    virSetError(orig_err);
    virFreeError(orig_err);

I think cgroup and namespace could use the same label - whether or not
you have a VIR_WARN for when if (!mem) is true is your call.

Using the label leaves out the ugliness of (mem && ret < 0) from the
cleanup path.

ACK w/ adjustments...

John


> +        if (teardownlabel && qemuSecurityRestoreMemoryLabel(driver, vm, mem) < 0)
> +            VIR_WARN("Unable to restore security label on memdev");
> +    }
> +
>      virObjectUnref(cfg);
>      VIR_FREE(devstr);
>      VIR_FREE(objalias);
> @@ -3748,6 +3758,9 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
>      if ((idx = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
>          virDomainMemoryRemove(vm->def, idx);
>  
> +    if (qemuSecurityRestoreMemoryLabel(driver, vm, mem) < 0)
> +        VIR_WARN("Unable to restore security label on memdev");
> +
>      virDomainMemoryDefFree(mem);
>  
>      /* fix the balloon size */
> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index f2931976b..61934f990 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -245,3 +245,59 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver,
>      virSecurityManagerTransactionAbort(driver->securityManager);
>      return ret;
>  }
> +
> +
> +int
> +qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver,
> +                           virDomainObjPtr vm,
> +                           virDomainMemoryDefPtr mem)
> +{
> +    int ret = -1;
> +
> +    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> +        virSecurityManagerTransactionStart(driver->securityManager) < 0)
> +        goto cleanup;
> +
> +    if (virSecurityManagerSetMemoryLabel(driver->securityManager,
> +                                         vm->def,
> +                                         mem) < 0)
> +        goto cleanup;
> +
> +    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> +        virSecurityManagerTransactionCommit(driver->securityManager,
> +                                            vm->pid) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    virSecurityManagerTransactionAbort(driver->securityManager);
> +    return ret;
> +}
> +
> +
> +int
> +qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver,
> +                               virDomainObjPtr vm,
> +                               virDomainMemoryDefPtr mem)
> +{
> +    int ret = -1;
> +
> +    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> +        virSecurityManagerTransactionStart(driver->securityManager) < 0)
> +        goto cleanup;
> +
> +    if (virSecurityManagerRestoreMemoryLabel(driver->securityManager,
> +                                             vm->def,
> +                                             mem) < 0)
> +        goto cleanup;
> +
> +    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> +        virSecurityManagerTransactionCommit(driver->securityManager,
> +                                            vm->pid) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    virSecurityManagerTransactionAbort(driver->securityManager);
> +    return ret;
> +}
> diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
> index d86db3f6b..7b25855bf 100644
> --- a/src/qemu/qemu_security.h
> +++ b/src/qemu/qemu_security.h
> @@ -62,6 +62,14 @@ int qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver,
>                                      virDomainObjPtr vm,
>                                      virDomainHostdevDefPtr hostdev);
>  
> +int qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver,
> +                               virDomainObjPtr vm,
> +                               virDomainMemoryDefPtr mem);
> +
> +int qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver,
> +                                   virDomainObjPtr vm,
> +                                   virDomainMemoryDefPtr mem);
> +
>  /* Please note that for these APIs there is no wrapper yet. Do NOT blindly add
>   * new APIs here. If an API can touch a /dev file add a proper wrapper instead.
>   */
>