Re: [libvirt] [PATCH v3 20/28] security_manager: Load lock plugin on init
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
>>> ---
>>> 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
Re: [libvirt] [PATCH v3 20/28] security_manager: Load lock plugin on init
On Mon, 2018-09-03 at 17:13 +0200, Michal Privoznik wrote:
> On 08/31/2018 07:35 PM, John Ferlan wrote:
> > > +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.
The relevant chapter of our guidelines[1] states:
Omit the curly braces around an if, while, for etc. body only
when both that body and the condition itself occupy a single
line. In every other case we require the braces. This ensures
that it is trivially easy to identify a single-statement loop:
each has only one line in its body.
while (expr) // single line body; {} is forbidden
single_line_stmt();
while (expr(arg1,
arg2)) // indentation makes it obvious it is single line,
single_line_stmt(); // {} is optional (not enforced either way)
while (expr1 &&
expr2) { // multi-line, at same indentation, {} required
single_line_stmt();
}
In the recent patch you mention[2] we were squarely in Example
Three territory, so there was no question about whether or not
curly braces should be added.
This time around we may refer to Example Two, "not enforced
either way", but given just how much whitespace there is on
the second line I would argue the braces should definitely be
there to disambiguate the situation.
> 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.
Completely agree. We have way too many subtleties in our
guidelines, which leads to uneven adoption despite
syntax-check and people like me annoying contributors during
review :)
While switching to an all braces, all the time approach is
probably not realistic due to the sheer amount of changes
that would be required to make existing code compliant, we
could start requiring it for new code and go from there.
[1] https://libvirt.org/hacking.html#curly_braces
[2] https://www.redhat.com/archives/libvir-list/2018-August/msg01269.html
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
>> ---
>> 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
Re: [libvirt] [PATCH v3 20/28] security_manager: Load lock plugin on init
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
> ---
> 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
[libvirt] [PATCH v3 20/28] security_manager: Load lock plugin on init
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
---
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; \
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")))
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;
+
+bool pathLocked;
+};
+
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);
+
VIR_FREE(mgr->privateData);
}
+static void
+virSecurityManagerLockDispose(void *obj)
+{
+virSecurityManagerLockPtr lock = obj;
+
+virCondDestroy(>cond);
+
+if
