10:13 a.m.
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>
> ---
> 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.
> goto error;
> } else {
> if (qemuSecurityStackAddNested(stack, mgr) < 0)
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 21eb6f7452..caaff1f703 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -28,21 +28,39 @@
> #include "viralloc.h"
> #include "virobject.h"
> #include "virlog.h"
> +#include "locking/lock_manager.h"
>
> #define VIR_FROM_THIS VIR_FROM_SECURITY
>
> VIR_LOG_INIT("security.security_manager");
>
> +typedef struct _virSecurityManagerLock virSecurityManagerLock;
> +typedef virSecurityManagerLock *virSecurityManagerLockPtr;
> +struct _virSecurityManagerLock {
> + virObjectLockable parent;
> +
> + virCond cond;
> +
> + virLockManagerPluginPtr lockPlugin;
> + virLockManagerPtr lock;
using @lock here is really confusing later on when I see lock->lock.
> +
> + bool pathLocked;
Unused for now...
> +};
> +
> struct _virSecurityManager {
> virObjectLockable parent;
>
> virSecurityDriverPtr drv;
> unsigned int flags;
> const char *virtDriver;
> +
> + virSecurityManagerLockPtr lock;
so maybe s/lock/metadataLock/ here?
> +
> void *privateData;
> };
>
> static virClassPtr virSecurityManagerClass;
> +static virClassPtr virSecurityManagerLockClass;
>
>
> static
> @@ -52,16 +70,36 @@ void virSecurityManagerDispose(void *obj)
>
> if (mgr->drv->close)
> mgr->drv->close(mgr);
> +
> + virObjectUnref(mgr->lock);
> +
So this is the opposite end (so to speak) of the virObjectRef in
virSecurityManagerNewStack and virSecurityManagerStackAddNested, right?
Yes.
Once enough of these are called that then triggers the call to>
virSecurityManagerLockDispose, right?
Yes.
As I say above, the idea is to have only one instance of lock driver for
both security drivers. Only then the connection can be shared. If it was
two independent lock drivers (one per each sec driver), there is no way
we could make them to share one connection.
Therefore, the first call that creates a security driver has to create
the lock driver, and any subsequent call to sec driver create will just
take a reference to already existing lock driver.
> VIR_FREE(mgr->privateData);
> }
>
>
> +static void
> +virSecurityManagerLockDispose(void *obj)
> +{
> + virSecurityManagerLockPtr lock = obj;
> +
> + virCondDestroy(&lock->cond);
> +
> + if (lock->lock)
> + virLockManagerCloseConn(lock->lock, 0);
So without looking forward to the next patch[es] - I'm stumped by this.
The code is certainly not related to the Ref/Unref of mgr->lock and I
haven't seen anything that's filling in lock->lock yet.
This is basically there to clean any stale connection. Any connection
where our lock() is not paired with unlock() (should there be such case).
> + virLockManagerFree(lock->lock);
Likewise for this since all that virSecurityManagerLockNew does is
managed ->cond and ->lockPlugin.
I get that future code will handle this, but I think it's easier to
review so that when future code is added we then adjust this as well.
Okay, I'll move it respective patches.
> + virLockManagerPluginUnref(lock->lockPlugin);
> +}
> +
> +
> static int
> virSecurityManagerOnceInit(void)
> {
> if (!VIR_CLASS_NEW(virSecurityManager, virClassForObjectLockable()))
> return -1;
>
> + if (!VIR_CLASS_NEW(virSecurityManagerLock, virClassForObjectLockable()))
> + return -1;
> +
> return 0;
> }
>
> @@ -106,8 +144,32 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
> }
>
>
> +static virSecurityManagerLockPtr
> +virSecurityManagerLockNew(const char *lockManagerName)
> +{
> + virSecurityManagerLockPtr ret;
> +
> + if (!(ret = virObjectLockableNew(virSecurityManagerLockClass)))
> + return NULL;
> +
> + if (virCondInit(&ret->cond) < 0)
> + goto error;
> +
> + if (!(ret->lockPlugin = virLockManagerPluginNew(lockManagerName,
> + NULL, NULL, 0))) {
> + goto error;
> + }
The { } aren't necessary, but understandable.
Andrea would disagree. In one other patch that I had on the list
recently he made me to put the brackets there arguing that if() is not a
single line or something.
On a side note, this points to a bug in our coding style. Either we
should put brackets everywhere (even for true single line if()-s like
those in the context above), or not be picky about "multiline" if()-s
like the one we are talking about here.
> +
> + return ret;
> + error:
> + virObjectUnref(ret);
> + return NULL;
> +}
> +
> +
> virSecurityManagerPtr
> -virSecurityManagerNewStack(virSecurityManagerPtr primary)
> +virSecurityManagerNewStack(virSecurityManagerPtr primary,
> + const char *lockManagerName)
> {
> virSecurityManagerPtr mgr =
> virSecurityManagerNewDriver(&virSecurityDriverStack,
> @@ -117,9 +179,16 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary)
> if (!mgr)
> return NULL;
>
> + if (!(mgr->lock = virSecurityManagerLockNew(lockManagerName)))
You're in a world of hurt if @lockManagerName == NULL
Sure, but such call is broken virtually by definition :-)
Maybe this is where the conversion to "nop" could take
place. I know
we're following existing convention for domain lock code, but should we?
I rather segfault than proceed with false sense of safety.
> + goto error;
> +
> if (virSecurityStackAddNested(mgr, primary) < 0)
> goto error;
>
> + /* Propagate lock manager */
> + if (!primary->lock)
> + primary->lock = virObjectRef(mgr->lock);
> +
> return mgr;
> error:
> virObjectUnref(mgr);
> @@ -133,7 +202,15 @@ virSecurityManagerStackAddNested(virSecurityManagerPtr stack,
> {
> if (STRNEQ("stack", stack->drv->name))
> return -1;
> - return virSecurityStackAddNested(stack, nested);
> +
> + if (virSecurityStackAddNested(stack, nested) < 0)
> + return -1;
> +
> + /* Propagate lock manager */
> + if (!nested->lock)
> + nested->lock = virObjectRef(stack->lock);
> +
> + return 0;
> }
>
>
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index 1ead369e82..c589b8808d 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -47,7 +47,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name,
> const char *virtDriver,
> unsigned int flags);
>
> -virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary);
> +virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary,
> + const char *lockManagerName);
Cannot be NULL, true? Should we ATTRIBUTE_NONNULL it?
Sure.
Michal