[libvirt] [PATCH v3 20/28] security_manager: Load lock plugin on init

[John Ferlan]

On 09/03/2018 11:13 AM, Michal Privoznik wrote:
> On 08/31/2018 07:35 PM, John Ferlan wrote:
>>
>>
>> On 08/27/2018 04:08 AM, Michal Privoznik wrote:
>>> When creating the security managers stack load the lock plugin
>>> too. This is done by creating a single object that all secdrivers
>>> take a reference to. We have to have one shared object so that
>>> the connection to virlockd can be shared between individual
>>> secdrivers. It is important that the connection is shared because
>>> if the connection is closed from one driver while other has a
>>> file locked, then virtlockd does its job and kills libvirtd.
>>>
>>> The cfg.mk change is needed in order to allow syntax-check
>>> to include lock_manager.h. This is generally safe thing to do as
>>> this APIs defined there will always exist. 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/qemu/qemu_driver.c          | 12 ++++--
>>>  src/security/security_manager.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>>>  src/security/security_manager.h |  3 +-
>>>  tests/testutilsqemu.c           |  2 +-
>>>  5 files changed, 94 insertions(+), 8 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; \
>>
>> This I don't really understand - black magic, voodoo stuff ;-)
> 
> This is a simple bash version of switch-case. Except in bash you'd
> s/switch/case/ (because why not, right?). So what this says is: if $dir
> is "util/" then safe="util"; or if $dir is either of "cpu/", "network/",
> "node_device/" .... then safe is "($dir|util|conf|storage)", of if $dir
> is "security/" then safe is "($dir|util|conf|storage|locking)".
> 
> Long story short, this shuts up 'syntax-check' because without it it
> complains about "unsafe" cross-directory include. Hmpf.
> 

Still black magic...  essentially though you've added "locking" to safe
list of tree's that security/* files can access.

>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index da8c4e8991..e06dee8dfb 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -358,7 +358,9 @@ qemuSecurityInit(virQEMUDriverPtr driver)
>>>                                          flags)))
>>>                  goto error;
>>>              if (!stack) {
>>> -                if (!(stack = qemuSecurityNewStack(mgr)))
>>> +                if (!(stack = qemuSecurityNewStack(mgr,
>>> +                                                   cfg->metadataLockManagerName ?
>>> +                                                   cfg->metadataLockManagerName : "nop")))
>>>                      goto error;
>>>              } else {
>>>                  if (qemuSecurityStackAddNested(stack, mgr) < 0)
>>> @@ -372,7 +374,9 @@ qemuSecurityInit(virQEMUDriverPtr driver)
>>>                                      QEMU_DRIVER_NAME,
>>>                                      flags)))
>>>              goto error;
>>> -        if (!(stack = qemuSecurityNewStack(mgr)))
>>> +        if (!(stack = qemuSecurityNewStack(mgr,
>>> +                                           cfg->metadataLockManagerName ?
>>> +                                           cfg->metadataLockManagerName : "nop")))
>>>              goto error;
>>>          mgr = NULL;
>>>      }
>>> @@ -389,7 +393,9 @@ qemuSecurityInit(virQEMUDriverPtr driver)
>>>                                         qemuSecurityChownCallback)))
>>>              goto error;
>>>          if (!stack) {
>>> -            if (!(stack = qemuSecurityNewStack(mgr)))
>>> +            if (!(stack = qemuSecurityNewStack(mgr,
>>> +                                               cfg->metadataLockManagerName ?
>>> +                                               cfg->metadataLockManagerName : "nop")))
>>
>> This essentially gets called through the driver state initialize
>> function, right?  But, wouldn't daemon locks be at a different level?
>> The domain locks would be managed by whatever is managing the domain,
>> the the daemon locks would be managed by whatever is managing the daemon
>> locks, wouldn't they?
>>
>> This also assumes that security_driver is defined/used which wasn't
>> described in the previous patch (while you understand that, it's not
>> necessarily that obvious).  If I just uncomment metadata_lock_manager,
>> but not security_driver, then nothing would happen.
> 
> There is no way you can disable DAC driver. That one is always there. So
> at least one driver is always enabled.
> 
>>
>> I'm trying to figure out the "owner" (so to speak) of this lock. If
>> multiple daemons can use it, then someone has to own it. This partially
>> makes it appear that qemu would own it, but I would think virtlockd
>> would need to own it.  As it, when something causes virtlockd to start
>> so that it's managing locks, that's where initialization takes place.
> 
> I'm not sure what you mean by "owner of the lock". There is owner of the
> lock as recorded in the kernel (this is going to be virtlockd after it
> fcntl(fd, F_SETLK, ..)-s the file we tell it to). The virtlockd however
> has its own records which tell on behalf of which PID it has done the
> locking. Such record is also referred to as the lock owner. Then there
> is lock driver and all that machinery. Its owner is the security driver
> because security driver is actually the place where chown()/setfilecon()
> happen.
> 

Although we've had the #virt conversation today - I at least wanted to
try to provide a bit more context from my thoughts.

At first, the owner I was thinking of would be libvirtd w/ PID. Since
qemu.conf got changed to add "metadata_lock_manager" and not something
perhaps up further and not hypervisor specific. This was perhaps the,
ahh, hmm, moment after reading the previous patch leading to self doubt
whether the previous one would be right.

IIRC, in the long run though what's being developed is something to
handle restoring things in the event that the hypervisor neglected to
restore/undo or it's untimely death prevented it.

> And we want both security drivers (DAC + SELinux) to have the same lock
> driver so that they can share one connection to virtlockd.
> 
> It's important to realize that once a connection to virtlockd is closed
> and there was at least one path locked (= one metadata type lock
> acquired) ALL bets are off. virtlockd will trigger client cleanup code
> (virLockDaemonClientFree) and the registered lock owner (=libvirtd) is
> killed instantly. With this in mind all the KEEP_OPEN flags and
> refcounting starts to make sense.
> 

Ah, yes, the subtlety of the core principle for the design... I'll try
to keep this in mind for future adjustments/reviews.

>>
>> The fact that qemu has an "interest" in knowing if the running lockd
>> code is/can handle these metadata locks still would seemingly mean that
>> the lockmanager would need to have a way to indicate that it is managing
>> the locks.
>>
>> Maybe it's just patch order that is causing the blank stare I have.
>> Maybe the answer lies in a subsequent patch, but something just feels
>> off and I'm not sure I can describe it well enough.
>>
>> I'm concerned with libvirtd restart too and someone changing
>> metadata_lock_manager (one way or the other) and how that alters
>> reality. Maybe I'm overthinking, who knows, it is Friday though and I
>> can almost hear the glasses clinking a the pub you're at ;-).
> 
> Daemon restart should not have any implications. If
> metadata_lock_manager was set, then virtlockd will release all the
> metadata locks that restarting daemon might have had (yay, we want
> this!), or if it wasn't then nothing needs releasing.
> 

Perhaps overthinking on my part... lock mgmt is a tricky beast. You've
had the luxury of more time/iterations to come up with this model.
There's some subtleties that I'm more aware of now. I look forward to
seeing how things change with the next iteration.

>>
>> Does migration present any strange problems? I guess it's not a true
>> distributed locking mechanism, so perhaps not, but it is a concern.
> 
> No problems there in my testing.
> 

OK

John

[...] ... I also snipped the CC to Andrea out too, since I'm not
recommenting on the area you CC'd him for. I agree that the usage of {
... } is one of those gray areas. There's a few other things too, but I
fear posting a patch just in case someone asks me to also modify the
make check tests too.