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

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(a)redhat.com&gt; > --- > 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.