[libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling

[John Ferlan]

On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> Fortunately, we have qemu wrappers so it's sufficient to put
> lock/unlock call only there.
> 

Kind of sparse... Maybe a few words related to what benefit locking the
metadata provides. There is a danger in all this too if the unlock does
fail since metadata locks are wait type locks any subsequent lock will wait.

> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_security.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index af3be42854..527563947c 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -26,6 +26,7 @@
>  #include "qemu_domain.h"
>  #include "qemu_security.h"
>  #include "virlog.h"
> +#include "locking/domain_lock.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> @@ -39,6 +40,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>  {
>      int ret = -1;
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> +    bool locked = false;
> +
> +    if (virDomainLockMetadataLock(driver->lockManager, vm) < 0)
> +        goto cleanup;
> +
> +    locked = true;
>  
>      if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>          virSecurityManagerTransactionStart(driver->securityManager) < 0)
> @@ -55,9 +62,17 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>                                              vm->pid) < 0)
>          goto cleanup;
>  
> +    locked = false;

Technically not true yet. Also , I think two such attempts with the
second eliciting the VIR_WARN is perfectly reasonable (IOW: on failure
of the following goto cleanup with locked == true, try again, and spew
that message).

This repeats a few times, but I'll note just once.

> +
> +    if (virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      virSecurityManagerTransactionAbort(driver->securityManager);
> +    if (locked &&
> +        virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
> +        VIR_WARN("unable to release metadata lock");

Should we add the  @stdin_path and @vm->pid and/or vm->name here? It may
help some day. Similar comments for other new VIR_WARN's, but the
parameters change...

>      return ret;
>  }
>  
> @@ -68,6 +83,10 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
>                              bool migrated)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> +    bool unlock = true;
> +
> +    if (virDomainLockMetadataLock(driver->lockManager, vm) < 0)
> +        unlock = false;
>  
>      /* In contrast to qemuSecuritySetAllLabel, do not use
>       * secdriver transactions here. This function is called from
> @@ -79,6 +98,10 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
>                                        vm->def,
>                                        migrated,
>                                        priv->chardevStdioLogd);
> +
> +    if (unlock &&
> +        virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
> +        VIR_WARN("unable to release metadata lock");
>  }
>  
>  
> @@ -88,6 +111,12 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
>                           virDomainDiskDefPtr disk)
>  {
>      int ret = -1;
> +    bool locked = false;
> +
> +    if (virDomainLockMetadataDiskLock(driver->lockManager, vm, disk) < 0)
> +        goto cleanup;
> +
> +    locked = true;
>  
>      if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>          virSecurityManagerTransactionStart(driver->securityManager) < 0)
> @@ -103,9 +132,17 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
>                                              vm->pid) < 0)
>          goto cleanup;
>  
> +    locked = false;
> +
> +    if (virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      virSecurityManagerTransactionAbort(driver->securityManager);
> +    if (locked &&
> +        virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0)
> +        VIR_WARN("unable to release disk metadata lock");
>      return ret;
>  }
>  
> @@ -116,6 +153,12 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
>                               virDomainDiskDefPtr disk)
>  {
>      int ret = -1;
> +    bool locked = false;
> +
> +    if (virDomainLockMetadataDiskLock(driver->lockManager, vm, disk) < 0)
> +        goto cleanup;
> +
> +    locked = true;
>  
>      if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>          virSecurityManagerTransactionStart(driver->securityManager) < 0)
> @@ -131,9 +174,17 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
>                                              vm->pid) < 0)
>          goto cleanup;
>  
> +    locked = false;
> +
> +    if (virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      virSecurityManagerTransactionAbort(driver->securityManager);
> +    if (locked &&
> +        virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0)
> +        VIR_WARN("unable to release disk metadata lock");
>      return ret;
>  }
>  
> @@ -144,6 +195,12 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
>                            virStorageSourcePtr src)
>  {
>      int ret = -1;
> +    bool locked = false;
> +
> +    if (virDomainLockMetadataImageLock(driver->lockManager, vm, src) < 0)
> +        goto cleanup;
> +
> +    locked = true;
>  
>      if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>          virSecurityManagerTransactionStart(driver->securityManager) < 0)
> @@ -159,9 +216,17 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
>                                              vm->pid) < 0)
>          goto cleanup;
>  
> +    locked = false;
> +
> +    if (virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      virSecurityManagerTransactionAbort(driver->securityManager);
> +    if (locked &&
> +        virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0)
> +        VIR_WARN("unable to release image metadata lock");
>      return ret;
>  }
>  
> @@ -172,6 +237,12 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver,
>                                virStorageSourcePtr src)
>  {
>      int ret = -1;
> +    bool locked = false;
> +
> +    if (virDomainLockMetadataImageLock(driver->lockManager, vm, src) < 0)
> +        goto cleanup;
> +
> +    locked = true;
>  
>      if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>          virSecurityManagerTransactionStart(driver->securityManager) < 0)
> @@ -187,9 +258,17 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver,
>                                              vm->pid) < 0)
>          goto cleanup;
>  
> +    locked = false;
> +
> +    if (virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      virSecurityManagerTransactionAbort(driver->securityManager);
> +    if (locked &&
> +        virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0)
> +        VIR_WARN("unable to release image metadata lock");
>      return ret;
>  }
>  

Just below here is HostdevLabel processing... For a SCSI host device
that would seem to cover the type of resource described in the cover
letter to patch6 - at least for iSCSI LUNs (and of course vHBA LUNs).

Need to look at the hostdev->mode and hostdev->source.subsys.type and
dig into the subsys.u.scsi to get to the src->path.

Other than that nothing else sticks out to me.

John

> @@ -258,6 +337,12 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver,
>                             virDomainMemoryDefPtr mem)
>  {
>      int ret = -1;
> +    bool locked = false;
> +
> +    if (virDomainLockMetadataMemLock(driver->lockManager, vm, mem) < 0)
> +        goto cleanup;
> +
> +    locked = true;
>  
>      if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>          virSecurityManagerTransactionStart(driver->securityManager) < 0)
> @@ -273,9 +358,17 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver,
>                                              vm->pid) < 0)
>          goto cleanup;
>  
> +    locked = false;
> +
> +    if (virDomainLockMetadataMemUnlock(driver->lockManager, vm, mem) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      virSecurityManagerTransactionAbort(driver->securityManager);
> +    if (locked &&
> +        virDomainLockMetadataMemUnlock(driver->lockManager, vm, mem) < 0)
> +        VIR_WARN("unable to release memory metadata lock");
>      return ret;
>  }
>  
> @@ -286,6 +379,12 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver,
>                                 virDomainMemoryDefPtr mem)
>  {
>      int ret = -1;
> +    bool locked = false;
> +
> +    if (virDomainLockMetadataMemLock(driver->lockManager, vm, mem) < 0)
> +        goto cleanup;
> +
> +    locked = true;
>  
>      if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>          virSecurityManagerTransactionStart(driver->securityManager) < 0)
> @@ -301,9 +400,17 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver,
>                                              vm->pid) < 0)
>          goto cleanup;
>  
> +    locked = false;
> +
> +    if (virDomainLockMetadataMemUnlock(driver->lockManager, vm, mem) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      virSecurityManagerTransactionAbort(driver->securityManager);
> +    if (locked &&
> +        virDomainLockMetadataMemUnlock(driver->lockManager, vm, mem) < 0)
> +        VIR_WARN("unable to release memory metadata lock");
>      return ret;
>  }
>  
>