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

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

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

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.

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

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