[libvirt] [PATCH v4 15/23] security_manager: Load lock plugin on init

[Michal Privoznik]

On 09/17/2018 09:56 PM, John Ferlan wrote:
> 
> 
> On 09/10/2018 05:36 AM, Michal Privoznik wrote:
>> Now that we know what metadata lock manager user wishes to use we
>> can load it when initializing security driver. This is achieved
>> by adding new argument to virSecurityManagerNewDriver() and
>> subsequently to all functions that end up calling it.
>>
>> The cfg.mk change is needed in order to allow lock_manager.h
>> inclusion in security driver without 'syntax-check' complaining.
>> This is safe thing to do as locking APIs will always exist (it's
>> only backend implementation that changes). However, instead of
>> allowing the include for all other drivers (like cpu, network,
>> and so on) allow it only for security driver. This will still
>> trigger the error if including from other drivers.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  cfg.mk                           |  4 +++-
>>  src/lxc/lxc_controller.c         |  3 ++-
>>  src/lxc/lxc_driver.c             |  2 +-
>>  src/qemu/qemu_driver.c           |  3 +++
>>  src/security/security_manager.c  | 22 ++++++++++++++++++++++
>>  src/security/security_manager.h  |  2 ++
>>  tests/seclabeltest.c             |  2 +-
>>  tests/securityselinuxlabeltest.c |  2 +-
>>  tests/securityselinuxtest.c      |  2 +-
>>  tests/testutilsqemu.c            |  2 +-
>>  10 files changed, 37 insertions(+), 7 deletions(-)
>>
>> diff --git a/cfg.mk b/cfg.mk
>> index 609ae869c2..e0a7b5105a 100644
>> --- a/cfg.mk
>> +++ b/cfg.mk
>> @@ -787,8 +787,10 @@ sc_prohibit_cross_inclusion:
>>  	  case $$dir in \
>>  	    util/) safe="util";; \
>>  	    access/ | conf/) safe="($$dir|conf|util)";; \
>> -	    cpu/| network/| node_device/| rpc/| security/| storage/) \
>> +	    cpu/| network/| node_device/| rpc/| storage/) \
>>  	      safe="($$dir|util|conf|storage)";; \
>> +	    security/) \
>> +	      safe="($$dir|util|conf|storage|locking)";; \
>>  	    xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";; \
>>  	    *) safe="($$dir|$(mid_dirs)|util)";; \
>>  	  esac; \
>> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
>> index 4e84391bf5..5f957eb1f8 100644
>> --- a/src/lxc/lxc_controller.c
>> +++ b/src/lxc/lxc_controller.c
>> @@ -2625,7 +2625,8 @@ int main(int argc, char *argv[])
>>      ctrl->handshakeFd = handshakeFd;
>>  
>>      if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver,
>> -                                                        LXC_DRIVER_NAME, 0)))
>> +                                                        LXC_DRIVER_NAME,
>> +                                                        "nop", 0)))
>>          goto cleanup;
>>  
>>      if (ctrl->def->seclabels) {
>> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
>> index 8867645cdc..93aa25e9e6 100644
>> --- a/src/lxc/lxc_driver.c
>> +++ b/src/lxc/lxc_driver.c
>> @@ -1532,7 +1532,7 @@ lxcSecurityInit(virLXCDriverConfigPtr cfg)
>>          flags |= VIR_SECURITY_MANAGER_REQUIRE_CONFINED;
>>  
>>      virSecurityManagerPtr mgr = virSecurityManagerNew(cfg->securityDriverName,
>> -                                                      LXC_DRIVER_NAME, flags);
>> +                                                      LXC_DRIVER_NAME, "nop", flags);
> 
> I think we should use NULL instead of "nop"?  Keeping the default of
> "nop" under the covers (so to speak). Similar for other callers...
> 
>>      if (!mgr)
>>          goto error;
>>  
> 
> [...]
> 
>> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
>> index 9f770d8c53..5c8370c159 100644
>> --- a/src/security/security_manager.c
>> +++ b/src/security/security_manager.c
> 
> [...]
> 
>>  static virClassPtr virSecurityManagerClass;
>> @@ -52,6 +55,9 @@ void virSecurityManagerDispose(void *obj)
>>  
>>      if (mgr->drv->close)
>>          mgr->drv->close(mgr);
> 
> Order/setup issue here mgr->drv-> cannot be assumed right now!
> 
>> +
>> +    virObjectUnref(mgr->lockPlugin);
>> +
>>      VIR_FREE(mgr->privateData);
>>  }
>>  
>> @@ -71,6 +77,7 @@ VIR_ONCE_GLOBAL_INIT(virSecurityManager);
>>  static virSecurityManagerPtr
>>  virSecurityManagerNewDriver(virSecurityDriverPtr drv,
>>                              const char *virtDriver,
>> +                            const char *lockManagerPluginName,
>>                              unsigned int flags)
>>  {
>>      virSecurityManagerPtr mgr = NULL;
>> @@ -90,6 +97,14 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
>>      if (!(mgr = virObjectLockableNew(virSecurityManagerClass)))
>>          goto error;
>>  
>> +    if (!lockManagerPluginName)
>> +        lockManagerPluginName = "nop";
>> +
>> +    if (!(mgr->lockPlugin = virLockManagerPluginNew(lockManagerPluginName,
>> +                                                    NULL, NULL, 0))) {
>> +        goto error;
>> +    }
>> +
> 
> Want to watching things die spectacularly?  Don't pass NULL, "nop", or
> "lockd" here... For example, change securityselinuxlabeltest.c to pass
> "foobar" and enjoy.
> 
> Problem is that mgr->drv-> and eventually mgr->fd == -1 is assumed in
> virSecurityManagerDispose.
> 
>>      mgr->drv = drv;
>>      mgr->flags = flags;
>>      mgr->virtDriver = virtDriver;
> 
> [...]
> 
>> diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
>> index 48fee7cd28..85797411eb 100644
>> --- a/tests/securityselinuxlabeltest.c
>> +++ b/tests/securityselinuxlabeltest.c
>> @@ -349,7 +349,7 @@ mymain(void)
>>      if (!rc)
>>          return EXIT_AM_SKIP;
>>  
>> -    if (!(mgr = virSecurityManagerNew("selinux", "QEMU",
>> +    if (!(mgr = virSecurityManagerNew("selinux", "QEMU", "nop",
> 
> Just for "completeness", why not pass "lockd" here?

Because that would require virtlockd running every time the test is run.
Well, not right now but after the last patch in the series.

> 
> Wonder if it's worth creating a false test that passes a non existent
> image (foobar) just to ensure it fails properly so that someone doesn't
> inadvertently undo what you'll need to change...

Maybe. But I'll save that for a follow up patch.

Michal