[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 08/17/2018 08:38 PM, John Ferlan wrote:
> 
> 
> 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 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.

I don't think it is safe to unlock something twice if we've locked it
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...

Ah, good point. Okay.

> 
>>      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.

Oh, this was the reason I made VIR_WARN() so sparse. I did not wanted to
get into business of getting paths from all the complicated structs we
have. I'll just put vm->def->name and vm->pid into the warn message.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]