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@xxxxxxxxxx>
> ---
> 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 ;-)
> 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.
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.
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 ;-).
Does migration present any strange problems? I guess it's not a true
distributed locking mechanism, so perhaps not, but it is a concern.
> 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;
> +
> 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?
Once enough of these are called that then triggers the call to
virSecurityManagerLockDispose, right?
> 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.
> + 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.
> + 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.
> +
> + 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
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?
> + 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?
John
> int virSecurityManagerStackAddNested(virSecurityManagerPtr stack,
> virSecurityManagerPtr nested);
>
> diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
> index 8438613f28..2a2a88361b 100644
> --- a/tests/testutilsqemu.c
> +++ b/tests/testutilsqemu.c
> @@ -721,7 +721,7 @@ int qemuTestDriverInit(virQEMUDriver *driver)
> if (!(mgr = virSecurityManagerNew("none", "qemu",
> VIR_SECURITY_MANAGER_PRIVILEGED)))
> goto error;
> - if (!(driver->securityManager = virSecurityManagerNewStack(mgr)))
> + if (!(driver->securityManager = virSecurityManagerNewStack(mgr, "nop")))
> goto error;
>
> return 0;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
