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

[Michal Prívozník]

On 08/20/2018 05:16 PM, Daniel P. Berrangé wrote:
> On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:
>> Fortunately, we have qemu wrappers so it's sufficient to put
>> lock/unlock call only there.
>>
>> 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;
>> +
>> +    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");
>>      return ret;
>>  }
> 
> I'm wondering if this is really the right level to plug in the metadata
> locking ?  What about if we just pass a virLockManagerPtr to the security
> drivers and let them lock each resource at the time they need to modify
> its metadata. This will trivially guarantee that we always lock the exact
> set of files that we'll be changing metadata on.
> 
> eg in SELinux driver the virSecuritySELinuxSetFileconHelper method
> would directly call virLockManagerAcquire & virLockManagerRelease,
> avoiding the entire virDomainLock  abstraction which was really
> focused around managing the content locks around lifecycle state
> changes.
> 

Yeah, I vaguely recall writing code like this (when I was trying to
solve this some time ago). Okay, let me see if that's feasible.

But boy, this is getting hairy.

Michal