12:25 p.m.
So, you may be familiar with this already. Well, I've tried to get these
patches in like a year ago (or even more). Point is, these ones are new,
written from scratch. However, still based on idea, that virtlockd will keep
the track of the original seclabels. So far only DAC driver is fixed, but the
infrastructure I'm proposing here is easily extensible to other drivers too.
Even if there's some disagreement on the design, the first few patches fix some
bugs, so they should make it in.
Michal Privoznik (23):
virtlockd: Don't SIGSEGV on SIGUSR1
security_dac: Fix TODO marks
virSecurityDACSetOwnershipInternal: Don't chown so often
security_dac: Introduce remember/recall stubs
virSecurityDACSetOwnership: Pass virSecurityDACDataPtr
virSecurityDACRestoreSecurityFileLabel: Pass virSecurityDACDataPtr
security_dac: Limit usage of virSecurityDACSetOwnershipInternal
security_dac: Plug in remember/recall APIs
lock_protocol: Add two new remote procedures
lock_daemon: Switch to wrapper locking functions
locking: Introduce virSeclabelSpace
virtlockd: Work virSeclabelSpace in
virLockDriver: Introduce virLockDriverRemember and virLockDriverRecall
lock_driver_nop: Implement remember and recall APIs
lock_driver_lockd: Implement remember and recall APIs
lock_manager: Implement remember & recall APIs
locking: Favour enum type over int
lock_driver: Introduce VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL
virSecurityManagerNewDAC: Pass locking plugin in
security_dac: Remember security labels
security_dac: Restore original owner more often
security: Introduce virSecurityManagerDomainRestoreDirLabel
qemuProcessStop: Restore seclabels on dirs too
cfg.mk | 2 +-
po/POTFILES.in | 1 +
src/Makefile.am | 3 +
src/libvirt_private.syms | 3 +
src/lock_protocol-structs | 15 +
src/locking/lock_daemon.c | 69 ++++-
src/locking/lock_daemon.h | 8 +
src/locking/lock_daemon_dispatch.c | 68 +++++
src/locking/lock_daemon_seclabels.c | 545 ++++++++++++++++++++++++++++++++++++
src/locking/lock_daemon_seclabels.h | 43 +++
src/locking/lock_driver.h | 42 ++-
src/locking/lock_driver_lockd.c | 103 ++++++-
src/locking/lock_driver_nop.c | 28 +-
src/locking/lock_driver_sanlock.c | 4 +-
src/locking/lock_manager.c | 36 ++-
src/locking/lock_manager.h | 13 +-
src/locking/lock_protocol.x | 29 +-
src/qemu/qemu_driver.c | 3 +-
src/qemu/qemu_process.c | 22 +-
src/security/security_dac.c | 288 +++++++++++++++----
src/security/security_dac.h | 2 +
src/security/security_driver.h | 5 +-
src/security/security_manager.c | 20 +-
src/security/security_manager.h | 7 +-
src/security/security_selinux.c | 16 ++
src/security/security_stack.c | 20 ++
26 files changed, 1310 insertions(+), 85 deletions(-)
create mode 100644 src/locking/lock_daemon_seclabels.c
create mode 100644 src/locking/lock_daemon_seclabels.h
--
2.4.9
12:25 p.m.
New subject: [libvirt] [PATCH v1 01/23] virtlockd: Don't SIGSEGV on SIGUSR1
So we have this mechanism that on SIGUSR1 the virtlockd dumps its
internal state into a JSON file, reexec itself and the reloads
the internal state back. However, there's a bug in our
implementation:
(gdb) signal SIGUSR1
Continuing with signal SIGUSR1.
[Thread 0x7fd094f7b700 (LWP 10602) exited]
process 10600 is executing new program:
/home/zippy/work/libvirt/libvirt.git/src/virtlockd
warning: Could not load shared library symbols for linux-vdso.so.1.
Do you need "set solib-search-path" or "set sysroot"?
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7fb28bc3c700 (LWP 14501)]
Program received signal SIGSEGV, Segmentation fault.
0x00007fb29133d530 in virExpandN (ptrptr=0x70, size=8, countptr=0x68, add=1,
report=true, domcode=7, filename=0x7fb29138aeab "rpc/virnetserver.c",
funcname=0x7fb29138b680 <__FUNCTION__.15821> "virNetServerAddProgram",
linenr=661) at util/viralloc.c:288
288 if (*countptr + add < *countptr) {
(gdb) bt
#0 0x00007fb29133d530 in virExpandN (ptrptr=0x70, size=8, countptr=0x68, add=1,
report=true, domcode=7, filename=0x7fb29138aeab "rpc/virnetserver.c",
funcname=0x7fb29138b680 <__FUNCTION__.15821> "virNetServerAddProgram",
linenr=661) at util/viralloc.c:288
#1 0x00007fb29132a267 in virNetServerAddProgram (srv=0x0, prog=0x7fb2915d08b0) at
rpc/virnetserver.c:661
#2 0x00007fb29131f27f in main (argc=1, argv=0x7fff8f771298) at
locking/lock_daemon.c:1445
Notice the NULL @srv passed to frame 2? Usually, the @srv
variable is initialized on fresh start. However, in case of
daemon reload, the code path that is responsible for initializing
the value was not triggered and therefore we crashed immediately.
Fix this by always setting the variable.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_daemon.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 55fc0c3..56db959 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -1366,6 +1366,8 @@ int main(int argc, char **argv) {
ret = VIR_LOCK_DAEMON_ERR_NETWORK;
goto cleanup;
}
+ } else if (rv == 1) {
+ srv = virNetDaemonGetServer(lockDaemon->dmn, 0);
}
if (timeout != -1) {
--
2.4.9
11:37 p.m.
New subject: [libvirt] [PATCH v1 01/23] virtlockd: Don't SIGSEGV on SIGUSR1
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
So we have this mechanism that on SIGUSR1 the virtlockd dumps its
internal state into a JSON file, reexec itself and the reloads
the internal state back. However, there's a bug in our
implementation:
(gdb) signal SIGUSR1
Continuing with signal SIGUSR1.
[Thread 0x7fd094f7b700 (LWP 10602) exited]
process 10600 is executing new program:
/home/zippy/work/libvirt/libvirt.git/src/virtlockd
warning: Could not load shared library symbols for linux-vdso.so.1.
Do you need "set solib-search-path" or "set sysroot"?
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7fb28bc3c700 (LWP 14501)]
Program received signal SIGSEGV, Segmentation fault.
0x00007fb29133d530 in virExpandN (ptrptr=0x70, size=8, countptr=0x68, add=1,
report=true, domcode=7, filename=0x7fb29138aeab "rpc/virnetserver.c",
funcname=0x7fb29138b680 <__FUNCTION__.15821> "virNetServerAddProgram",
linenr=661) at util/viralloc.c:288
288 if (*countptr + add < *countptr) {
(gdb) bt
#0 0x00007fb29133d530 in virExpandN (ptrptr=0x70, size=8, countptr=0x68, add=1,
report=true, domcode=7, filename=0x7fb29138aeab "rpc/virnetserver.c",
funcname=0x7fb29138b680 <__FUNCTION__.15821> "virNetServerAddProgram",
linenr=661) at util/viralloc.c:288
#1 0x00007fb29132a267 in virNetServerAddProgram (srv=0x0, prog=0x7fb2915d08b0) at
rpc/virnetserver.c:661
#2 0x00007fb29131f27f in main (argc=1, argv=0x7fff8f771298) at
locking/lock_daemon.c:1445
Notice the NULL @srv passed to frame 2? Usually, the @srv
variable is initialized on fresh start. However, in case of
daemon reload, the code path that is responsible for initializing
the value was not triggered and therefore we crashed immediately.
Fix this by always setting the variable.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_daemon.c | 2 ++
1 file changed, 2 insertions(+)
ACK - although you may want to considering altering the comment after
the virLockDaemonPostExecRestart call to indicate that getting the need
to get the srv pointer again...
John
12:25 p.m.
New subject: [libvirt] [PATCH v1 02/23] security_dac: Fix TODO marks
Correctly mark the places where we need to remember and recall
file ownership. We don't want to mislead any potential developer.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 864d75b..0dfe570 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -312,6 +312,7 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
static int
virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
{
+ /* XXX record previous ownership */
return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid);
}
@@ -324,7 +325,7 @@ virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr
priv,
VIR_INFO("Restoring DAC user and group on '%s'",
NULLSTR(src ? src->path : path));
- /* XXX record previous ownership */
+ /* XXX recall previous ownership */
return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);
}
--
2.4.9
11:55 p.m.
New subject: [libvirt] [PATCH v1 02/23] security_dac: Fix TODO marks
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
Correctly mark the places where we need to remember and recall
file ownership. We don't want to mislead any potential developer.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
ACK
John
12:25 p.m.
New subject: [libvirt] [PATCH v1 03/23] virSecurityDACSetOwnershipInternal: Don't chown so often
It's better if we stat() file that we are about to chown() at
first and check if there's something we need to change. Not that
it would make much difference, but for the upcoming patches we
need to be doing stat() anyway.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 0dfe570..9b079e0 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -271,17 +271,18 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
path = src->path;
}
+ if (stat(path, &sb) < 0) {
+ virReportSystemError(errno, _("unable to stat: %s"), path);
+ return -1;
+ }
+
+ if (sb.st_uid == uid && sb.st_gid == gid) {
+ /* nothing to chown */
+ return 0;
+ }
+
rc = chown(path, uid, gid);
chown_errno = errno;
-
- if (rc < 0 &&
- stat(path, &sb) >= 0) {
- if (sb.st_uid == uid &&
- sb.st_gid == gid) {
- /* It's alright, there's nothing to change anyway. */
- return 0;
- }
- }
}
if (rc < 0) {
--
2.4.9
11:56 p.m.
New subject: [libvirt] [PATCH v1 03/23] virSecurityDACSetOwnershipInternal: Don't chown so often
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
It's better if we stat() file that we are about to chown() at
first and check if there's something we need to change. Not that
it would make much difference, but for the upcoming patches we
need to be doing stat() anyway.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 0dfe570..9b079e0 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -271,17 +271,18 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
path = src->path;
}
+ if (stat(path, &sb) < 0) {
+ virReportSystemError(errno, _("unable to stat: %s"), path);
+ return -1;
+ }
+
+ if (sb.st_uid == uid && sb.st_gid == gid) {
+ /* nothing to chown */
+ return 0;
+ }
+
rc = chown(path, uid, gid);
chown_errno = errno;
-
- if (rc < 0 &&
- stat(path, &sb) >= 0) {
- if (sb.st_uid == uid &&
- sb.st_gid == gid) {
- /* It's alright, there's nothing to change anyway. */
- return 0;
- }
- }
I don't believe chown_errno is necessary anymore either since a failure
in stat() won't overwrite errno.
Your call to keep it or not since it's also part of the "if" portion
(although that path doesn't overwrite either).
ACK -
John
}
if (rc < 0) {
7:29 a.m.
New subject: [libvirt] [PATCH v1 03/23] virSecurityDACSetOwnershipInternal: Don't chown so often
On Mon, Oct 12, 2015 at 12:25:48 +0200, Michal Privoznik wrote:
It's better if we stat() file that we are about to chown() at
first and check if there's something we need to change. Not that
it would make much difference, but for the upcoming patches we
need to be doing stat() anyway.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 0dfe570..9b079e0 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -271,17 +271,18 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
path = src->path;
}
+ if (stat(path, &sb) < 0) {
+ virReportSystemError(errno, _("unable to stat: %s"), path);
+ return -1;
+ }
I'd like to see a more specific error message in terms of what is
happening at the point stat was called. Otherwise the message might be
too confusing at this point since it might have failed due to lack of
permissions actually.
Peter
12:25 p.m.
New subject: [libvirt] [PATCH v1 04/23] security_dac: Introduce remember/recall stubs
These stubs will be worked in later. They merely lay out the
structure of the feature.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 9b079e0..9b53332 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -184,6 +184,51 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel,
return 0;
}
+/**
+ * virSecurityDACRememberLabel:
+ * @priv: driver's private data
+ * @path: path to the file
+ * @uid: user owning the @path
+ * @gid: group owning the @path
+ *
+ * Remember the owner of @path (represented by @uid:@gid).
+ *
+ * Returns: 0 on success, -1 on failure
+ */
+static int
+ATTRIBUTE_UNUSED
+virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
+ const char *path ATTRIBUTE_UNUSED,
+ uid_t uid ATTRIBUTE_UNUSED,
+ gid_t gid ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+/**
+ * virSecurityDACRecallLabel:
+ * @priv: driver's private data
+ * @path: path to the file
+ * @uid: user owning the @path
+ * @gid: group owning the @path
+ *
+ * Recall the previously recorded owner for the @path. However, it may happen
+ * that @path is still in use (e.g. by another domain). In that case, 1 is
+ * returned and caller should not relabel the @path.
+ *
+ * Returns: 1 if @path is still in use (@uid and @gid not touched)
+ * 0 if @path should be restored (@uid and @gid set)
+ * -1 on failure (@uid and @gid not touched)
+ */
+static int
+ATTRIBUTE_UNUSED
+virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
+ const char *path ATTRIBUTE_UNUSED,
+ uid_t *uid ATTRIBUTE_UNUSED,
+ gid_t *gid ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
static virSecurityDriverStatus
virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED)
--
2.4.9
12:22 a.m.
New subject: [libvirt] [PATCH v1 04/23] security_dac: Introduce remember/recall stubs
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
These stubs will be worked in later. They merely lay out the
structure of the feature.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 9b079e0..9b53332 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -184,6 +184,51 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel,
return 0;
}
+/**
+ * virSecurityDACRememberLabel:
+ * @priv: driver's private data
+ * @path: path to the file
+ * @uid: user owning the @path
+ * @gid: group owning the @path
+ *
+ * Remember the owner of @path (represented by @uid:@gid).
+ *
+ * Returns: 0 on success, -1 on failure
+ */
+static int
+ATTRIBUTE_UNUSED
Not that they stick around long, but didn't we have a recent bug/issue
with putting this in static functions? yeah - you wrote the patch about
apibuild.py, but I see that has to do with ATTRIBUTE_NONNULL(1)
I dunno, but at least I was paying attention to recent patches ;-)
ACK (whether you need to remove or not)
John
+virSecurityDACRememberLabel(virSecurityDACDataPtr priv
ATTRIBUTE_UNUSED,
+ const char *path ATTRIBUTE_UNUSED,
+ uid_t uid ATTRIBUTE_UNUSED,
+ gid_t gid ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+/**
+ * virSecurityDACRecallLabel:
+ * @priv: driver's private data
+ * @path: path to the file
+ * @uid: user owning the @path
+ * @gid: group owning the @path
+ *
+ * Recall the previously recorded owner for the @path. However, it may happen
+ * that @path is still in use (e.g. by another domain). In that case, 1 is
+ * returned and caller should not relabel the @path.
+ *
+ * Returns: 1 if @path is still in use (@uid and @gid not touched)
+ * 0 if @path should be restored (@uid and @gid set)
+ * -1 on failure (@uid and @gid not touched)
+ */
+static int
+ATTRIBUTE_UNUSED
+virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
+ const char *path ATTRIBUTE_UNUSED,
+ uid_t *uid ATTRIBUTE_UNUSED,
+ gid_t *gid ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
static virSecurityDriverStatus
virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED)
7:43 a.m.
New subject: [libvirt] [PATCH v1 04/23] security_dac: Introduce remember/recall stubs
On Mon, Oct 12, 2015 at 12:25:49 +0200, Michal Privoznik wrote:
These stubs will be worked in later. They merely lay out the
structure of the feature.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 9b079e0..9b53332 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -184,6 +184,51 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel,
return 0;
}
+/**
+ * virSecurityDACRememberLabel:
+ * @priv: driver's private data
+ * @path: path to the file
+ * @uid: user owning the @path
+ * @gid: group owning the @path
+ *
+ * Remember the owner of @path (represented by @uid:@gid).
+ *
+ * Returns: 0 on success, -1 on failure
+ */
+static int
+ATTRIBUTE_UNUSED
I'm not a fan of unused static functions. Since the patch also doesn't
contain any explanation in the commit message I'd suggest you merge it
to the patch that calls the functions.
+virSecurityDACRememberLabel(virSecurityDACDataPtr priv
ATTRIBUTE_UNUSED,
+ const char *path ATTRIBUTE_UNUSED,
I'm afraid you will eventually need a more complex data type than just
a string. We are already doing DAC ownership manipulation on gluster
volumes via the chown callback so we probably want to do the same thing
there too at least for disk images.
+ uid_t uid ATTRIBUTE_UNUSED,
+ gid_t gid ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
Peter
4:54 p.m.
New subject: [libvirt] [PATCH v1 04/23] security_dac: Introduce remember/recall stubs
On 16.10.2015 07:43, Peter Krempa wrote:
On Mon, Oct 12, 2015 at 12:25:49 +0200, Michal Privoznik wrote:
> These stubs will be worked in later. They merely lay out the
> structure of the feature.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/security/security_dac.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 9b079e0..9b53332 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -184,6 +184,51 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel,
> return 0;
> }
>
> +/**
> + * virSecurityDACRememberLabel:
> + * @priv: driver's private data
> + * @path: path to the file
> + * @uid: user owning the @path
> + * @gid: group owning the @path
> + *
> + * Remember the owner of @path (represented by @uid:@gid).
> + *
> + * Returns: 0 on success, -1 on failure
> + */
> +static int
> +ATTRIBUTE_UNUSED
I'm not a fan of unused static functions. Since the patch also doesn't
contain any explanation in the commit message I'd suggest you merge it
to the patch that calls the functions.
Fair enough.
> +virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
> + const char *path ATTRIBUTE_UNUSED,
I'm afraid you will eventually need a more complex data type than just
a string. We are already doing DAC ownership manipulation on gluster
volumes via the chown callback so we probably want to do the same thing
there too at least for disk images.
"more complex data type than just a string" - you mean for @path? Well,
that one is not interpreted by virtlockd. So maybe we can store there
"$domain.name:$disk.target" or something. However, since domain XML can
change over time I rather avoid using disk targets as it's files (or
gluster volumes) what we chown() really. Any bright idea?
Michal
12:25 p.m.
New subject: [libvirt] [PATCH v1 05/23] virSecurityDACSetOwnership: Pass virSecurityDACDataPtr
This is pure code adjustment. The structure is going to be needed
later as it will hold a reference that will be used to talk to
virtlockd. However, so far this is no functional change just code
preparation.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 9b53332..d951e21 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -356,10 +356,13 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
static int
-virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
+virSecurityDACSetOwnership(virSecurityDACDataPtr priv,
+ const char *path,
+ uid_t uid,
+ gid_t gid)
{
/* XXX record previous ownership */
- return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid);
+ return virSecurityDACSetOwnershipInternal(priv, NULL, path, uid, gid);
}
@@ -522,7 +525,7 @@ virSecurityDACSetSecurityHostdevLabelHelper(const char *file,
if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL))
return -1;
- return virSecurityDACSetOwnership(file, user, group);
+ return virSecurityDACSetOwnership(priv, file, user, group);
}
@@ -816,7 +819,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
switch ((virDomainChrType) dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
- ret = virSecurityDACSetOwnership(dev_source->data.file.path,
+ ret = virSecurityDACSetOwnership(priv, dev_source->data.file.path,
user, group);
break;
@@ -825,11 +828,11 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
(virAsprintf(&out, "%s.out", dev_source->data.file.path)
< 0))
goto done;
if (virFileExists(in) && virFileExists(out)) {
- if ((virSecurityDACSetOwnership(in, user, group) < 0) ||
- (virSecurityDACSetOwnership(out, user, group) < 0)) {
+ if ((virSecurityDACSetOwnership(priv, in, user, group) < 0) ||
+ (virSecurityDACSetOwnership(priv, out, user, group) < 0)) {
goto done;
}
- } else if (virSecurityDACSetOwnership(dev_source->data.file.path,
+ } else if (virSecurityDACSetOwnership(priv, dev_source->data.file.path,
user, group) < 0) {
goto done;
}
@@ -838,7 +841,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
case VIR_DOMAIN_CHR_TYPE_UNIX:
if (!dev_source->data.nix.listen) {
- if (virSecurityDACSetOwnership(dev_source->data.nix.path,
+ if (virSecurityDACSetOwnership(priv, dev_source->data.nix.path,
user, group) < 0)
goto done;
}
@@ -1103,19 +1106,19 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr,
return -1;
if (def->os.loader && def->os.loader->nvram &&
- virSecurityDACSetOwnership(def->os.loader->nvram, user, group) < 0)
+ virSecurityDACSetOwnership(priv, def->os.loader->nvram, user, group) <
0)
return -1;
if (def->os.kernel &&
- virSecurityDACSetOwnership(def->os.kernel, user, group) < 0)
+ virSecurityDACSetOwnership(priv, def->os.kernel, user, group) < 0)
return -1;
if (def->os.initrd &&
- virSecurityDACSetOwnership(def->os.initrd, user, group) < 0)
+ virSecurityDACSetOwnership(priv, def->os.initrd, user, group) < 0)
return -1;
if (def->os.dtb &&
- virSecurityDACSetOwnership(def->os.dtb, user, group) < 0)
+ virSecurityDACSetOwnership(priv, def->os.dtb, user, group) < 0)
return -1;
return 0;
@@ -1137,7 +1140,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr,
if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0)
return -1;
- return virSecurityDACSetOwnership(savefile, user, group);
+ return virSecurityDACSetOwnership(priv, savefile, user, group);
}
@@ -1456,7 +1459,7 @@ virSecurityDACDomainSetDirLabel(virSecurityManagerPtr mgr,
if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0)
return -1;
- return virSecurityDACSetOwnership(path, user, group);
+ return virSecurityDACSetOwnership(priv, path, user, group);
}
virSecurityDriver virSecurityDriverDAC = {
--
2.4.9
7:47 a.m.
New subject: [libvirt] [PATCH v1 05/23] virSecurityDACSetOwnership: Pass virSecurityDACDataPtr
On Mon, Oct 12, 2015 at 12:25:50 +0200, Michal Privoznik wrote:
This is pure code adjustment. The structure is going to be needed
later as it will hold a reference that will be used to talk to
virtlockd. However, so far this is no functional change just code
preparation.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
Since you are changing every caller you might as well as get rid of the
'virSecurityDACSetOwnership' wrapper completely and rename
virSecurityDACSetOwnershipInternal.
The original purpose of virSecurityDACSetOwnership was to avoid
changing every caller.
Peter
12:25 p.m.
New subject: [libvirt] [PATCH v1 06/23] virSecurityDACRestoreSecurityFileLabel: Pass virSecurityDACDataPtr
This is pure code adjustment. The structure is going to be needed
later as it will hold a reference that will be used to talk to
virtlockd. However, so far this is no functional change just code
preparation.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 44 ++++++++++++++++++++++++++------------------
1 file changed, 26 insertions(+), 18 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index d951e21..ccd9261 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -380,9 +380,10 @@ virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr
priv,
static int
-virSecurityDACRestoreSecurityFileLabel(const char *path)
+virSecurityDACRestoreSecurityFileLabel(virSecurityDACDataPtr priv,
+ const char *path)
{
- return virSecurityDACRestoreSecurityFileLabelInternal(NULL, NULL, path);
+ return virSecurityDACRestoreSecurityFileLabelInternal(priv, NULL, path);
}
@@ -664,27 +665,33 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreSecurityPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED,
const char *file,
- void *opaque ATTRIBUTE_UNUSED)
+ void *opaque)
{
- return virSecurityDACRestoreSecurityFileLabel(file);
+ virSecurityManagerPtr mgr = opaque;
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ return virSecurityDACRestoreSecurityFileLabel(priv, file);
}
static int
virSecurityDACRestoreSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED,
const char *file,
- void *opaque ATTRIBUTE_UNUSED)
+ void *opaque)
{
- return virSecurityDACRestoreSecurityFileLabel(file);
+ virSecurityManagerPtr mgr = opaque;
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ return virSecurityDACRestoreSecurityFileLabel(priv, file);
}
static int
virSecurityDACRestoreSecuritySCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED,
const char *file,
- void *opaque ATTRIBUTE_UNUSED)
+ void *opaque)
{
- return virSecurityDACRestoreSecurityFileLabel(file);
+ virSecurityManagerPtr mgr = opaque;
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ return virSecurityDACRestoreSecurityFileLabel(priv, file);
}
@@ -869,11 +876,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
}
static int
-virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
+virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def ATTRIBUTE_UNUSED,
virDomainChrDefPtr dev,
virDomainChrSourceDefPtr dev_source)
{
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityDeviceLabelDefPtr chr_seclabel = NULL;
char *in = NULL, *out = NULL;
int ret = -1;
@@ -888,7 +896,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
switch ((virDomainChrType) dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
- ret = virSecurityDACRestoreSecurityFileLabel(dev_source->data.file.path);
+ ret = virSecurityDACRestoreSecurityFileLabel(priv,
dev_source->data.file.path);
break;
case VIR_DOMAIN_CHR_TYPE_PIPE:
@@ -896,11 +904,11 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
(virAsprintf(&in, "%s.in", dev_source->data.file.path) <
0))
goto done;
if (virFileExists(in) && virFileExists(out)) {
- if ((virSecurityDACRestoreSecurityFileLabel(out) < 0) ||
- (virSecurityDACRestoreSecurityFileLabel(in) < 0)) {
+ if ((virSecurityDACRestoreSecurityFileLabel(priv, out) < 0) ||
+ (virSecurityDACRestoreSecurityFileLabel(priv, in) < 0)) {
goto done;
}
- } else if (virSecurityDACRestoreSecurityFileLabel(dev_source->data.file.path)
< 0) {
+ } else if (virSecurityDACRestoreSecurityFileLabel(priv,
dev_source->data.file.path) < 0) {
goto done;
}
ret = 0;
@@ -1026,19 +1034,19 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr,
}
if (def->os.loader && def->os.loader->nvram &&
- virSecurityDACRestoreSecurityFileLabel(def->os.loader->nvram) < 0)
+ virSecurityDACRestoreSecurityFileLabel(priv, def->os.loader->nvram) <
0)
rc = -1;
if (def->os.kernel &&
- virSecurityDACRestoreSecurityFileLabel(def->os.kernel) < 0)
+ virSecurityDACRestoreSecurityFileLabel(priv, def->os.kernel) < 0)
rc = -1;
if (def->os.initrd &&
- virSecurityDACRestoreSecurityFileLabel(def->os.initrd) < 0)
+ virSecurityDACRestoreSecurityFileLabel(priv, def->os.initrd) < 0)
rc = -1;
if (def->os.dtb &&
- virSecurityDACRestoreSecurityFileLabel(def->os.dtb) < 0)
+ virSecurityDACRestoreSecurityFileLabel(priv, def->os.dtb) < 0)
rc = -1;
return rc;
@@ -1154,7 +1162,7 @@ virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr,
if (!priv->dynamicOwnership)
return 0;
- return virSecurityDACRestoreSecurityFileLabel(savefile);
+ return virSecurityDACRestoreSecurityFileLabel(priv, savefile);
}
--
2.4.9
7:49 a.m.
New subject: [libvirt] [PATCH v1 06/23] virSecurityDACRestoreSecurityFileLabel: Pass virSecurityDACDataPtr
On Mon, Oct 12, 2015 at 12:25:51 +0200, Michal Privoznik wrote:
This is pure code adjustment. The structure is going to be needed
later as it will hold a reference that will be used to talk to
virtlockd. However, so far this is no functional change just code
preparation.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 44 ++++++++++++++++++++++++++------------------
1 file changed, 26 insertions(+), 18 deletions(-)
Same as with previous patch. virSecurityDACRestoreSecurityFileLabel was
purely introduced to avoid changing all callers. It can be dropped since
you modify the callers and
virSecurityDACRestoreSecurityFileLabelInternal renamed.
Peter
12:25 p.m.
New subject: [libvirt] [PATCH v1 07/23] security_dac: Limit usage of virSecurityDACSetOwnershipInternal
This function should really be called only when we want to change
ownership of a file (or disk source). Lets switch to calling a
wrapper function which will eventually record the current owner
of the file and call virSecurityDACSetOwnershipInternal
subsequently.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index ccd9261..a38c46c 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -357,12 +357,13 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
static int
virSecurityDACSetOwnership(virSecurityDACDataPtr priv,
+ virStorageSourcePtr src,
const char *path,
uid_t uid,
gid_t gid)
{
/* XXX record previous ownership */
- return virSecurityDACSetOwnershipInternal(priv, NULL, path, uid, gid);
+ return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
}
@@ -418,7 +419,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
return -1;
}
- return virSecurityDACSetOwnershipInternal(priv, src, NULL, user, group);
+ return virSecurityDACSetOwnership(priv, src, NULL, user, group);
}
@@ -526,7 +527,7 @@ virSecurityDACSetSecurityHostdevLabelHelper(const char *file,
if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL))
return -1;
- return virSecurityDACSetOwnership(priv, file, user, group);
+ return virSecurityDACSetOwnership(priv, NULL, file, user, group);
}
@@ -826,7 +827,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
switch ((virDomainChrType) dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
- ret = virSecurityDACSetOwnership(priv, dev_source->data.file.path,
+ ret = virSecurityDACSetOwnership(priv, NULL,
+ dev_source->data.file.path,
user, group);
break;
@@ -835,11 +837,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
(virAsprintf(&out, "%s.out", dev_source->data.file.path)
< 0))
goto done;
if (virFileExists(in) && virFileExists(out)) {
- if ((virSecurityDACSetOwnership(priv, in, user, group) < 0) ||
- (virSecurityDACSetOwnership(priv, out, user, group) < 0)) {
+ if ((virSecurityDACSetOwnership(priv, NULL, in, user, group) < 0) ||
+ (virSecurityDACSetOwnership(priv, NULL, out, user, group) < 0)) {
goto done;
}
- } else if (virSecurityDACSetOwnership(priv, dev_source->data.file.path,
+ } else if (virSecurityDACSetOwnership(priv, NULL,
+ dev_source->data.file.path,
user, group) < 0) {
goto done;
}
@@ -848,7 +851,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
case VIR_DOMAIN_CHR_TYPE_UNIX:
if (!dev_source->data.nix.listen) {
- if (virSecurityDACSetOwnership(priv, dev_source->data.nix.path,
+ if (virSecurityDACSetOwnership(priv, NULL,
+ dev_source->data.nix.path,
user, group) < 0)
goto done;
}
@@ -1114,19 +1118,23 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr,
return -1;
if (def->os.loader && def->os.loader->nvram &&
- virSecurityDACSetOwnership(priv, def->os.loader->nvram, user, group) <
0)
+ virSecurityDACSetOwnership(priv, NULL,
+ def->os.loader->nvram, user, group) < 0)
return -1;
if (def->os.kernel &&
- virSecurityDACSetOwnership(priv, def->os.kernel, user, group) < 0)
+ virSecurityDACSetOwnership(priv, NULL,
+ def->os.kernel, user, group) < 0)
return -1;
if (def->os.initrd &&
- virSecurityDACSetOwnership(priv, def->os.initrd, user, group) < 0)
+ virSecurityDACSetOwnership(priv, NULL,
+ def->os.initrd, user, group) < 0)
return -1;
if (def->os.dtb &&
- virSecurityDACSetOwnership(priv, def->os.dtb, user, group) < 0)
+ virSecurityDACSetOwnership(priv, NULL,
+ def->os.dtb, user, group) < 0)
return -1;
return 0;
@@ -1148,7 +1156,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr,
if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0)
return -1;
- return virSecurityDACSetOwnership(priv, savefile, user, group);
+ return virSecurityDACSetOwnership(priv, NULL, savefile, user, group);
}
@@ -1467,7 +1475,7 @@ virSecurityDACDomainSetDirLabel(virSecurityManagerPtr mgr,
if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0)
return -1;
- return virSecurityDACSetOwnership(priv, path, user, group);
+ return virSecurityDACSetOwnership(priv, NULL, path, user, group);
}
virSecurityDriver virSecurityDriverDAC = {
--
2.4.9
7:52 a.m.
New subject: [libvirt] [PATCH v1 07/23] security_dac: Limit usage of virSecurityDACSetOwnershipInternal
On Mon, Oct 12, 2015 at 12:25:52 +0200, Michal Privoznik wrote:
This function should really be called only when we want to change
ownership of a file (or disk source). Lets switch to calling a
wrapper function which will eventually record the current owner
of the file and call virSecurityDACSetOwnershipInternal
subsequently.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)
Ah, you want to create a full wrapper so that you can extend it
afterwards. Fair enough.
ACK to the previous ones too.
Peter
12:25 p.m.
New subject: [libvirt] [PATCH v1 08/23] security_dac: Plug in remember/recall APIs
Even though the APIs are not implemented yet, they create a
skeleton that can be filled in later.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 38 +++++++++++++++++++++++++++++++++-----
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index a38c46c..6c4e351 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -196,7 +196,6 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel,
* Returns: 0 on success, -1 on failure
*/
static int
-ATTRIBUTE_UNUSED
virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
const char *path ATTRIBUTE_UNUSED,
uid_t uid ATTRIBUTE_UNUSED,
@@ -221,7 +220,6 @@ virSecurityDACRememberLabel(virSecurityDACDataPtr priv
ATTRIBUTE_UNUSED,
* -1 on failure (@uid and @gid not touched)
*/
static int
-ATTRIBUTE_UNUSED
virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
const char *path ATTRIBUTE_UNUSED,
uid_t *uid ATTRIBUTE_UNUSED,
@@ -362,7 +360,22 @@ virSecurityDACSetOwnership(virSecurityDACDataPtr priv,
uid_t uid,
gid_t gid)
{
- /* XXX record previous ownership */
+ struct stat sb;
+
+ if (!path && src && src->path &&
+ virStorageSourceIsLocalStorage(src))
+ path = src->path;
+
+ if (path) {
+ if (stat(path, &sb) < 0) {
+ virReportSystemError(errno, _("unable to stat: %s"), path);
+ return -1;
+ }
+
+ if (virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid) < 0)
+ return -1;
+ }
+
return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
}
@@ -372,11 +385,26 @@ virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr
priv,
virStorageSourcePtr src,
const char *path)
{
+ int rv;
+ uid_t uid = 0; /* By default return to root:root */
+ gid_t gid = 0;
+
VIR_INFO("Restoring DAC user and group on '%s'",
NULLSTR(src ? src->path : path));
- /* XXX recall previous ownership */
- return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);
+ if (!path && src && src->path &&
+ virStorageSourceIsLocalStorage(src))
+ path = src->path;
+
+ if (path) {
+ rv = virSecurityDACRecallLabel(priv, path, &uid, &gid);
+ if (rv < 0)
+ return -1;
+ if (rv > 0)
+ return 0;
+ }
+
+ return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
}
--
2.4.9
12:38 a.m.
New subject: [libvirt] [PATCH v1 08/23] security_dac: Plug in remember/recall APIs
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
Even though the APIs are not implemented yet, they create a
skeleton that can be filled in later.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 38 +++++++++++++++++++++++++++++++++-----
1 file changed, 33 insertions(+), 5 deletions(-)
ACK 5 - 8
John
(I'll continue with this tomorrow - figured I'd at least get a start)
8:03 a.m.
New subject: [libvirt] [PATCH v1 08/23] security_dac: Plug in remember/recall APIs
On Mon, Oct 12, 2015 at 12:25:53 +0200, Michal Privoznik wrote:
Even though the APIs are not implemented yet, they create a
skeleton that can be filled in later.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 38 +++++++++++++++++++++++++++++++++-----
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index a38c46c..6c4e351 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -196,7 +196,6 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel,
* Returns: 0 on success, -1 on failure
*/
static int
-ATTRIBUTE_UNUSED
virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
const char *path ATTRIBUTE_UNUSED,
uid_t uid ATTRIBUTE_UNUSED,
I'd suggest you fold the addition of the stub into this patch. This
patch either does not do much explaining so the separate commits don't
make sense.
Peter
12:25 p.m.
New subject: [libvirt] [PATCH v1 09/23] lock_protocol: Add two new remote procedures
These procedures will be used to store and bring back security
labels. So far, the idea is that tuple (path, model, label) is
enough. Well, certainly for DAC and SELinux. The functions are:
VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL
VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL
Yeah, they really need that VIR_LOCK_SPACE_PROTOCOL_PROC prefix
due to way we call gendispatch.pl.
So the former will take the whole tuple and remember it. The
latter will then take just pair of (path, model) and return label
stored previously. Moreover, the return value of recall will be
important: value greater than zero means @path is still in use,
don't relabel it. Value of zero means @path is no longer used,
and a negative value means an error (e.g. @path not found, OOM,
etc.).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/lock_protocol-structs | 15 +++++++++++++++
src/locking/lock_daemon_dispatch.c | 21 +++++++++++++++++++++
src/locking/lock_protocol.x | 29 ++++++++++++++++++++++++++++-
3 files changed, 64 insertions(+), 1 deletion(-)
diff --git a/src/lock_protocol-structs b/src/lock_protocol-structs
index 8e8b84f..c45086b 100644
--- a/src/lock_protocol-structs
+++ b/src/lock_protocol-structs
@@ -43,6 +43,19 @@ struct virLockSpaceProtocolReleaseResourceArgs {
struct virLockSpaceProtocolCreateLockSpaceArgs {
virLockSpaceProtocolNonNullString path;
};
+struct virLockSpaceProtocolRememberSeclabelArgs {
+ virLockSpaceProtocolNonNullString model;
+ virLockSpaceProtocolNonNullString path;
+ virLockSpaceProtocolNonNullString label;
+};
+struct virLockSpaceProtocolRecallSeclabelArgs {
+ virLockSpaceProtocolNonNullString model;
+ virLockSpaceProtocolNonNullString path;
+};
+struct virLockSpaceProtocolRecallSeclabelRet {
+ virLockSpaceProtocolString label;
+ u_int ret;
+};
enum virLockSpaceProtocolProcedure {
VIR_LOCK_SPACE_PROTOCOL_PROC_REGISTER = 1,
VIR_LOCK_SPACE_PROTOCOL_PROC_RESTRICT = 2,
@@ -52,4 +65,6 @@ enum virLockSpaceProtocolProcedure {
VIR_LOCK_SPACE_PROTOCOL_PROC_ACQUIRE_RESOURCE = 6,
VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE = 7,
VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8,
+ VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL = 9,
+ VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL = 10,
};
diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c
index 1b479db..2d0bd81 100644
--- a/src/locking/lock_daemon_dispatch.c
+++ b/src/locking/lock_daemon_dispatch.c
@@ -430,3 +430,24 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServerPtr server
ATTRIBUTE_UNU
virMutexUnlock(&priv->lock);
return rv;
}
+
+static int
+virLockSpaceProtocolDispatchRememberSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client
ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virLockSpaceProtocolRememberSeclabelArgs
*args ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+static int
+virLockSpaceProtocolDispatchRecallSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client
ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+ virLockSpaceProtocolRecallSeclabelArgs *args
ATTRIBUTE_UNUSED,
+ virLockSpaceProtocolRecallSeclabelRet *ret
ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
diff --git a/src/locking/lock_protocol.x b/src/locking/lock_protocol.x
index a77a784..bac4f0c 100644
--- a/src/locking/lock_protocol.x
+++ b/src/locking/lock_protocol.x
@@ -71,6 +71,21 @@ struct virLockSpaceProtocolCreateLockSpaceArgs {
virLockSpaceProtocolNonNullString path;
};
+struct virLockSpaceProtocolRememberSeclabelArgs {
+ virLockSpaceProtocolNonNullString model;
+ virLockSpaceProtocolNonNullString path;
+ virLockSpaceProtocolNonNullString label;
+};
+
+struct virLockSpaceProtocolRecallSeclabelArgs {
+ virLockSpaceProtocolNonNullString model;
+ virLockSpaceProtocolNonNullString path;
+};
+
+struct virLockSpaceProtocolRecallSeclabelRet {
+ virLockSpaceProtocolString label;
+ unsigned int ret;
+};
/* Define the program number, protocol version and procedure numbers here. */
const VIR_LOCK_SPACE_PROTOCOL_PROGRAM = 0xEA7BEEF;
@@ -149,5 +164,17 @@ enum virLockSpaceProtocolProcedure {
* @generate: none
* @acl: none
*/
- VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8
+ VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8,
+
+ /**
+ * @generate: none
+ * @acl: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL = 9,
+
+ /**
+ * @generate: none
+ * @acl: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL = 10
};
--
2.4.9
9:17 a.m.
New subject: [libvirt] [PATCH v1 09/23] lock_protocol: Add two new remote procedures
On Mon, Oct 12, 2015 at 12:25:54 +0200, Michal Privoznik wrote:
These procedures will be used to store and bring back security
labels. So far, the idea is that tuple (path, model, label) is
enough. Well, certainly for DAC and SELinux. The functions are:
I'm afraid that a 'path' per-se isn't enough to express storage
locations in a unique way. Said that a string is fine though, but I'd
rather call it 'identifier' or something similar and explicitly
document some formats for possibly other storage systems that might use
security labeling but don't have a local representation in the host.
I'd imagine something like:
STORAGE_SYSTEM_UUID:VOLUME_UUID
and perhaps
HOST_UUID:/path for local files
One other thing to consider is that if this approach will be used across
multiple hosts the paths although equal string-wise might not result
being the same file. Not honoring that would result into security
implications.
VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL
VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL
Yeah, they really need that VIR_LOCK_SPACE_PROTOCOL_PROC prefix
due to way we call gendispatch.pl.
So the former will take the whole tuple and remember it. The
latter will then take just pair of (path, model) and return label
stored previously. Moreover, the return value of recall will be
important: value greater than zero means @path is still in use,
don't relabel it. Value of zero means @path is no longer used,
and a negative value means an error (e.g. @path not found, OOM,
etc.).
I wanted to suggest other constraints but found out that they are
actually documented in the patch that adds the API for this RPC.
Peter
4:54 p.m.
New subject: [libvirt] [PATCH v1 09/23] lock_protocol: Add two new remote procedures
On 16.10.2015 09:17, Peter Krempa wrote:
On Mon, Oct 12, 2015 at 12:25:54 +0200, Michal Privoznik wrote:
> These procedures will be used to store and bring back security
> labels. So far, the idea is that tuple (path, model, label) is
> enough. Well, certainly for DAC and SELinux. The functions are:
I'm afraid that a 'path' per-se isn't enough to express storage
locations in a unique way. Said that a string is fine though, but I'd
rather call it 'identifier' or something similar and explicitly
document some formats for possibly other storage systems that might use
security labeling but don't have a local representation in the host.
I'd imagine something like:
STORAGE_SYSTEM_UUID:VOLUME_UUID
I can imagine this, okay.
and perhaps
HOST_UUID:/path for local files
Hm... what good it will have to store UUID among with the path? I mean,
we can't know if two paths from two different hosts is the same file or
not anyway.
One other thing to consider is that if this approach will be used across
multiple hosts the paths although equal string-wise might not result
being the same file. Not honoring that would result into security
implications.
What security implications do you have in mind?
On the other hand, I just realized that this approach will not fly. I
mean, virtlockd is running per host. So consider a file on NFS share,
and to make things more complicated assume it's accessible under
different paths from two different hosts. There is still a race between
those two virtlockd-s - they will not mutually exclude each other and
therefore can store already chown()-ed owership info. For instance:
host1: stat()
host1: store ownership (e.g. 100:20)
host1: chown (e.g. to 20:20)
host2: stat()
host2: store ownership (20:20)
host1: recall ownership (100:20)
host1: restore ownership (100:20)
host2: recall ownership (20:20)
host2: restore ownership (20:20)
Both hosts think that they are the last ones to use the file, therefore
they restore ownerships. However, after all the original ownership is
not preserved.
One way out of this could be that we already require virtlockd in order
to work properly that lockspace is on a shared mountpoint accessible by
both hosts. So instead of keeping seclabel space in memory, we can load
& store it within a file on that mountpoint and guard accesses via
flock(). But this is rather unpleasant as we would have to parse and
format the whole seclabel space just to update a refcounter to some entry.
The other approach would be to have a seclabel space as a directory on
the shared mountpoint and create some file (with internal structure) per
path remembered. The internal structure of the file would then contain
tuple (model, seclabel, refcount) for each model. When updating the
refcount, only the file would need to be flock()-ed getting us much
higher throughput. But we would still need to parse and format a file on
each access (even though the file would be much smaller).
I'm out of ideas here. Anybody has some?
Michal
9:58 p.m.
New subject: [libvirt] [PATCH v1 09/23] lock_protocol: Add two new remote procedures
On 10/16/2015 10:54 AM, Michal Privoznik wrote:
On 16.10.2015 09:17, Peter Krempa wrote:
> On Mon, Oct 12, 2015 at 12:25:54 +0200, Michal Privoznik wrote:
>> These procedures will be used to store and bring back security
>> labels. So far, the idea is that tuple (path, model, label) is
>> enough. Well, certainly for DAC and SELinux. The functions are:
>
> I'm afraid that a 'path' per-se isn't enough to express storage
> locations in a unique way. Said that a string is fine though, but I'd
> rather call it 'identifier' or something similar and explicitly
> document some formats for possibly other storage systems that might use
> security labeling but don't have a local representation in the host.
>
> I'd imagine something like:
> STORAGE_SYSTEM_UUID:VOLUME_UUID
I can imagine this, okay.
> and perhaps
> HOST_UUID:/path for local files
Hm... what good it will have to store UUID among with the path? I mean,
we can't know if two paths from two different hosts is the same file or
not anyway.
>
> One other thing to consider is that if this approach will be used across
> multiple hosts the paths although equal string-wise might not result
> being the same file. Not honoring that would result into security
> implications.
What security implications do you have in mind?
On the other hand, I just realized that this approach will not fly. I
mean, virtlockd is running per host. So consider a file on NFS share,
and to make things more complicated assume it's accessible under
different paths from two different hosts. There is still a race between
those two virtlockd-s - they will not mutually exclude each other and
therefore can store already chown()-ed owership info. For instance:
host1: stat()
host1: store ownership (e.g. 100:20)
host1: chown (e.g. to 20:20)
host2: stat()
host2: store ownership (20:20)
host1: recall ownership (100:20)
host1: restore ownership (100:20)
host2: recall ownership (20:20)
host2: restore ownership (20:20)
Both hosts think that they are the last ones to use the file, therefore
they restore ownerships. However, after all the original ownership is
not preserved.
Ah - the age old problem of networked and shared storage... And it's
compounded by two hosts that don't know they are modifying the same
resource. If realistically the server is the 'only' one to know, then
who's problem is this to solve?
Say nothing of choosing NFS... For this one you have to also consider
root_squash vs. non root squash environment. The attempt to change
anything is managed by the server, but for the sake of your argument
it's set up to allow "no_root_squash" - meaning clients can have more
control...
In your scenario, H2 never chown()'s... So was it trying set to (20:20)
- in which case, why would it do that since the stat it would get would
show (20:20)... Or does it try to set say (40:20). If the latter, then
H1 is going to have access problems as soon as H2 is successful. If H1
then goes to restore and finds the current stat of (40:20), but it
believes it set (20:20), then should it restore? Or does it "try to"
restore anyway and then let virtlockd hold onto the restore until H2 is
done? Eventually H2 will restore to (20:20) at which time whatever is
waiting to restore H1's value finds the (20:20) and can finally
successfully restore back to (100:20)... Conversely if H2 did save and
chown (20:20), but when it goes to restore (20:20) and finds (100:20),
you wouldn't want some thread hanging around... However, in this case
if the "store" == "restore" and the current is something else, then
perhaps the restore gets dropped.
Maybe a few drinks will help the thought process more ;-)
John
One way out of this could be that we already require virtlockd in order
to work properly that lockspace is on a shared mountpoint accessible by
both hosts. So instead of keeping seclabel space in memory, we can load
& store it within a file on that mountpoint and guard accesses via
flock(). But this is rather unpleasant as we would have to parse and
format the whole seclabel space just to update a refcounter to some entry.
The other approach would be to have a seclabel space as a directory on
the shared mountpoint and create some file (with internal structure) per
path remembered. The internal structure of the file would then contain
tuple (model, seclabel, refcount) for each model. When updating the
refcount, only the file would need to be flock()-ed getting us much
higher throughput. But we would still need to parse and format a file on
each access (even though the file would be much smaller).
I'm out of ideas here. Anybody has some?
Michal
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:49 a.m.
New subject: [libvirt] [PATCH v1 09/23] lock_protocol: Add two new remote procedures
On 16.10.2015 21:58, John Ferlan wrote:
On 10/16/2015 10:54 AM, Michal Privoznik wrote:
> On 16.10.2015 09:17, Peter Krempa wrote:
>> On Mon, Oct 12, 2015 at 12:25:54 +0200, Michal Privoznik wrote:
>>> These procedures will be used to store and bring back security
>>> labels. So far, the idea is that tuple (path, model, label) is
>>> enough. Well, certainly for DAC and SELinux. The functions are:
>>
>> I'm afraid that a 'path' per-se isn't enough to express storage
>> locations in a unique way. Said that a string is fine though, but I'd
>> rather call it 'identifier' or something similar and explicitly
>> document some formats for possibly other storage systems that might use
>> security labeling but don't have a local representation in the host.
>>
>> I'd imagine something like:
>> STORAGE_SYSTEM_UUID:VOLUME_UUID
>
> I can imagine this, okay.
>
>> and perhaps
>> HOST_UUID:/path for local files
>
> Hm... what good it will have to store UUID among with the path? I mean,
> we can't know if two paths from two different hosts is the same file or
> not anyway.
>
>>
>> One other thing to consider is that if this approach will be used across
>> multiple hosts the paths although equal string-wise might not result
>> being the same file. Not honoring that would result into security
>> implications.
>
> What security implications do you have in mind?
>
> On the other hand, I just realized that this approach will not fly. I
> mean, virtlockd is running per host. So consider a file on NFS share,
> and to make things more complicated assume it's accessible under
> different paths from two different hosts. There is still a race between
> those two virtlockd-s - they will not mutually exclude each other and
> therefore can store already chown()-ed owership info. For instance:
>
> host1: stat()
> host1: store ownership (e.g. 100:20)
> host1: chown (e.g. to 20:20)
>
> host2: stat()
> host2: store ownership (20:20)
host2: chown (e.g. to 20:40)
>
> host1: recall ownership (100:20)
> host1: restore ownership (100:20)
(this will cut off host2)
>
> host2: recall ownership (20:20)
> host2: restore ownership (20:20)
>
> Both hosts think that they are the last ones to use the file, therefore
> they restore ownerships. However, after all the original ownership is
> not preserved.
Ah - the age old problem of networked and shared storage... And it's
compounded by two hosts that don't know they are modifying the same
resource. If realistically the server is the 'only' one to know, then
who's problem is this to solve?
Say nothing of choosing NFS... For this one you have to also consider
root_squash vs. non root squash environment. The attempt to change
anything is managed by the server, but for the sake of your argument
it's set up to allow "no_root_squash" - meaning clients can have more
control...
In your scenario, H2 never chown()'s... So was it trying set to (20:20)
- in which case, why would it do that since the stat it would get would
show (20:20)... Or does it try to set say (40:20). If the latter, then
H1 is going to have access problems as soon as H2 is successful. If H1
then goes to restore and finds the current stat of (40:20), but it
believes it set (20:20), then should it restore? Or does it "try to"
restore anyway and then let virtlockd hold onto the restore until H2 is
done? Eventually H2 will restore to (20:20) at which time whatever is
waiting to restore H1's value finds the (20:20) and can finally
successfully restore back to (100:20)... Conversely if H2 did save and
chown (20:20), but when it goes to restore (20:20) and finds (100:20),
you wouldn't want some thread hanging around... However, in this case
if the "store" == "restore" and the current is something else, then
perhaps the restore gets dropped.
Oh, yeah. H2 should chown() too. But not necessarily cutting off H1 (it
still can have access via user while H2 via group part of the permission).
And we should not consider just DAC labels here. The framework should be
capable of storing SELinux labels too. In that world, different labels
(or context ranges) do not necessarily mean EPERM like in DAC. Therefore
I think we should not check if current label is the one we set earlier.
Moreover, even if we do check on restore if the seclabel has changed
since the time we set it and if it has do not restore - in my scenario
the file would has the wrong ownership at the end anyway, wouldn't it? I
mean, at restore, h1 finds the seclabel has changed (it's 20:40 and it
has set it to 20:20), so it leaves the file alone. Then, h2 comes around
and finds the seclabel has NOT changed, so it restores it to what it
thinks was the original label (20:20).
Maybe a few drinks will help the thought process more ;-)
John
>
> One way out of this could be that we already require virtlockd in order
> to work properly that lockspace is on a shared mountpoint accessible by
> both hosts. So instead of keeping seclabel space in memory, we can load
> & store it within a file on that mountpoint and guard accesses via
> flock(). But this is rather unpleasant as we would have to parse and
> format the whole seclabel space just to update a refcounter to some entry.
>
> The other approach would be to have a seclabel space as a directory on
> the shared mountpoint and create some file (with internal structure) per
> path remembered. The internal structure of the file would then contain
> tuple (model, seclabel, refcount) for each model. When updating the
> refcount, only the file would need to be flock()-ed getting us much
> higher throughput. But we would still need to parse and format a file on
> each access (even though the file would be much smaller).
I've given this some thoughts and even after few beers (reaching Ballmer
peak) it still seems like good idea.
Michal
3:12 p.m.
New subject: [libvirt] [PATCH v1 09/23] lock_protocol: Add two new remote procedures
On 10/19/2015 01:49 AM, Michal Privoznik wrote:
On 16.10.2015 21:58, John Ferlan wrote:
>
>
> On 10/16/2015 10:54 AM, Michal Privoznik wrote:
>> On 16.10.2015 09:17, Peter Krempa wrote:
>>> On Mon, Oct 12, 2015 at 12:25:54 +0200, Michal Privoznik wrote:
>>>> These procedures will be used to store and bring back security
>>>> labels. So far, the idea is that tuple (path, model, label) is
>>>> enough. Well, certainly for DAC and SELinux. The functions are:
>>>
>>> I'm afraid that a 'path' per-se isn't enough to express
storage
>>> locations in a unique way. Said that a string is fine though, but I'd
>>> rather call it 'identifier' or something similar and explicitly
>>> document some formats for possibly other storage systems that might use
>>> security labeling but don't have a local representation in the host.
>>>
>>> I'd imagine something like:
>>> STORAGE_SYSTEM_UUID:VOLUME_UUID
>>
>> I can imagine this, okay.
>>
>>> and perhaps
>>> HOST_UUID:/path for local files
>>
>> Hm... what good it will have to store UUID among with the path? I mean,
>> we can't know if two paths from two different hosts is the same file or
>> not anyway.
>>
>>>
>>> One other thing to consider is that if this approach will be used across
>>> multiple hosts the paths although equal string-wise might not result
>>> being the same file. Not honoring that would result into security
>>> implications.
>>
>> What security implications do you have in mind?
>>
>> On the other hand, I just realized that this approach will not fly. I
>> mean, virtlockd is running per host. So consider a file on NFS share,
>> and to make things more complicated assume it's accessible under
>> different paths from two different hosts. There is still a race between
>> those two virtlockd-s - they will not mutually exclude each other and
>> therefore can store already chown()-ed owership info. For instance:
>>
>> host1: stat()
>> host1: store ownership (e.g. 100:20)
>> host1: chown (e.g. to 20:20)
>>
>> host2: stat()
>> host2: store ownership (20:20)
host2: chown (e.g. to 20:40)
>>
>> host1: recall ownership (100:20)
>> host1: restore ownership (100:20)
(this will cut off host2)
>>
>> host2: recall ownership (20:20)
>> host2: restore ownership (20:20)
>>
>> Both hosts think that they are the last ones to use the file, therefore
>> they restore ownerships. However, after all the original ownership is
>> not preserved.
>
>
> Ah - the age old problem of networked and shared storage... And it's
> compounded by two hosts that don't know they are modifying the same
> resource. If realistically the server is the 'only' one to know, then
> who's problem is this to solve?
>
> Say nothing of choosing NFS... For this one you have to also consider
> root_squash vs. non root squash environment. The attempt to change
> anything is managed by the server, but for the sake of your argument
> it's set up to allow "no_root_squash" - meaning clients can have more
> control...
>
> In your scenario, H2 never chown()'s... So was it trying set to (20:20)
> - in which case, why would it do that since the stat it would get would
> show (20:20)... Or does it try to set say (40:20). If the latter, then
> H1 is going to have access problems as soon as H2 is successful. If H1
> then goes to restore and finds the current stat of (40:20), but it
> believes it set (20:20), then should it restore? Or does it "try to"
> restore anyway and then let virtlockd hold onto the restore until H2 is
> done? Eventually H2 will restore to (20:20) at which time whatever is
> waiting to restore H1's value finds the (20:20) and can finally
> successfully restore back to (100:20)... Conversely if H2 did save and
> chown (20:20), but when it goes to restore (20:20) and finds (100:20),
> you wouldn't want some thread hanging around... However, in this case
> if the "store" == "restore" and the current is something else,
then
> perhaps the restore gets dropped.
Oh, yeah. H2 should chown() too. But not necessarily cutting off H1 (it
still can have access via user while H2 via group part of the permission).
And we should not consider just DAC labels here. The framework should be
capable of storing SELinux labels too. In that world, different labels
(or context ranges) do not necessarily mean EPERM like in DAC. Therefore
I think we should not check if current label is the one we set earlier.
Understood that my thoughts were DAC specific... Although a comparison
routine for the specific driver could be called from a generic spot to
determine if it's changed - it was just easier to type it thinking only
DAC though.
Also, consider that when using DAC with uid:gid mapping, the uid:gid on
the client*s* would need to match the uid:gid on the server and the NFS
server would need to be configured properly to allow client*s* to
change. Otherwise, there's some perhaps unintended consequences.
Moreover, even if we do check on restore if the seclabel has changed
since the time we set it and if it has do not restore - in my scenario
the file would has the wrong ownership at the end anyway, wouldn't it? I
mean, at restore, h1 finds the seclabel has changed (it's 20:40 and it
has set it to 20:20), so it leaves the file alone. Then, h2 comes around
and finds the seclabel has NOT changed, so it restores it to what it
thinks was the original label (20:20).
The nuance for h1 is can it or should it 'wait' to find the label has
reverted back to what it set it to? Where the wait thread lives I
hadn't thought too much about, but perhaps would be "no different" than
some other libvirt thread that waits for something and then sends an
event when done. It's not a fool proof method for sure, but throwing
spaghetti on the wall to see if anything sticks.
>
> Maybe a few drinks will help the thought process more ;-)
>
> John
>>
>> One way out of this could be that we already require virtlockd in order
>> to work properly that lockspace is on a shared mountpoint accessible by
>> both hosts. So instead of keeping seclabel space in memory, we can load
>> & store it within a file on that mountpoint and guard accesses via
>> flock(). But this is rather unpleasant as we would have to parse and
>> format the whole seclabel space just to update a refcounter to some entry.
>>
>> The other approach would be to have a seclabel space as a directory on
>> the shared mountpoint and create some file (with internal structure) per
>> path remembered. The internal structure of the file would then contain
>> tuple (model, seclabel, refcount) for each model. When updating the
>> refcount, only the file would need to be flock()-ed getting us much
>> higher throughput. But we would still need to parse and format a file on
>> each access (even though the file would be much smaller).
I've given this some thoughts and even after few beers (reaching Ballmer
peak) it still seems like good idea.
So a solution to the NFS problem (or other such shared filesystem) is to
carve out space on that file system? If it's NFS and root squashed -
under which uid:gid does that happen? How "large" is it? How do you
"guarantee" something doesn't come along and reap it or change it's
access?
I would think it should be up to the networked file system to provide
the mechanism for secure access, change, etc. IOW: Is this really
libvirt's problem to solve? I'm not sure every nuance of every target
server file system could be dealt with properly by some generic method.
If someone wants to use NFS and they want this rollback, can we convince
them to run virtlockd on their NFS Server? And then use that connection
in order to manage things if they really care about this label rollback
feature? Are we building something too complex to use and/or describe?
Do we really want to be the "owner" of the security problem?
John
4:24 p.m.
New subject: [libvirt] [PATCH v1 09/23] lock_protocol: Add two new remote procedures
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
These procedures will be used to store and bring back security
labels. So far, the idea is that tuple (path, model, label) is
enough. Well, certainly for DAC and SELinux. The functions are:
VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL
VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL
Yeah, they really need that VIR_LOCK_SPACE_PROTOCOL_PROC prefix
due to way we call gendispatch.pl.
So the former will take the whole tuple and remember it. The
latter will then take just pair of (path, model) and return label
stored previously. Moreover, the return value of recall will be
important: value greater than zero means @path is still in use,
don't relabel it. Value of zero means @path is no longer used,
and a negative value means an error (e.g. @path not found, OOM,
etc.).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/lock_protocol-structs | 15 +++++++++++++++
src/locking/lock_daemon_dispatch.c | 21 +++++++++++++++++++++
src/locking/lock_protocol.x | 29 ++++++++++++++++++++++++++++-
3 files changed, 64 insertions(+), 1 deletion(-)
Disclaimer: My working knowledge of seclabels is limited. I know what
they are, but never dug into the details. They remind me of an OpenVMS
technology that I knew quite well years ago (known as ACL's)... However,
it seems in general they are passed around as character strings, so
that's my working idea...
As I started thinking about this - essentially you're looking to
generate a object in virtlockd that has a unique name for which you'd
like to store/save of some character string context so that at some
point in time that context could be restored.
Without looking too far ahead, I'm assuming this concept is to handle
the 'dynamic' seclabel type's and the static relabel='yes' types in
order to allow a relabel after libvirt is done. So you're then saving
the "model" (dac, selinux, apparmor) and provided "label" (specific
to
the model).
I think Peter covered some issues around the 'path' which could/should
be able to describe the 'identifier' object.
Again, not looking forward and just thinking in terms of what's in a
"<seclabel.../>"... Since it's possible to have multiple seclabels
for
each 'path'/'identifier' - I assume you must have built in some logic to
first search on the identifier, then if found using the model in order
to determine if something was already defined for that. If so, you're
returning '1' for already in use; otherwise, you create and return '0'
on success and '-1' on failure. So in a way, the code would seemingly
already have a way to take a unique object name and search to determine
if the 'payload' type already exists and make decisions from there.
Since you've set @generate to none that says to me the virtlockd would
then have some "built-in" knowledge about the format of what is being
received so that it can make decisions about what to return. That got me
to thinking about other uses...
Although the design center is security labels for disks and the naming
is centered around that, it could be a security label for anything,
correct? Or thinking more generically, perhaps a MAC for a network?
Some network filter? A secret for something? Or perhaps a resource (such
as disk) which is currently being used by a pool in which case we don't
want some other pool to use the same resource (yes, I have a bz on that
and IIRC there's a different one dealing with disk usage across domains).
Although with multiple uses of the same "object by name", it seems some
sort of tagging on the payload would be required and a way to handle the
"I don't know this type of 'payload'". I guess what I'm thinking
we
should avoid is code copy-n-paste bloat for the "next" remember/recall
item. Although yes, easier to debug and perhaps easier to design if we
don't have to think about other object uses or the future - I would
think we have enough current examples that we could come up with
something generic.
In the end perhaps something along the lines of for remember.
{
*String unique_id; /* char string to uniquely id the object */
int payload; /* Payload type - enum {SECLABEL, ...} */
*String object1; /* Currently "model" - payload specific search */
*String object2; /* Currently "label" - whatever it is to store */
}
John
diff --git a/src/lock_protocol-structs b/src/lock_protocol-structs
index 8e8b84f..c45086b 100644
--- a/src/lock_protocol-structs
+++ b/src/lock_protocol-structs
@@ -43,6 +43,19 @@ struct virLockSpaceProtocolReleaseResourceArgs {
struct virLockSpaceProtocolCreateLockSpaceArgs {
virLockSpaceProtocolNonNullString path;
};
+struct virLockSpaceProtocolRememberSeclabelArgs {
+ virLockSpaceProtocolNonNullString model;
+ virLockSpaceProtocolNonNullString path;
+ virLockSpaceProtocolNonNullString label;
+};
+struct virLockSpaceProtocolRecallSeclabelArgs {
+ virLockSpaceProtocolNonNullString model;
+ virLockSpaceProtocolNonNullString path;
+};
+struct virLockSpaceProtocolRecallSeclabelRet {
+ virLockSpaceProtocolString label;
+ u_int ret;
+};
enum virLockSpaceProtocolProcedure {
VIR_LOCK_SPACE_PROTOCOL_PROC_REGISTER = 1,
VIR_LOCK_SPACE_PROTOCOL_PROC_RESTRICT = 2,
@@ -52,4 +65,6 @@ enum virLockSpaceProtocolProcedure {
VIR_LOCK_SPACE_PROTOCOL_PROC_ACQUIRE_RESOURCE = 6,
VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE = 7,
VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8,
+ VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL = 9,
+ VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL = 10,
};
diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c
index 1b479db..2d0bd81 100644
--- a/src/locking/lock_daemon_dispatch.c
+++ b/src/locking/lock_daemon_dispatch.c
@@ -430,3 +430,24 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServerPtr server
ATTRIBUTE_UNU
virMutexUnlock(&priv->lock);
return rv;
}
+
+static int
+virLockSpaceProtocolDispatchRememberSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client
ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virLockSpaceProtocolRememberSeclabelArgs
*args ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+static int
+virLockSpaceProtocolDispatchRecallSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client
ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+ virLockSpaceProtocolRecallSeclabelArgs *args
ATTRIBUTE_UNUSED,
+ virLockSpaceProtocolRecallSeclabelRet *ret
ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
diff --git a/src/locking/lock_protocol.x b/src/locking/lock_protocol.x
index a77a784..bac4f0c 100644
--- a/src/locking/lock_protocol.x
+++ b/src/locking/lock_protocol.x
@@ -71,6 +71,21 @@ struct virLockSpaceProtocolCreateLockSpaceArgs {
virLockSpaceProtocolNonNullString path;
};
+struct virLockSpaceProtocolRememberSeclabelArgs {
+ virLockSpaceProtocolNonNullString model;
+ virLockSpaceProtocolNonNullString path;
+ virLockSpaceProtocolNonNullString label;
+};
+
+struct virLockSpaceProtocolRecallSeclabelArgs {
+ virLockSpaceProtocolNonNullString model;
+ virLockSpaceProtocolNonNullString path;
+};
+
+struct virLockSpaceProtocolRecallSeclabelRet {
+ virLockSpaceProtocolString label;
+ unsigned int ret;
+};
/* Define the program number, protocol version and procedure numbers here. */
const VIR_LOCK_SPACE_PROTOCOL_PROGRAM = 0xEA7BEEF;
@@ -149,5 +164,17 @@ enum virLockSpaceProtocolProcedure {
* @generate: none
* @acl: none
*/
- VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8
+ VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8,
+
+ /**
+ * @generate: none
+ * @acl: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL = 9,
+
+ /**
+ * @generate: none
+ * @acl: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL = 10
};
12:25 p.m.
New subject: [libvirt] [PATCH v1 10/23] lock_daemon: Switch to wrapper locking functions
Lets use wrapper functions virLockDaemonLock and
virLockDaemonUnlock instead of virMutexLock and virMutexUnlock.
This has no functional impact, but it's easier to read (at least
for me).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_daemon.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 56db959..3984b4d 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -125,6 +125,17 @@ virLockDaemonFree(virLockDaemonPtr lockd)
VIR_FREE(lockd);
}
+static void
+virLockDaemonLock(virLockDaemonPtr lockd)
+{
+ virMutexLock(&lockd->lock);
+}
+
+static void
+virLockDaemonUnlock(virLockDaemonPtr lockd)
+{
+ virMutexUnlock(&lockd->lock);
+}
static void virLockDaemonLockSpaceDataFree(void *data,
const void *key ATTRIBUTE_UNUSED)
@@ -275,9 +286,9 @@ int virLockDaemonAddLockSpace(virLockDaemonPtr lockd,
virLockSpacePtr lockspace)
{
int ret;
- virMutexLock(&lockd->lock);
+ virLockDaemonLock(lockd);
ret = virHashAddEntry(lockd->lockspaces, path, lockspace);
- virMutexUnlock(&lockd->lock);
+ virLockDaemonUnlock(lockd);
return ret;
}
@@ -285,12 +296,12 @@ virLockSpacePtr virLockDaemonFindLockSpace(virLockDaemonPtr lockd,
const char *path)
{
virLockSpacePtr lockspace;
- virMutexLock(&lockd->lock);
+ virLockDaemonLock(lockd);
if (path && STRNEQ(path, ""))
lockspace = virHashLookup(lockd->lockspaces, path);
else
lockspace = lockd->defaultLockspace;
- virMutexUnlock(&lockd->lock);
+ virLockDaemonUnlock(lockd);
return lockspace;
}
@@ -675,14 +686,14 @@ virLockDaemonClientFree(void *opaque)
/* Release all locks associated with this
* owner in all lockspaces */
- virMutexLock(&lockDaemon->lock);
+ virLockDaemonLock(lockDaemon);
virHashForEach(lockDaemon->lockspaces,
virLockDaemonClientReleaseLockspace,
&data);
virLockDaemonClientReleaseLockspace(lockDaemon->defaultLockspace,
"",
&data);
- virMutexUnlock(&lockDaemon->lock);
+ virLockDaemonUnlock(lockDaemon);
/* If the client had some active leases when it
* closed the connection, we must kill it off
--
2.4.9
8:27 a.m.
New subject: [libvirt] [PATCH v1 10/23] lock_daemon: Switch to wrapper locking functions
On Mon, Oct 12, 2015 at 12:25:55 +0200, Michal Privoznik wrote:
Lets use wrapper functions virLockDaemonLock and
virLockDaemonUnlock instead of virMutexLock and virMutexUnlock.
This has no functional impact, but it's easier to read (at least
for me).
Actually we removed such helpers in favor of virObjectLock in most
places.
On the other hand, converting a singleton to virObject is a waste of
resources.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_daemon.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
Reluctant ACK if you add a 'inline' keyword.
Peter
12:25 p.m.
New subject: [libvirt] [PATCH v1 11/23] locking: Introduce virSeclabelSpace
This module will handle all the security label remembering and
recalling for virlockd. We need to record a tuple of (path,
security model, security label). So I'm going with a hash table,
where @path would be the key, and tuple of (model, label) is
going to be the payload. Well, the payload tuple is wrapped into
self inflating array (virSeclabelSpaceLabels). So for each path,
we will have to traverse through array of various models to find
the needed one. But since there's gonna be many paths and just a
few security models, it's going to be the best approach.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
po/POTFILES.in | 1 +
src/Makefile.am | 2 +
src/locking/lock_daemon_seclabels.c | 545 ++++++++++++++++++++++++++++++++++++
src/locking/lock_daemon_seclabels.h | 43 +++
4 files changed, 591 insertions(+)
create mode 100644 src/locking/lock_daemon_seclabels.c
create mode 100644 src/locking/lock_daemon_seclabels.h
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 0cc5b99..04dbad3 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -74,6 +74,7 @@ src/libvirt-qemu.c
src/locking/lock_daemon.c
src/locking/lock_daemon_config.c
src/locking/lock_daemon_dispatch.c
+src/locking/lock_daemon_seclabels.c
src/locking/lock_driver_lockd.c
src/locking/lock_driver_sanlock.c
src/locking/lock_manager.c
diff --git a/src/Makefile.am b/src/Makefile.am
index d5dd9fb..e706166 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -253,6 +253,8 @@ LOCK_DAEMON_SOURCES = \
locking/lock_daemon_config.c \
locking/lock_daemon_dispatch.c \
locking/lock_daemon_dispatch.h \
+ locking/lock_daemon_seclabels.c \
+ locking/lock_daemon_seclabels.h \
$(NULL)
locking/lock_daemon_dispatch_stubs.h: $(LOCK_PROTOCOL) \
diff --git a/src/locking/lock_daemon_seclabels.c b/src/locking/lock_daemon_seclabels.c
new file mode 100644
index 0000000..8c3e4a0
--- /dev/null
+++ b/src/locking/lock_daemon_seclabels.c
@@ -0,0 +1,545 @@
+/*
+ * lock_daemon_seclabels.c: Security label mgmt
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Michal Privoznik <mprivozn(a)redhat.com>
+ */
+
+#include <config.h>
+
+#include "viralloc.h"
+#include "virerror.h"
+#include "virhash.h"
+#include "virjson.h"
+#include "virlog.h"
+#include "virstring.h"
+
+#define VIR_FROM_THIS VIR_FROM_RPC
+
+VIR_LOG_INIT("locking.lock_daemon_dispatch");
+
+#include "lock_daemon.h"
+#include "lock_daemon_seclabels.h"
+
+typedef struct _virSeclabel virSeclabel;
+typedef virSeclabel *virSeclabelPtr;
+
+struct _virSeclabel {
+ char *model;
+ char *label;
+ unsigned int refcount;
+};
+
+
+typedef struct _virSeclabelSpaceLabels virSeclabelSpaceLabels;
+typedef virSeclabelSpaceLabels *virSeclabelSpaceLabelsPtr;
+
+struct _virSeclabelSpaceLabels {
+ virSeclabelPtr *labels;
+ size_t nlabels;
+};
+
+struct _virSeclabelSpace {
+ virMutex lock;
+
+ virHashTablePtr labels;
+};
+
+
+static void
+virSeclabelFree(virSeclabelPtr label)
+{
+ if (!label)
+ return;
+
+ VIR_FREE(label->model);
+ VIR_FREE(label->label);
+ VIR_FREE(label);
+}
+
+
+static virSeclabelPtr
+virSeclabelNew(const char *model,
+ const char *label)
+{
+ virSeclabelPtr ret;
+
+ if (VIR_ALLOC(ret) < 0)
+ return NULL;
+
+ if (VIR_STRDUP(ret->model, model) < 0 ||
+ VIR_STRDUP(ret->label, label) < 0)
+ goto error;
+
+ ret->refcount = 1;
+
+ return ret;
+ error:
+ virSeclabelFree(ret);
+ return NULL;
+}
+
+
+static void
+virSeclabelSpaceLabelsFree(virSeclabelSpaceLabelsPtr labels)
+{
+ size_t i;
+
+ if (!labels)
+ return;
+
+ for (i = 0; i < labels->nlabels; i++)
+ virSeclabelFree(labels->labels[i]);
+
+ VIR_FREE(labels->labels);
+ VIR_FREE(labels);
+}
+
+static void
+virSeclabelSpaceLabelsHashFree(void *payload,
+ const void *name ATTRIBUTE_UNUSED)
+{
+ virSeclabelSpaceLabelsFree(payload);
+}
+
+
+static void
+virSeclabelSpaceLock(virSeclabelSpacePtr space)
+{
+ virMutexLock(&space->lock);
+}
+
+
+static void
+virSeclabelSpaceUnlock(virSeclabelSpacePtr space)
+{
+ virMutexUnlock(&space->lock);
+}
+
+
+void
+virSeclabelSpaceFree(virSeclabelSpacePtr space)
+{
+ if (!space)
+ return;
+
+ virMutexDestroy(&space->lock);
+ virHashFree(space->labels);
+ VIR_FREE(space);
+}
+
+
+virSeclabelSpacePtr
+virSeclabelSpaceNew(void)
+{
+ virSeclabelSpacePtr ret;
+
+ if (VIR_ALLOC(ret) < 0)
+ return NULL;
+
+ if (virMutexInit(&ret->lock) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to init mutex"));
+ goto error;
+ }
+
+ if (!(ret->labels = virHashCreate(10, virSeclabelSpaceLabelsHashFree)))
+ goto error;
+
+ return ret;
+ error:
+ virSeclabelSpaceFree(ret);
+ return NULL;
+}
+
+
+static virSeclabelPtr
+virSeclabelSpaceLookup(virSeclabelSpacePtr space,
+ const char *path,
+ const char *model)
+{
+ virSeclabelSpaceLabelsPtr labels;
+ virSeclabelPtr ret;
+ size_t i;
+
+ if (!(labels = virHashLookup(space->labels, path)))
+ return NULL;
+
+ for (i = 0; labels->nlabels; i++) {
+ ret = labels->labels[i];
+
+ if (STREQ(ret->model, model))
+ return ret;
+ }
+
+ return NULL;
+}
+
+
+static int
+virSeclabelSpaceAdd(virSeclabelSpacePtr space,
+ const char *path,
+ virSeclabelPtr label)
+{
+ virSeclabelSpaceLabelsPtr labels;
+ virSeclabelPtr tmp;
+ size_t i;
+ int ret = -1;
+
+ if (!(labels = virHashLookup(space->labels, path))) {
+ /* Add new */
+ if (VIR_ALLOC(labels) < 0 ||
+ VIR_ALLOC(labels->labels) < 0 ||
+ virHashAddEntry(space->labels, path, labels) < 0) {
+ virSeclabelSpaceLabelsFree(labels);
+ goto cleanup;
+ }
+
+ labels->labels[0] = label;
+ labels->nlabels = 1;
+ } else {
+ /* Update old */
+ for (i = 0; i < labels->nlabels; i++) {
+ tmp = labels->labels[i];
+
+ if (STREQ(tmp->model, label->model)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("duplicate label for model '%s': old:
'%s' new '%s'"),
+ tmp->model, tmp->label, label->label);
+ goto cleanup;
+ }
+ }
+
+ if (VIR_APPEND_ELEMENT_COPY(labels->labels, labels->nlabels, label) <
0)
+ goto cleanup;
+
+ if (virHashUpdateEntry(space->labels, path, labels) < 0)
+ goto cleanup;
+ }
+
+ ret = 0;
+ cleanup:
+ return ret;
+}
+
+
+static void
+virSeclabelSpaceRemove(virSeclabelSpacePtr space,
+ const char *path,
+ virSeclabelPtr seclabel)
+{
+ virSeclabelSpaceLabelsPtr labels;
+ virSeclabelPtr tmp;
+ size_t i;
+
+ if (!(labels = virHashLookup(space->labels, path))) {
+ /* path not found */
+ return;
+ }
+
+ for (i = 0; i < labels->nlabels; i++) {
+ tmp = labels->labels[i];
+
+ if (STREQ(seclabel->model, tmp->model))
+ break;
+ }
+
+ if (i == labels->nlabels) {
+ /* model not found */
+ return;
+ }
+
+ tmp = labels->labels[i];
+
+ VIR_DELETE_ELEMENT(labels->labels, i, labels->nlabels);
+
+ if (!labels->nlabels)
+ virHashRemoveEntry(space->labels, path);
+}
+
+
+struct virSeclabelSpaceIteratorData {
+ virJSONValuePtr labels;
+ bool error;
+};
+
+
+static void
+virSeclabelSpaceIterator(void *payload,
+ const void *name,
+ void *opaque)
+{
+ virSeclabelSpaceLabelsPtr labels = payload;
+ const char *path = name;
+ struct virSeclabelSpaceIteratorData *data = opaque;
+ virSeclabelPtr tmp;
+ virJSONValuePtr item, jsonLabels, jsonLabel;
+ size_t i;
+
+ if (data->error)
+ return;
+
+ if (!(item = virJSONValueNewObject()))
+ goto error;
+
+ if (virJSONValueObjectAppendString(item, "path", path) < 0)
+ goto error;
+
+ if (!(jsonLabels = virJSONValueNewArray()))
+ goto error;
+
+ for (i = 0; i < labels->nlabels; i++) {
+ tmp = labels->labels[i];
+
+ if (!(jsonLabel = virJSONValueNewObject()))
+ goto error;
+
+ if (virJSONValueObjectAppendString(jsonLabel, "model", tmp->model)
< 0 ||
+ virJSONValueObjectAppendString(jsonLabel, "label", tmp->label)
< 0 ||
+ virJSONValueObjectAppendNumberUint(jsonLabel, "refcount",
tmp->refcount) < 0)
+ goto error;
+
+ if (virJSONValueArrayAppend(jsonLabels, jsonLabel) < 0)
+ goto error;
+ jsonLabel = NULL;
+ }
+
+ if (virJSONValueObjectAppend(item, "labels", jsonLabels) < 0)
+ goto error;
+ jsonLabels = NULL;
+
+ if (virJSONValueArrayAppend(data->labels, item) < 0)
+ goto error;
+ item = NULL;
+
+ return;
+ error:
+ virJSONValueFree(jsonLabel);
+ virJSONValueFree(jsonLabels);
+ virJSONValueFree(item);
+ data->error = true;
+}
+
+
+/**
+ * virSeclabelSpacePreExecRestart:
+ *
+ * @space: object to dump
+ *
+ * Dumps virSeclabel object into JSON.
+ *
+ * Returns: a non-NULL pointer to JSON object on success,
+ * NULL on error
+ */
+virJSONValuePtr
+virSeclabelSpacePreExecRestart(virSeclabelSpacePtr space)
+{
+ virJSONValuePtr object = NULL;
+ virJSONValuePtr labels = NULL;
+ struct virSeclabelSpaceIteratorData data;
+
+ if (!space)
+ return NULL;
+
+ virSeclabelSpaceLock(space);
+
+ if (!(object = virJSONValueNewObject()))
+ return NULL;
+
+ if (!(labels = virJSONValueNewArray()))
+ goto error;
+
+ data.labels = labels;
+ data.error = false;
+ if (virHashForEach(space->labels, virSeclabelSpaceIterator, &data) < 0)
+ goto error;
+
+ if (virJSONValueObjectAppend(object, "seclabels", labels) < 0)
+ goto error;
+ /* From now on, @labels is contained in @object. Avoid double freeing it. */
+ labels = NULL;
+
+ virSeclabelSpaceUnlock(space);
+ return object;
+ error:
+ virSeclabelSpaceUnlock(space);
+ virJSONValueFree(labels);
+ virJSONValueFree(object);
+ return NULL;
+}
+
+
+/**
+ * virSeclabelSpacePostExecRestart:
+ *
+ * @object: JSON representation of internal state
+ *
+ * Read in JSON object, create new virSeclabelSpace object and restore its internal state
from JSON.
+ *
+ * Returns: virSeclabelSpace object
+ * NULL on error
+ */
+virSeclabelSpacePtr
+virSeclabelSpacePostExecRestart(virJSONValuePtr object)
+{
+ virSeclabelSpacePtr ret;
+ virJSONValuePtr labels;
+ size_t i, npaths;
+
+ if (!(ret = virSeclabelSpaceNew()))
+ return NULL;
+
+ if (!(labels = virJSONValueObjectGetArray(object, "seclabels")))
+ goto error;
+
+ npaths = virJSONValueArraySize(labels);
+ for (i = 0; i < npaths; i++) {
+ virJSONValuePtr item = virJSONValueArrayGet(labels, i);
+ virJSONValuePtr arr = virJSONValueObjectGetArray(item, "labels");
+ const char *path = virJSONValueObjectGetString(item, "path");
+ size_t j, nlabels;
+
+ nlabels = virJSONValueArraySize(arr);
+
+ for (j = 0; j < nlabels; j++) {
+ virJSONValuePtr arr_item = virJSONValueArrayGet(arr, j);
+ const char *model = virJSONValueObjectGetString(arr_item,
"model");
+ const char *label = virJSONValueObjectGetString(arr_item,
"label");
+ virSeclabelPtr seclabel;
+
+ if (!(seclabel = virSeclabelNew(model, label)))
+ goto error;
+
+ if (virJSONValueObjectGetNumberUint(arr_item,
+ "refcount",
+ &seclabel->refcount) < 0) {
+ virSeclabelFree(seclabel);
+ goto error;
+ }
+
+ if (virSeclabelSpaceAdd(ret, path, seclabel) < 0) {
+ virSeclabelFree(seclabel);
+ goto error;
+ }
+ }
+ }
+
+ return ret;
+ error:
+ virSeclabelSpaceFree(ret);
+ return NULL;
+}
+
+
+/**
+ * virSeclabelSpaceRemember:
+ *
+ * @space: seclabel space object
+ * @path: path to remember
+ * @model: security model of @label ("selinux", "dac", etc.)
+ * @label: actual value to hold (e.g. "root:root")
+ *
+ * This function should remember the original label for a @path on the first
+ * call. Any subsequent call over the same @path and @model just increments the
+ * refcounter for the label (actual value of @label is ignored in this case).
+ *
+ * Returns: 0 on success
+ * -1 otherwise
+ */
+int
+virSeclabelSpaceRemember(virSeclabelSpacePtr space,
+ const char *path,
+ const char *model,
+ const char *label)
+{
+ virSeclabelPtr seclabel;
+ int ret = -1;
+
+ virSeclabelSpaceLock(space);
+ if ((seclabel = virSeclabelSpaceLookup(space, path, model))) {
+ /* We don't really care about label here. There already is an existing
+ * record for the @path and @model, so @label is likely to not have the
+ * original label anyway. */
+ seclabel->refcount++;
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (!(seclabel = virSeclabelNew(model, label)))
+ goto cleanup;
+
+ if (virSeclabelSpaceAdd(space, path, seclabel) < 0) {
+ virSeclabelFree(seclabel);
+ goto cleanup;
+ }
+
+ ret = 0;
+ cleanup:
+ virSeclabelSpaceUnlock(space);
+ return ret;
+}
+
+
+/**
+ * virSeclabelSpaceRecall:
+ *
+ * @space: seclabel space object
+ * @path: path to recall
+ * @model: model to recall
+ * @label: the original value remembered
+ *
+ * Counterpart for virSeclabelSpaceRemember. Upon successful return (retval ==
+ * 0) @label is set to point to the original value remembered. The caller is
+ * responsible for freeing the @label when no longer needed.
+ *
+ * Returns: 1 if label found, but still in use (refcount > 1), @label not touched
+ * 0 if label found and not used anymore (refcount = 1), @label set
+ * -1 if no label was recorded with (@path, @model) tuple, @label not touched
+ */
+int
+virSeclabelSpaceRecall(virSeclabelSpacePtr space,
+ const char *path,
+ const char *model,
+ char **label)
+{
+ int ret = -1;
+ virSeclabelPtr seclabel;
+
+ virSeclabelSpaceLock(space);
+
+ if (!(seclabel = virSeclabelSpaceLookup(space, path, model)))
+ goto cleanup;
+
+ seclabel->refcount--;
+ if (seclabel->refcount) {
+ /* still in use */
+ ret = 1;
+ goto cleanup;
+ }
+
+ virSeclabelSpaceRemove(space, path, seclabel);
+ /* steal the label pointer before freeing */
+ *label = seclabel->label;
+ seclabel->label = NULL;
+ virSeclabelFree(seclabel);
+
+ ret = 0;
+ cleanup:
+ virSeclabelSpaceUnlock(space);
+ return ret;
+}
diff --git a/src/locking/lock_daemon_seclabels.h b/src/locking/lock_daemon_seclabels.h
new file mode 100644
index 0000000..3164206
--- /dev/null
+++ b/src/locking/lock_daemon_seclabels.h
@@ -0,0 +1,43 @@
+/*
+ * lock_daemon_seclabels.h: Security label mgmt
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Michal Privoznik <mprivozn(a)redhat.com>
+ */
+
+#ifndef __VIR_LOCK_DAEMON_SECLABELS_H__
+# define __VIR_LOCK_DAEMON_SECLABELS_H__
+
+typedef struct _virSeclabelSpace virSeclabelSpace;
+typedef virSeclabelSpace *virSeclabelSpacePtr;
+
+void virSeclabelSpaceFree(virSeclabelSpacePtr space);
+virSeclabelSpacePtr virSeclabelSpaceNew(void);
+
+int virSeclabelSpaceRemember(virSeclabelSpacePtr space,
+ const char *path,
+ const char *model,
+ const char *label);
+int virSeclabelSpaceRecall(virSeclabelSpacePtr space,
+ const char *path,
+ const char *model,
+ char **label);
+
+virJSONValuePtr virSeclabelSpacePreExecRestart(virSeclabelSpacePtr space);
+virSeclabelSpacePtr virSeclabelSpacePostExecRestart(virJSONValuePtr object);
+#endif /* __VIR_LOCK_DAEMON_SECLABELS_H__ */
--
2.4.9
11:32 p.m.
New subject: [libvirt] [PATCH v1 11/23] locking: Introduce virSeclabelSpace
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
This module will handle all the security label remembering and
recalling for virlockd. We need to record a tuple of (path,
security model, security label). So I'm going with a hash table,
where @path would be the key, and tuple of (model, label) is
going to be the payload. Well, the payload tuple is wrapped into
self inflating array (virSeclabelSpaceLabels). So for each path,
we will have to traverse through array of various models to find
the needed one. But since there's gonna be many paths and just a
few security models, it's going to be the best approach.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
po/POTFILES.in | 1 +
src/Makefile.am | 2 +
src/locking/lock_daemon_seclabels.c | 545 ++++++++++++++++++++++++++++++++++++
src/locking/lock_daemon_seclabels.h | 43 +++
4 files changed, 591 insertions(+)
create mode 100644 src/locking/lock_daemon_seclabels.c
create mode 100644 src/locking/lock_daemon_seclabels.h
As a "first pass" - I ran this through Coverity and it found a couple
issues...
John
...
+
+
+static void
+virSeclabelSpaceIterator(void *payload,
+ const void *name,
+ void *opaque)
+{
+ virSeclabelSpaceLabelsPtr labels = payload;
+ const char *path = name;
+ struct virSeclabelSpaceIteratorData *data = opaque;
+ virSeclabelPtr tmp;
+ virJSONValuePtr item, jsonLabels, jsonLabel;
Coverity found:
jsonLabel and jsonLabels should be initialized since one can jump to
error and call virJSONValueFree before they are properly initialized.
NOTE: Build fails because of this...
+ size_t i;
+
+ if (data->error)
+ return;
+
+ if (!(item = virJSONValueNewObject()))
+ goto error;
+
+ if (virJSONValueObjectAppendString(item, "path", path) < 0)
+ goto error;
+
+ if (!(jsonLabels = virJSONValueNewArray()))
+ goto error;
+
+ for (i = 0; i < labels->nlabels; i++) {
+ tmp = labels->labels[i];
+
+ if (!(jsonLabel = virJSONValueNewObject()))
+ goto error;
+
+ if (virJSONValueObjectAppendString(jsonLabel, "model", tmp->model)
< 0 ||
+ virJSONValueObjectAppendString(jsonLabel, "label", tmp->label)
< 0 ||
+ virJSONValueObjectAppendNumberUint(jsonLabel, "refcount",
tmp->refcount) < 0)
+ goto error;
+
+ if (virJSONValueArrayAppend(jsonLabels, jsonLabel) < 0)
+ goto error;
+ jsonLabel = NULL;
+ }
+
+ if (virJSONValueObjectAppend(item, "labels", jsonLabels) < 0)
+ goto error;
+ jsonLabels = NULL;
+
+ if (virJSONValueArrayAppend(data->labels, item) < 0)
+ goto error;
+ item = NULL;
+
+ return;
+ error:
+ virJSONValueFree(jsonLabel);
+ virJSONValueFree(jsonLabels);
+ virJSONValueFree(item);
+ data->error = true;
+}
+
+
...
+/**
+ * virSeclabelSpacePostExecRestart:
+ *
+ * @object: JSON representation of internal state
+ *
+ * Read in JSON object, create new virSeclabelSpace object and restore its internal
state from JSON.
+ *
+ * Returns: virSeclabelSpace object
+ * NULL on error
+ */
+virSeclabelSpacePtr
+virSeclabelSpacePostExecRestart(virJSONValuePtr object)
+{
+ virSeclabelSpacePtr ret;
+ virJSONValuePtr labels;
+ size_t i, npaths;
+
+ if (!(ret = virSeclabelSpaceNew()))
+ return NULL;
+
+ if (!(labels = virJSONValueObjectGetArray(object, "seclabels")))
+ goto error;
+
+ npaths = virJSONValueArraySize(labels);
Coverity found:
if npaths is -1, then the following loop ends on an unsigned variable...
+ for (i = 0; i < npaths; i++) {
+ virJSONValuePtr item = virJSONValueArrayGet(labels, i);
+ virJSONValuePtr arr = virJSONValueObjectGetArray(item, "labels");
Coverity found:
if 'arr' is NULL, then deref below won't end well.
+ const char *path = virJSONValueObjectGetString(item,
"path");
+ size_t j, nlabels;
+
+ nlabels = virJSONValueArraySize(arr);
Coverity found:
if nlabels is -1, then the following loop ends on an unsigned variable...
+
+ for (j = 0; j < nlabels; j++) {
+ virJSONValuePtr arr_item = virJSONValueArrayGet(arr, j);
+ const char *model = virJSONValueObjectGetString(arr_item,
"model");
+ const char *label = virJSONValueObjectGetString(arr_item,
"label");
+ virSeclabelPtr seclabel;
+
+ if (!(seclabel = virSeclabelNew(model, label)))
+ goto error;
+
+ if (virJSONValueObjectGetNumberUint(arr_item,
+ "refcount",
+ &seclabel->refcount) < 0) {
+ virSeclabelFree(seclabel);
+ goto error;
+ }
+
+ if (virSeclabelSpaceAdd(ret, path, seclabel) < 0) {
+ virSeclabelFree(seclabel);
+ goto error;
+ }
+ }
+ }
+
+ return ret;
+ error:
+ virSeclabelSpaceFree(ret);
+ return NULL;
+}
+
+
12:25 p.m.
New subject: [libvirt] [PATCH v1 12/23] virtlockd: Work virSeclabelSpace in
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_daemon.c | 44 ++++++++++++++++++++++++++
src/locking/lock_daemon.h | 8 +++++
src/locking/lock_daemon_dispatch.c | 65 ++++++++++++++++++++++++++++++++------
3 files changed, 108 insertions(+), 9 deletions(-)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 3984b4d..3a0a7ff 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -49,6 +49,7 @@
#include "virstring.h"
#include "locking/lock_daemon_dispatch.h"
+#include "locking/lock_daemon_seclabels.h"
#include "locking/lock_protocol.h"
#include "configmake.h"
@@ -64,6 +65,7 @@ struct _virLockDaemon {
virNetDaemonPtr dmn;
virHashTablePtr lockspaces;
virLockSpacePtr defaultLockspace;
+ virSeclabelSpacePtr seclabelSpace;
};
virLockDaemonPtr lockDaemon = NULL;
@@ -121,6 +123,7 @@ virLockDaemonFree(virLockDaemonPtr lockd)
virObjectUnref(lockd->dmn);
virHashFree(lockd->lockspaces);
virLockSpaceFree(lockd->defaultLockspace);
+ virSeclabelSpaceFree(lockd->seclabelSpace);
VIR_FREE(lockd);
}
@@ -179,6 +182,9 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool privileged)
if (!(lockd->defaultLockspace = virLockSpaceNew(NULL)))
goto error;
+ if (!(lockd->seclabelSpace = virSeclabelSpaceNew()))
+ goto error;
+
return lockd;
error:
@@ -193,6 +199,7 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool
privileged)
virLockDaemonPtr lockd;
virJSONValuePtr child;
virJSONValuePtr lockspaces;
+ virJSONValuePtr seclabelSpace;
virNetServerPtr srv;
size_t i;
ssize_t n;
@@ -248,6 +255,14 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool
privileged)
}
}
+ if (!(seclabelSpace = virJSONValueObjectGet(object, "seclabelSpace"))) {
+ /* It's okay, if there's not seclabel space info. */
+ if (!(lockd->seclabelSpace = virSeclabelSpaceNew()))
+ goto error;
+ } else if (!(lockd->seclabelSpace =
virSeclabelSpacePostExecRestart(seclabelSpace))) {
+ goto error;
+ }
+
if (virJSONValueObjectHasKey(object, "daemon")) {
if (!(child = virJSONValueObjectGet(object, "daemon"))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -281,6 +296,26 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool
privileged)
}
+int virLockDaemonRememberSeclabel(virLockDaemonPtr lockd,
+ const char *path,
+ const char *model,
+ const char *label)
+{
+ return virSeclabelSpaceRemember(lockd->seclabelSpace,
+ path, model, label);
+}
+
+
+int virLockDaemonRecallSeclabel(virLockDaemonPtr lockd,
+ const char *path,
+ const char *model,
+ char **label)
+{
+ return virSeclabelSpaceRecall(lockd->seclabelSpace,
+ path, model, label);
+}
+
+
int virLockDaemonAddLockSpace(virLockDaemonPtr lockd,
const char *path,
virLockSpacePtr lockspace)
@@ -1005,6 +1040,7 @@ virLockDaemonPreExecRestart(const char *state_file,
char *magic;
virHashKeyValuePairPtr pairs = NULL, tmp;
virJSONValuePtr lockspaces;
+ virJSONValuePtr seclabelSpace;
VIR_DEBUG("Running pre-restart exec");
@@ -1050,6 +1086,14 @@ virLockDaemonPreExecRestart(const char *state_file,
tmp++;
}
+ if (!(seclabelSpace = virSeclabelSpacePreExecRestart(lockDaemon->seclabelSpace)))
+ goto cleanup;
+
+ if (virJSONValueObjectAppend(object, "seclabelSpace", seclabelSpace) <
0) {
+ virJSONValueFree(seclabelSpace);
+ goto cleanup;
+ }
+
if (!(magic = virLockDaemonGetExecRestartMagic()))
goto cleanup;
diff --git a/src/locking/lock_daemon.h b/src/locking/lock_daemon.h
index da62edc..fb72a73 100644
--- a/src/locking/lock_daemon.h
+++ b/src/locking/lock_daemon.h
@@ -53,4 +53,12 @@ int virLockDaemonAddLockSpace(virLockDaemonPtr lockd,
virLockSpacePtr virLockDaemonFindLockSpace(virLockDaemonPtr lockd,
const char *path);
+int virLockDaemonRememberSeclabel(virLockDaemonPtr lockd,
+ const char *path,
+ const char *model,
+ const char *label);
+int virLockDaemonRecallSeclabel(virLockDaemonPtr lockd,
+ const char *path,
+ const char *model,
+ char **label);
#endif /* __VIR_LOCK_DAEMON_H__ */
diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c
index 2d0bd81..5ad7e11 100644
--- a/src/locking/lock_daemon_dispatch.c
+++ b/src/locking/lock_daemon_dispatch.c
@@ -24,6 +24,7 @@
#include "rpc/virnetdaemon.h"
#include "rpc/virnetserverclient.h"
+#include "viralloc.h"
#include "virlog.h"
#include "virstring.h"
#include "lock_daemon.h"
@@ -36,6 +37,7 @@
VIR_LOG_INIT("locking.lock_daemon_dispatch");
#include "lock_daemon_dispatch_stubs.h"
+#include "lock_daemon_seclabels.h"
static int
virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNUSED,
@@ -433,21 +435,66 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServerPtr server
ATTRIBUTE_UNU
static int
virLockSpaceProtocolDispatchRememberSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED,
- virNetServerClientPtr client
ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
- virLockSpaceProtocolRememberSeclabelArgs
*args ATTRIBUTE_UNUSED)
+ virNetMessageErrorPtr rerr,
+ virLockSpaceProtocolRememberSeclabelArgs
*args)
{
- return 0;
+ int rv = -1;
+ virLockDaemonClientPtr priv =
+ virNetServerClientGetPrivateData(client);
+
+ virMutexLock(&priv->lock);
+
+ if (virLockDaemonRememberSeclabel(lockDaemon,
+ args->path, args->model, args->label) <
0)
+ goto cleanup;
+
+ rv = 0;
+ cleanup:
+ if (rv < 0)
+ virNetMessageSaveError(rerr);
+ virMutexUnlock(&priv->lock);
+ return rv;
}
static int
virLockSpaceProtocolDispatchRecallSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED,
- virNetServerClientPtr client
ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
- virLockSpaceProtocolRecallSeclabelArgs *args
ATTRIBUTE_UNUSED,
- virLockSpaceProtocolRecallSeclabelRet *ret
ATTRIBUTE_UNUSED)
+ virNetMessageErrorPtr rerr,
+ virLockSpaceProtocolRecallSeclabelArgs *args,
+ virLockSpaceProtocolRecallSeclabelRet *ret)
{
- return 0;
+ int rv = -1;
+ int funcRet;
+ virLockDaemonClientPtr priv =
+ virNetServerClientGetPrivateData(client);
+ char *label = NULL;
+ char **label_p = NULL;
+
+ virMutexLock(&priv->lock);
+
+ memset(ret, 0, sizeof(*ret));
+
+ funcRet = virLockDaemonRecallSeclabel(lockDaemon,
+ args->path, args->model, &label);
+
+ if (funcRet == 0 &&
+ (VIR_ALLOC(label_p) < 0 ||
+ VIR_STRDUP(*label_p, label) < 0))
+ goto cleanup;
+
+ ret->label = label_p;
+ ret->ret = funcRet;
+ rv = 0;
+
+ cleanup:
+ if (rv < 0) {
+ VIR_FREE(label_p);
+ virNetMessageSaveError(rerr);
+ }
+ virMutexUnlock(&priv->lock);
+ VIR_FREE(label);
+ return rv;
}
--
2.4.9
9:32 a.m.
New subject: [libvirt] [PATCH v1 12/23] virtlockd: Work virSeclabelSpace in
On Mon, Oct 12, 2015 at 12:25:57 +0200, Michal Privoznik wrote:
A rather sparse commit message.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_daemon.c | 44 ++++++++++++++++++++++++++
src/locking/lock_daemon.h | 8 +++++
src/locking/lock_daemon_dispatch.c | 65 ++++++++++++++++++++++++++++++++------
3 files changed, 108 insertions(+), 9 deletions(-)
[...]
diff --git a/src/locking/lock_daemon_dispatch.c
b/src/locking/lock_daemon_dispatch.c
index 2d0bd81..5ad7e11 100644
--- a/src/locking/lock_daemon_dispatch.c
+++ b/src/locking/lock_daemon_dispatch.c
[...]
@@ -433,21 +435,66 @@
virLockSpaceProtocolDispatchCreateLockSpace(virNetServerPtr server ATTRIBUTE_UNU
[...]
static int
virLockSpaceProtocolDispatchRecallSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED,
- virNetServerClientPtr client
ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
- virLockSpaceProtocolRecallSeclabelArgs *args
ATTRIBUTE_UNUSED,
- virLockSpaceProtocolRecallSeclabelRet *ret
ATTRIBUTE_UNUSED)
+ virNetMessageErrorPtr rerr,
+ virLockSpaceProtocolRecallSeclabelArgs
*args,
+ virLockSpaceProtocolRecallSeclabelRet *ret)
{
- return 0;
+ int rv = -1;
+ int funcRet;
+ virLockDaemonClientPtr priv =
+ virNetServerClientGetPrivateData(client);
+ char *label = NULL;
+ char **label_p = NULL;
+
+ virMutexLock(&priv->lock);
+
+ memset(ret, 0, sizeof(*ret));
+
+ funcRet = virLockDaemonRecallSeclabel(lockDaemon,
+ args->path, args->model, &label);
+
+ if (funcRet == 0 &&
+ (VIR_ALLOC(label_p) < 0 ||
+ VIR_STRDUP(*label_p, label) < 0))
This looks rather weird? Why is the extra pointer necessary?
+ goto cleanup;
+
+ ret->label = label_p;
+ ret->ret = funcRet;
+ rv = 0;
Peter
4:54 p.m.
New subject: [libvirt] [PATCH v1 12/23] virtlockd: Work virSeclabelSpace in
On 16.10.2015 09:32, Peter Krempa wrote:
On Mon, Oct 12, 2015 at 12:25:57 +0200, Michal Privoznik wrote:
A rather sparse commit message.
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/locking/lock_daemon.c | 44 ++++++++++++++++++++++++++
> src/locking/lock_daemon.h | 8 +++++
> src/locking/lock_daemon_dispatch.c | 65 ++++++++++++++++++++++++++++++++------
> 3 files changed, 108 insertions(+), 9 deletions(-)
[...]
> diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c
> index 2d0bd81..5ad7e11 100644
> --- a/src/locking/lock_daemon_dispatch.c
> +++ b/src/locking/lock_daemon_dispatch.c
[...]
> @@ -433,21 +435,66 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServerPtr
server ATTRIBUTE_UNU
[...]
> static int
> virLockSpaceProtocolDispatchRecallSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED,
> - virNetServerClientPtr client
ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client,
> virNetMessagePtr msg ATTRIBUTE_UNUSED,
> - virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
> - virLockSpaceProtocolRecallSeclabelArgs
*args ATTRIBUTE_UNUSED,
> - virLockSpaceProtocolRecallSeclabelRet
*ret ATTRIBUTE_UNUSED)
> + virNetMessageErrorPtr rerr,
> + virLockSpaceProtocolRecallSeclabelArgs
*args,
> + virLockSpaceProtocolRecallSeclabelRet
*ret)
> {
> - return 0;
> + int rv = -1;
> + int funcRet;
> + virLockDaemonClientPtr priv =
> + virNetServerClientGetPrivateData(client);
> + char *label = NULL;
> + char **label_p = NULL;
> +
> + virMutexLock(&priv->lock);
> +
> + memset(ret, 0, sizeof(*ret));
> +
> + funcRet = virLockDaemonRecallSeclabel(lockDaemon,
> + args->path, args->model,
&label);
> +
> + if (funcRet == 0 &&
> + (VIR_ALLOC(label_p) < 0 ||
> + VIR_STRDUP(*label_p, label) < 0))
This looks rather weird? Why is the extra pointer necessary?
It's due to way we handle remote strings. If that was remote NON NULL
string, then a single pointer would be enough, but since I want it to be
NULL sometimes I must do it this way. If you look at the remote
procedures we generate automatically you'll find the same pattern (in
fact I've copied the idea from there).
Michal
12:25 p.m.
New subject: [libvirt] [PATCH v1 13/23] virLockDriver: Introduce virLockDriverRemember and virLockDriverRecall
These are just internal APIs that will pass the data back and
forth to and from virtlockd.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver.h | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index f8fd38e..4a2bbe4 100644
--- a/src/locking/lock_driver.h
+++ b/src/locking/lock_driver.h
@@ -285,6 +285,39 @@ typedef int (*virLockDriverInquire)(virLockManagerPtr man,
unsigned int flags);
+/**
+ * virLockDriverRemember:
+ * @man: the lock manager context
+ * @path: path to the file
+ * @model: security label model
+ * @label: security label to remember
+ *
+ * Remember given security label.
+ *
+ * Returns 0 on success, or -1 on failure.
+ */
+typedef int (*virLockDriverRemember)(virLockManagerPtr man,
+ const char *path,
+ const char *model,
+ const char *label);
+
+/**
+ * virLockDriverRecall:
+ * @man: the lock manager context
+ * @path: path to the file
+ * @model: security label model
+ * @label: security label to restore
+ *
+ * Recall previously saved security label
+ *
+ * Returns: 1 if @path is still in use (@label untouched),
+ * 0 if @path seclabel should be restored (@label set)
+ * -1 on failure (e.g. @path not found in records)
+ */
+typedef int (*virLockDriverRecall)(virLockManagerPtr man,
+ const char *path,
+ const char *model,
+ char **label);
struct _virLockManager {
virLockDriverPtr driver;
void *privateData;
@@ -313,6 +346,9 @@ struct _virLockDriver {
virLockDriverAcquire drvAcquire;
virLockDriverRelease drvRelease;
virLockDriverInquire drvInquire;
+
+ virLockDriverRemember drvRemember;
+ virLockDriverRecall drvRecall;
};
--
2.4.9
9:34 a.m.
New subject: [libvirt] [PATCH v1 13/23] virLockDriver: Introduce virLockDriverRemember and virLockDriverRecall
On Mon, Oct 12, 2015 at 12:25:58 +0200, Michal Privoznik wrote:
These are just internal APIs that will pass the data back and
forth to and from virtlockd.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver.h | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index f8fd38e..4a2bbe4 100644
--- a/src/locking/lock_driver.h
+++ b/src/locking/lock_driver.h
@@ -285,6 +285,39 @@ typedef int (*virLockDriverInquire)(virLockManagerPtr man,
unsigned int flags);
+/**
+ * virLockDriverRemember:
+ * @man: the lock manager context
+ * @path: path to the file
+ * @model: security label model
+ * @label: security label to remember
+ *
+ * Remember given security label.
As top level functions these would deserve a bit more documentation.
+ *
+ * Returns 0 on success, or -1 on failure.
Peter
12:25 p.m.
New subject: [libvirt] [PATCH v1 14/23] lock_driver_nop: Implement remember and recall APIs
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver_nop.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/src/locking/lock_driver_nop.c b/src/locking/lock_driver_nop.c
index 93a988e..89aad3a 100644
--- a/src/locking/lock_driver_nop.c
+++ b/src/locking/lock_driver_nop.c
@@ -101,6 +101,27 @@ static void virLockManagerNopFree(virLockManagerPtr lock
ATTRIBUTE_UNUSED)
{
}
+static int
+virLockManagerNopRemember(virLockManagerPtr lock ATTRIBUTE_UNUSED,
+ const char *path ATTRIBUTE_UNUSED,
+ const char *model ATTRIBUTE_UNUSED,
+ const char *label ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+static int
+virLockManagerNopRecall(virLockManagerPtr lock ATTRIBUTE_UNUSED,
+ const char *path ATTRIBUTE_UNUSED,
+ const char *model ATTRIBUTE_UNUSED,
+ char **label)
+{
+ if (label)
+ *label = NULL;
+
+ return 0;
+}
+
virLockDriver virLockDriverNop =
{
.version = VIR_LOCK_MANAGER_VERSION,
@@ -118,4 +139,7 @@ virLockDriver virLockDriverNop =
.drvRelease = virLockManagerNopRelease,
.drvInquire = virLockManagerNopInquire,
+
+ .drvRemember = virLockManagerNopRemember,
+ .drvRecall = virLockManagerNopRecall,
};
--
2.4.9
12:26 p.m.
New subject: [libvirt] [PATCH v1 15/23] lock_driver_lockd: Implement remember and recall APIs
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver_lockd.c | 87 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 87 insertions(+)
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index c974d60..70ed45d 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -775,6 +775,90 @@ static int virLockManagerLockDaemonInquire(virLockManagerPtr lock
ATTRIBUTE_UNUS
return 0;
}
+static int
+virLockManagerLockDaemonRemember(virLockManagerPtr lock,
+ const char *path,
+ const char *model,
+ const char *label)
+{
+ virNetClientPtr client = NULL;
+ virNetClientProgramPtr program = NULL;
+ virLockSpaceProtocolRememberSeclabelArgs args;
+ int counter = 0;
+ int rv = -1;
+
+ memset(&args, 0, sizeof(args));
+
+ if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))
+ goto cleanup;
+
+ args.path = (char *)path;
+ args.model = (char *)model;
+ args.label = (char *)label;
+
+ if (virNetClientProgramCall(program,
+ client,
+ counter++,
+ VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL,
+ 0, NULL, NULL, NULL,
+ (xdrproc_t)xdr_virLockSpaceProtocolRememberSeclabelArgs,
&args,
+ (xdrproc_t)xdr_void, NULL) < 0)
+ goto cleanup;
+ rv = 0;
+
+ cleanup:
+ virNetClientClose(client);
+ virObjectUnref(client);
+ virObjectUnref(program);
+
+ return rv;
+}
+
+static int
+virLockManagerLockDaemonRecall(virLockManagerPtr lock,
+ const char *path,
+ const char *model,
+ char **label)
+{
+ virNetClientPtr client = NULL;
+ virNetClientProgramPtr program = NULL;
+ virLockSpaceProtocolRecallSeclabelArgs args;
+ virLockSpaceProtocolRecallSeclabelRet ret;
+ int counter = 0;
+ int rv = -1;
+
+ memset(&args, 0, sizeof(args));
+ memset(&ret, 0, sizeof(ret));
+
+ if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))
+ goto cleanup;
+
+ args.path = (char *)path;
+ args.model = (char *)model;
+
+ if (virNetClientProgramCall(program,
+ client,
+ counter++,
+ VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL,
+ 0, NULL, NULL, NULL,
+ (xdrproc_t)xdr_virLockSpaceProtocolRecallSeclabelArgs,
&args,
+ (xdrproc_t)xdr_virLockSpaceProtocolRecallSeclabelRet,
&ret) < 0)
+ goto cleanup;
+
+ if (ret.ret == 0 && label)
+ *label = *ret.label;
+
+ rv = ret.ret;
+
+ cleanup:
+ VIR_FREE(ret.label);
+ virNetClientClose(client);
+ virObjectUnref(client);
+ virObjectUnref(program);
+
+ return rv;
+}
+
virLockDriver virLockDriverImpl =
{
.version = VIR_LOCK_MANAGER_VERSION,
@@ -792,4 +876,7 @@ virLockDriver virLockDriverImpl =
.drvRelease = virLockManagerLockDaemonRelease,
.drvInquire = virLockManagerLockDaemonInquire,
+
+ .drvRemember = virLockManagerLockDaemonRemember,
+ .drvRecall = virLockManagerLockDaemonRecall,
};
--
2.4.9
12:26 p.m.
New subject: [libvirt] [PATCH v1 16/23] lock_manager: Implement remember & recall APIs
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 2 ++
src/locking/lock_manager.c | 32 ++++++++++++++++++++++++++++++++
src/locking/lock_manager.h | 9 +++++++++
3 files changed, 43 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index be6ee19..72dda41 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1000,7 +1000,9 @@ virLockManagerPluginNew;
virLockManagerPluginRef;
virLockManagerPluginUnref;
virLockManagerPluginUsesState;
+virLockManagerRecallSeclabel;
virLockManagerRelease;
+virLockManagerRememberSeclabel;
# nodeinfo.h
diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c
index a002ea8..8fc82c8 100644
--- a/src/locking/lock_manager.c
+++ b/src/locking/lock_manager.c
@@ -397,3 +397,35 @@ int virLockManagerFree(virLockManagerPtr lock)
return 0;
}
+
+int virLockManagerRememberSeclabel(virLockManagerPtr lock,
+ const char *path,
+ const char *model,
+ const char *label)
+{
+ VIR_DEBUG("lock=%p path=%s model=%s label=%s",
+ lock, path, model, label);
+
+ if (!lock)
+ return 0;
+
+ CHECK_MANAGER(drvRemember, 0);
+
+ return lock->driver->drvRemember(lock, path, model, label);
+}
+
+int virLockManagerRecallSeclabel(virLockManagerPtr lock,
+ const char *path,
+ const char *model,
+ char **label)
+{
+ VIR_DEBUG("lock=%p path=%s model=%s label=%p",
+ lock, path, model, label);
+
+ if (!lock)
+ return 0;
+
+ CHECK_MANAGER(drvRecall, 0);
+
+ return lock->driver->drvRecall(lock, path, model, label);
+}
diff --git a/src/locking/lock_manager.h b/src/locking/lock_manager.h
index 4189759..4c2236f 100644
--- a/src/locking/lock_manager.h
+++ b/src/locking/lock_manager.h
@@ -67,4 +67,13 @@ int virLockManagerInquire(virLockManagerPtr manager,
int virLockManagerFree(virLockManagerPtr manager);
+int virLockManagerRememberSeclabel(virLockManagerPtr manager,
+ const char *path,
+ const char *model,
+ const char *label);
+int virLockManagerRecallSeclabel(virLockManagerPtr manager,
+ const char *path,
+ const char *model,
+ char **label);
+
#endif /* __VIR_LOCK_MANAGER_H__ */
--
2.4.9
9:49 a.m.
New subject: [libvirt] [PATCH v1 16/23] lock_manager: Implement remember & recall APIs
On Mon, Oct 12, 2015 at 12:26:01 +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 2 ++
src/locking/lock_manager.c | 32 ++++++++++++++++++++++++++++++++
src/locking/lock_manager.h | 9 +++++++++
3 files changed, 43 insertions(+)
[...]
diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c
index a002ea8..8fc82c8 100644
--- a/src/locking/lock_manager.c
+++ b/src/locking/lock_manager.c
@@ -397,3 +397,35 @@ int virLockManagerFree(virLockManagerPtr lock)
return 0;
}
+
+int virLockManagerRememberSeclabel(virLockManagerPtr lock,
+ const char *path,
+ const char *model,
+ const char *label)
+{
+ VIR_DEBUG("lock=%p path=%s model=%s label=%s",
+ lock, path, model, label);
+
+ if (!lock)
+ return 0;
+
+ CHECK_MANAGER(drvRemember, 0);
Reporting an error but then returning success doesn't seem to be the
right thing to do.
+
+ return lock->driver->drvRemember(lock, path, model, label);
+}
+
+int virLockManagerRecallSeclabel(virLockManagerPtr lock,
+ const char *path,
+ const char *model,
+ char **label)
+{
+ VIR_DEBUG("lock=%p path=%s model=%s label=%p",
+ lock, path, model, label);
+
+ if (!lock)
+ return 0;
+
+ CHECK_MANAGER(drvRecall, 0);
Same here.
+
+ return lock->driver->drvRecall(lock, path, model, label);
+}
Peter
12:26 p.m.
New subject: [libvirt] [PATCH v1 17/23] locking: Favour enum type over int
This is not a public API where we have to care about size of
data types. Moreover, I'll be adding new type to lock soon and it
would help if compiler could point on all the locations I need to
fix.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver.h | 4 ++--
src/locking/lock_driver_lockd.c | 4 ++--
src/locking/lock_driver_nop.c | 4 ++--
src/locking/lock_driver_sanlock.c | 4 ++--
src/locking/lock_manager.c | 4 ++--
src/locking/lock_manager.h | 4 ++--
6 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index 4a2bbe4..e7a8371 100644
--- a/src/locking/lock_driver.h
+++ b/src/locking/lock_driver.h
@@ -171,7 +171,7 @@ typedef int (*virLockDriverDeinit)(void);
* Returns 0 if successful initialized a new context, -1 on error
*/
typedef int (*virLockDriverNew)(virLockManagerPtr man,
- unsigned int type,
+ virLockManagerObjectType type,
size_t nparams,
virLockManagerParamPtr params,
unsigned int flags);
@@ -218,7 +218,7 @@ typedef void (*virLockDriverFree)(virLockManagerPtr man);
* Returns 0 on success, or -1 on failure
*/
typedef int (*virLockDriverAddResource)(virLockManagerPtr man,
- unsigned int type,
+ virLockManagerResourceType type,
const char *name,
size_t nparams,
virLockManagerParamPtr params,
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 70ed45d..a4a9a62 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -427,7 +427,7 @@ static void virLockManagerLockDaemonFree(virLockManagerPtr lock)
static int virLockManagerLockDaemonNew(virLockManagerPtr lock,
- unsigned int type,
+ virLockManagerObjectType type,
size_t nparams,
virLockManagerParamPtr params,
unsigned int flags)
@@ -492,7 +492,7 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock,
static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
- unsigned int type,
+ virLockManagerResourceType type,
const char *name,
size_t nparams,
virLockManagerParamPtr params,
diff --git a/src/locking/lock_driver_nop.c b/src/locking/lock_driver_nop.c
index 89aad3a..5f72ee5 100644
--- a/src/locking/lock_driver_nop.c
+++ b/src/locking/lock_driver_nop.c
@@ -48,7 +48,7 @@ static int virLockManagerNopDeinit(void)
static int virLockManagerNopNew(virLockManagerPtr lock ATTRIBUTE_UNUSED,
- unsigned int type ATTRIBUTE_UNUSED,
+ virLockManagerObjectType type ATTRIBUTE_UNUSED,
size_t nparams ATTRIBUTE_UNUSED,
virLockManagerParamPtr params ATTRIBUTE_UNUSED,
unsigned int flags_unused ATTRIBUTE_UNUSED)
@@ -57,7 +57,7 @@ static int virLockManagerNopNew(virLockManagerPtr lock
ATTRIBUTE_UNUSED,
}
static int virLockManagerNopAddResource(virLockManagerPtr lock ATTRIBUTE_UNUSED,
- unsigned int type ATTRIBUTE_UNUSED,
+ virLockManagerResourceType type
ATTRIBUTE_UNUSED,
const char *name ATTRIBUTE_UNUSED,
size_t nparams ATTRIBUTE_UNUSED,
virLockManagerParamPtr params ATTRIBUTE_UNUSED,
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index e052875..2de9a55 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -445,7 +445,7 @@ static int virLockManagerSanlockDeinit(void)
static int virLockManagerSanlockNew(virLockManagerPtr lock,
- unsigned int type,
+ virLockManagerObjectType type,
size_t nparams,
virLockManagerParamPtr params,
unsigned int flags)
@@ -732,7 +732,7 @@ static int virLockManagerSanlockCreateLease(struct sanlk_resource
*res)
static int virLockManagerSanlockAddResource(virLockManagerPtr lock,
- unsigned int type,
+ virLockManagerResourceType type,
const char *name,
size_t nparams,
virLockManagerParamPtr params,
diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c
index 8fc82c8..83684f2 100644
--- a/src/locking/lock_manager.c
+++ b/src/locking/lock_manager.c
@@ -295,7 +295,7 @@ virLockDriverPtr virLockManagerPluginGetDriver(virLockManagerPluginPtr
plugin)
* Returns a new lock manager context
*/
virLockManagerPtr virLockManagerNew(virLockDriverPtr driver,
- unsigned int type,
+ virLockManagerObjectType type,
size_t nparams,
virLockManagerParamPtr params,
unsigned int flags)
@@ -322,7 +322,7 @@ virLockManagerPtr virLockManagerNew(virLockDriverPtr driver,
int virLockManagerAddResource(virLockManagerPtr lock,
- unsigned int type,
+ virLockManagerResourceType type,
const char *name,
size_t nparams,
virLockManagerParamPtr params,
diff --git a/src/locking/lock_manager.h b/src/locking/lock_manager.h
index 4c2236f..0f85a7b 100644
--- a/src/locking/lock_manager.h
+++ b/src/locking/lock_manager.h
@@ -41,13 +41,13 @@ bool virLockManagerPluginUsesState(virLockManagerPluginPtr plugin);
virLockDriverPtr virLockManagerPluginGetDriver(virLockManagerPluginPtr plugin);
virLockManagerPtr virLockManagerNew(virLockDriverPtr driver,
- unsigned int type,
+ virLockManagerObjectType type,
size_t nparams,
virLockManagerParamPtr params,
unsigned int flags);
int virLockManagerAddResource(virLockManagerPtr manager,
- unsigned int type,
+ virLockManagerResourceType type,
const char *name,
size_t nparams,
virLockManagerParamPtr params,
--
2.4.9
12:26 p.m.
New subject: [libvirt] [PATCH v1 18/23] lock_driver: Introduce VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver.h | 2 ++
src/locking/lock_driver_lockd.c | 12 +++++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index e7a8371..71feea3 100644
--- a/src/locking/lock_driver.h
+++ b/src/locking/lock_driver.h
@@ -42,6 +42,8 @@ typedef enum {
typedef enum {
/* The managed object is a virtual guest domain */
VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN = 0,
+ /* The managed object is a seclabel */
+ VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL,
} virLockManagerObjectType;
typedef enum {
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index a4a9a62..f9867a1 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -54,6 +54,8 @@ struct _virLockManagerLockDaemonResource {
};
struct _virLockManagerLockDaemonPrivate {
+ virLockManagerObjectType type;
+
unsigned char uuid[VIR_UUID_BUFLEN];
char *name;
int id;
@@ -290,11 +292,13 @@ virLockManagerLockDaemonConnect(virLockManagerPtr lock,
int *counter)
{
virNetClientPtr client;
+ virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
if (!(client = virLockManagerLockDaemonConnectionNew(geteuid() == 0, program)))
return NULL;
- if (virLockManagerLockDaemonConnectionRegister(lock,
+ if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN &&
+ virLockManagerLockDaemonConnectionRegister(lock,
client,
*program,
counter) < 0)
@@ -441,6 +445,8 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock,
return -1;
lock->privateData = priv;
+ priv->type = type;
+
switch (type) {
case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
for (i = 0; i < nparams; i++) {
@@ -480,6 +486,10 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock,
}
break;
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL:
+ /* nada */
+ break;
+
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown lock manager object type %d"),
--
2.4.9
12:26 p.m.
New subject: [libvirt] [PATCH v1 19/23] virSecurityManagerNewDAC: Pass locking plugin in
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
cfg.mk | 2 +-
src/qemu/qemu_driver.c | 3 ++-
src/security/security_dac.c | 10 ++++++++++
src/security/security_dac.h | 2 ++
src/security/security_manager.c | 4 +++-
src/security/security_manager.h | 4 +++-
6 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index e436434..43a591f 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -783,7 +783,7 @@ sc_prohibit_cross_inclusion:
access/ | conf/) safe="($$dir|conf|util)";; \
locking/) safe="($$dir|util|conf|rpc)";; \
cpu/| network/| node_device/| rpc/| security/| storage/) \
- safe="($$dir|util|conf|storage)";; \
+ safe="($$dir|util|conf|storage|locking)";; \
xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen)";; \
*) safe="($$dir|$(mid_dirs)|util)";; \
esac; \
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8cd5ee3..5abccb0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -435,7 +435,8 @@ qemuSecurityInit(virQEMUDriverPtr driver)
cfg->user,
cfg->group,
flags,
- qemuSecurityChownCallback)))
+ qemuSecurityChownCallback,
+ driver->lockManager)))
goto error;
if (!stack) {
if (!(stack = virSecurityManagerNewStack(mgr)))
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 6c4e351..463b459 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -57,6 +57,7 @@ struct _virSecurityDACData {
bool dynamicOwnership;
char *baselabel;
virSecurityManagerDACChownCallback chownCallback;
+ virLockManagerPluginPtr lockPlugin;
};
typedef struct _virSecurityDACCallbackData virSecurityDACCallbackData;
@@ -101,6 +102,15 @@ virSecurityDACSetChownCallback(virSecurityManagerPtr mgr,
priv->chownCallback = chownCallback;
}
+void
+virSecurityDACSetLockingPlugin(virSecurityManagerPtr mgr,
+ virLockManagerPluginPtr lockPlugin)
+{
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ priv->lockPlugin = lockPlugin;
+}
+
+
/* returns 1 if label isn't found, 0 on success, -1 on error */
static int
ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
diff --git a/src/security/security_dac.h b/src/security/security_dac.h
index 846cefb..aefc262 100644
--- a/src/security/security_dac.h
+++ b/src/security/security_dac.h
@@ -35,4 +35,6 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
void virSecurityDACSetChownCallback(virSecurityManagerPtr mgr,
virSecurityManagerDACChownCallback chownCallback);
+void virSecurityDACSetLockingPlugin(virSecurityManagerPtr mgr,
+ virLockManagerPluginPtr lockPlugin);
#endif /* __VIR_SECURITY_DAC */
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 5b05a47..e41e761 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -141,7 +141,8 @@ virSecurityManagerNewDAC(const char *virtDriver,
uid_t user,
gid_t group,
unsigned int flags,
- virSecurityManagerDACChownCallback chownCallback)
+ virSecurityManagerDACChownCallback chownCallback,
+ virLockManagerPluginPtr lockPlugin)
{
virSecurityManagerPtr mgr;
@@ -162,6 +163,7 @@ virSecurityManagerNewDAC(const char *virtDriver,
virSecurityDACSetDynamicOwnership(mgr, flags &
VIR_SECURITY_MANAGER_DYNAMIC_OWNERSHIP);
virSecurityDACSetChownCallback(mgr, chownCallback);
+ virSecurityDACSetLockingPlugin(mgr, lockPlugin);
return mgr;
}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index e534e31..96f7053 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -26,6 +26,7 @@
# include "domain_conf.h"
# include "vircommand.h"
# include "virstoragefile.h"
+# include "locking/lock_manager.h"
typedef struct _virSecurityManager virSecurityManager;
typedef virSecurityManager *virSecurityManagerPtr;
@@ -71,7 +72,8 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver,
uid_t user,
gid_t group,
unsigned int flags,
- virSecurityManagerDACChownCallback
chownCallback);
+ virSecurityManagerDACChownCallback
chownCallback,
+ virLockManagerPluginPtr lockPlugin);
int virSecurityManagerPreFork(virSecurityManagerPtr mgr);
void virSecurityManagerPostFork(virSecurityManagerPtr mgr);
--
2.4.9
12:26 p.m.
New subject: [libvirt] [PATCH v1 20/23] security_dac: Remember security labels
https://bugzilla.redhat.com/show_bug.cgi?id=547546
So now that we have the whole infrastructure prepared, we can
remember and recall the DAC security labels.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/Makefile.am | 1 +
src/security/security_dac.c | 90 ++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 81 insertions(+), 10 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am
index e706166..51b4af0 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -2738,6 +2738,7 @@ libvirt_lxc_LDADD = \
libvirt-net-rpc-server.la \
libvirt-net-rpc.la \
libvirt_security_manager.la \
+ libvirt_driver.la \
libvirt_conf.la \
libvirt_util.la \
../gnulib/lib/libgnu.la
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 463b459..5c99dfa 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -206,12 +206,38 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel,
* Returns: 0 on success, -1 on failure
*/
static int
-virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
- const char *path ATTRIBUTE_UNUSED,
- uid_t uid ATTRIBUTE_UNUSED,
- gid_t gid ATTRIBUTE_UNUSED)
+virSecurityDACRememberLabel(virSecurityDACDataPtr priv,
+ const char *path,
+ uid_t uid,
+ gid_t gid)
{
- return 0;
+ int ret = -1;
+ virLockManagerPtr lockManager = NULL;
+ char *label;
+
+ if (!priv->lockPlugin)
+ return 0;
+
+ if (virAsprintf(&label, "+%u:+%u",
+ (unsigned int) uid,
+ (unsigned int) gid) < 0)
+ goto cleanup;
+
+ lockManager = virLockManagerNew(virLockManagerPluginGetDriver(priv->lockPlugin),
+ VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL,
+ 0, NULL, 0);
+ if (!lockManager)
+ goto cleanup;
+
+ if (virLockManagerRememberSeclabel(lockManager, path,
+ SECURITY_DAC_NAME, label) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(label);
+ virLockManagerFree(lockManager);
+ return ret;
}
/**
@@ -230,12 +256,56 @@ virSecurityDACRememberLabel(virSecurityDACDataPtr priv
ATTRIBUTE_UNUSED,
* -1 on failure (@uid and @gid not touched)
*/
static int
-virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
- const char *path ATTRIBUTE_UNUSED,
- uid_t *uid ATTRIBUTE_UNUSED,
- gid_t *gid ATTRIBUTE_UNUSED)
+virSecurityDACRecallLabel(virSecurityDACDataPtr priv,
+ const char *path,
+ uid_t *uid,
+ gid_t *gid)
{
- return 0;
+ int rv, ret = -1;
+ virLockManagerPtr lockManager = NULL;
+ char *label = NULL;
+
+ if (!priv->lockPlugin)
+ return 0;
+
+ lockManager = virLockManagerNew(virLockManagerPluginGetDriver(priv->lockPlugin),
+ VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL,
+ 0, NULL, 0);
+ if (!lockManager)
+ goto cleanup;
+
+ rv = virLockManagerRecallSeclabel(lockManager, path,
+ SECURITY_DAC_NAME, &label);
+
+ VIR_DEBUG("path=%s label=%s", path, label);
+
+ if (rv < 0) {
+ /* Okay, path was not found, or there was some other error. At any
+ * rate, claim success here. */
+ ret = 0;
+ goto cleanup;
+ } else if (rv > 0) {
+ /* Okay, path was found, but is still in use. Notify caller so that we
+ * don't relabel anything. */
+ ret = 1;
+ goto cleanup;
+ }
+
+ if (!label) {
+ /* Okay, underlying lock driver does not know how to store seclabels.
+ * Fall back to defaults. */
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (virParseOwnershipIds(label, uid, gid) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(label);
+ virLockManagerFree(lockManager);
+ return ret;
}
static virSecurityDriverStatus
--
2.4.9
12:26 p.m.
New subject: [libvirt] [PATCH v1 21/23] security_dac: Restore original owner more often
Now that we know what label we should restore and we do have
reference counter to each seclabel, we restore the original
seclabel only after the last domain is torn down. Therefore, we
can safely try to restore labels even for RO or shared disks. The
reference counter will take the care of everything.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 5c99dfa..59b16ef 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -561,14 +561,6 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr
mgr,
if (!priv->dynamicOwnership)
return 0;
- /* Don't restore labels on readoly/shared disks, because other VMs may
- * still be accessing these. Alternatively we could iterate over all
- * running domains and try to figure out if it is in use, but this would
- * not work for clustered filesystems, since we can't see running VMs using
- * the file on other nodes. Safest bet is thus to skip the restore step. */
- if (src->readonly || src->shared)
- return 0;
-
secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
if (secdef && !secdef->relabel)
return 0;
--
2.4.9
9:55 a.m.
New subject: [libvirt] [PATCH v1 21/23] security_dac: Restore original owner more often
On Mon, Oct 12, 2015 at 12:26:06 +0200, Michal Privoznik wrote:
Now that we know what label we should restore and we do have
reference counter to each seclabel, we restore the original
seclabel only after the last domain is torn down. Therefore, we
can safely try to restore labels even for RO or shared disks. The
reference counter will take the care of everything.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 5c99dfa..59b16ef 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -561,14 +561,6 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr
mgr,
if (!priv->dynamicOwnership)
return 0;
- /* Don't restore labels on readoly/shared disks, because other VMs may
- * still be accessing these. Alternatively we could iterate over all
- * running domains and try to figure out if it is in use, but this would
- * not work for clustered filesystems, since we can't see running VMs using
- * the file on other nodes. Safest bet is thus to skip the restore step. */
- if (src->readonly || src->shared)
- return 0;
-
This will regress if you use the 'nop' driver for "storing" locks but
still expect libvirt not to break your labelling.
Peter
12:26 p.m.
New subject: [libvirt] [PATCH v1 22/23] security: Introduce virSecurityManagerDomainRestoreDirLabel
This is a counterpart of virSecurityManagerDomainSetDirLabel.
We should restore the security labels on the directories where
we've changed them.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/security/security_dac.c | 18 ++++++++++++++++++
src/security/security_driver.h | 5 ++++-
src/security/security_manager.c | 16 ++++++++++++++++
src/security/security_manager.h | 3 +++
src/security/security_selinux.c | 16 ++++++++++++++++
src/security/security_stack.c | 20 ++++++++++++++++++++
7 files changed, 78 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 72dda41..0e03c6c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1030,6 +1030,7 @@ virSecurityDriverLookup;
# security/security_manager.h
virSecurityManagerCheckAllLabel;
virSecurityManagerClearSocketLabel;
+virSecurityManagerDomainRestoreDirLabel;
virSecurityManagerDomainSetDirLabel;
virSecurityManagerGenLabel;
virSecurityManagerGetBaseLabel;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 59b16ef..043866e 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1578,6 +1578,23 @@ virSecurityDACDomainSetDirLabel(virSecurityManagerPtr mgr,
return virSecurityDACSetOwnership(priv, NULL, path, user, group);
}
+static int
+virSecurityDACDomainRestoreDirLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ const char *path)
+{
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ virSecurityLabelDefPtr seclabel;
+
+ seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
+
+ if (!seclabel && !seclabel->relabel)
+ return 0;
+
+ return virSecurityDACRestoreSecurityFileLabel(priv, path);
+}
+
+
virSecurityDriver virSecurityDriverDAC = {
.privateDataLen = sizeof(virSecurityDACData),
.name = SECURITY_DAC_NAME,
@@ -1627,4 +1644,5 @@ virSecurityDriver virSecurityDriverDAC = {
.getBaseLabel = virSecurityDACGetBaseLabel,
.domainSetDirLabel = virSecurityDACDomainSetDirLabel,
+ .domainRestoreDirLabel = virSecurityDACDomainRestoreDirLabel,
};
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 784b0de..2503831 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -121,7 +121,9 @@ typedef int (*virSecurityDomainRestoreImageLabel)
(virSecurityManagerPtr mgr,
typedef int (*virSecurityDomainSetDirLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
const char *path);
-
+typedef int (*virSecurityDomainRestoreDirLabel) (virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ const char *path);
struct _virSecurityDriver {
size_t privateDataLen;
@@ -173,6 +175,7 @@ struct _virSecurityDriver {
virSecurityDriverGetBaseLabel getBaseLabel;
virSecurityDomainSetDirLabel domainSetDirLabel;
+ virSecurityDomainRestoreDirLabel domainRestoreDirLabel;
};
virSecurityDriverPtr virSecurityDriverLookup(const char *name,
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index e41e761..a5308ac 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1003,3 +1003,19 @@ virSecurityManagerDomainSetDirLabel(virSecurityManagerPtr mgr,
return 0;
}
+
+int
+virSecurityManagerDomainRestoreDirLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr vm,
+ const char *path)
+{
+ if (mgr->drv->domainRestoreDirLabel) {
+ int ret;
+ virObjectLock(mgr);
+ ret = mgr->drv->domainRestoreDirLabel(mgr, vm, path);
+ virObjectUnlock(mgr);
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 96f7053..bfec4fa 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -165,5 +165,8 @@ int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr,
int virSecurityManagerDomainSetDirLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
const char *path);
+int virSecurityManagerDomainRestoreDirLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr vm,
+ const char *path);
#endif /* VIR_SECURITY_MANAGER_H__ */
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index c2464c2..3d04123 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2550,6 +2550,21 @@ virSecuritySELinuxDomainSetDirLabel(virSecurityManagerPtr mgr,
return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel);
}
+static int
+virSecuritySELinuxDomainRestoreDirLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ const char *path)
+{
+ virSecurityLabelDefPtr seclabel;
+
+ seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
+ if (!seclabel || !seclabel->relabel)
+ return 0;
+
+ return virSecuritySELinuxRestoreSecurityFileLabel(mgr, path);
+}
+
+
virSecurityDriver virSecurityDriverSELinux = {
.privateDataLen = sizeof(virSecuritySELinuxData),
.name = SECURITY_SELINUX_NAME,
@@ -2596,4 +2611,5 @@ virSecurityDriver virSecurityDriverSELinux = {
.getBaseLabel = virSecuritySELinuxGetBaseLabel,
.domainSetDirLabel = virSecuritySELinuxDomainSetDirLabel,
+ .domainRestoreDirLabel = virSecuritySELinuxDomainRestoreDirLabel,
};
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 8d9560d..5c9d27f 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -617,6 +617,25 @@ virSecurityStackDomainSetDirLabel(virSecurityManagerPtr mgr,
return rc;
}
+static int
+virSecurityStackDomainRestoreDirLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr vm,
+ const char *path)
+{
+ virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ virSecurityStackItemPtr item = priv->itemsHead;
+ int rc = 0;
+
+ for (; item; item = item->next) {
+ if (virSecurityManagerDomainRestoreDirLabel(item->securityManager,
+ vm, path) < 0)
+ rc = -1;
+ }
+
+ return rc;
+}
+
+
virSecurityDriver virSecurityDriverStack = {
.privateDataLen = sizeof(virSecurityStackData),
.name = "stack",
@@ -668,4 +687,5 @@ virSecurityDriver virSecurityDriverStack = {
.getBaseLabel = virSecurityStackGetBaseLabel,
.domainSetDirLabel = virSecurityStackDomainSetDirLabel,
+ .domainRestoreDirLabel = virSecurityStackDomainRestoreDirLabel,
};
--
2.4.9
12:26 p.m.
New subject: [libvirt] [PATCH v1 23/23] qemuProcessStop: Restore seclabels on dirs too
This is a counterpart for f1f68ca33. While we set the security
labels on various directories that domain is going to use (e.g.
channel target directory) we don't restore the permissions back.
Leaving a hanging entry in virtlockd.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8aa9efc..e2968ea 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5243,15 +5243,21 @@ void qemuProcessStop(virQEMUDriverPtr driver,
priv->monConfig = NULL;
}
- ignore_value(virAsprintf(&tmppath, "%s/domain-%s",
- cfg->libDir, vm->def->name));
- virFileDeleteTree(tmppath);
- VIR_FREE(tmppath);
+ if (virAsprintf(&tmppath, "%s/domain-%s",
+ cfg->libDir, vm->def->name) > 0) {
+ virSecurityManagerDomainRestoreDirLabel(driver->securityManager,
+ vm->def, tmppath);
+ virFileDeleteTree(tmppath);
+ VIR_FREE(tmppath);
+ }
- ignore_value(virAsprintf(&tmppath, "%s/domain-%s",
- cfg->channelTargetDir, vm->def->name));
- virFileDeleteTree(tmppath);
- VIR_FREE(tmppath);
+ if (virAsprintf(&tmppath, "%s/domain-%s",
+ cfg->channelTargetDir, vm->def->name) > 0) {
+ virSecurityManagerDomainRestoreDirLabel(driver->securityManager,
+ vm->def, tmppath);
+ virFileDeleteTree(tmppath);
+ VIR_FREE(tmppath);
+ }
ignore_value(virDomainChrDefForeach(vm->def,
false,
--
2.4.9
4:54 p.m.
On 12.10.2015 12:25, Michal Privoznik wrote:
So, you may be familiar with this already. Well, I've tried to
get these
patches in like a year ago (or even more). Point is, these ones are new,
written from scratch. However, still based on idea, that virtlockd will keep
the track of the original seclabels. So far only DAC driver is fixed, but the
infrastructure I'm proposing here is easily extensible to other drivers too.
Even if there's some disagreement on the design, the first few patches fix some
bugs, so they should make it in.
Michal Privoznik (23):
virtlockd: Don't SIGSEGV on SIGUSR1
security_dac: Fix TODO marks
virSecurityDACSetOwnershipInternal: Don't chown so often
security_dac: Introduce remember/recall stubs
virSecurityDACSetOwnership: Pass virSecurityDACDataPtr
virSecurityDACRestoreSecurityFileLabel: Pass virSecurityDACDataPtr
security_dac: Limit usage of virSecurityDACSetOwnershipInternal
security_dac: Plug in remember/recall APIs
lock_protocol: Add two new remote procedures
lock_daemon: Switch to wrapper locking functions
locking: Introduce virSeclabelSpace
virtlockd: Work virSeclabelSpace in
virLockDriver: Introduce virLockDriverRemember and virLockDriverRecall
lock_driver_nop: Implement remember and recall APIs
lock_driver_lockd: Implement remember and recall APIs
lock_manager: Implement remember & recall APIs
locking: Favour enum type over int
lock_driver: Introduce VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL
virSecurityManagerNewDAC: Pass locking plugin in
security_dac: Remember security labels
security_dac: Restore original owner more often
security: Introduce virSecurityManagerDomainRestoreDirLabel
qemuProcessStop: Restore seclabels on dirs too
cfg.mk | 2 +-
po/POTFILES.in | 1 +
src/Makefile.am | 3 +
src/libvirt_private.syms | 3 +
src/lock_protocol-structs | 15 +
src/locking/lock_daemon.c | 69 ++++-
src/locking/lock_daemon.h | 8 +
src/locking/lock_daemon_dispatch.c | 68 +++++
src/locking/lock_daemon_seclabels.c | 545 ++++++++++++++++++++++++++++++++++++
src/locking/lock_daemon_seclabels.h | 43 +++
src/locking/lock_driver.h | 42 ++-
src/locking/lock_driver_lockd.c | 103 ++++++-
src/locking/lock_driver_nop.c | 28 +-
src/locking/lock_driver_sanlock.c | 4 +-
src/locking/lock_manager.c | 36 ++-
src/locking/lock_manager.h | 13 +-
src/locking/lock_protocol.x | 29 +-
src/qemu/qemu_driver.c | 3 +-
src/qemu/qemu_process.c | 22 +-
src/security/security_dac.c | 288 +++++++++++++++----
src/security/security_dac.h | 2 +
src/security/security_driver.h | 5 +-
src/security/security_manager.c | 20 +-
src/security/security_manager.h | 7 +-
src/security/security_selinux.c | 16 ++
src/security/security_stack.c | 20 ++
26 files changed, 1310 insertions(+), 85 deletions(-)
create mode 100644 src/locking/lock_daemon_seclabels.c
create mode 100644 src/locking/lock_daemon_seclabels.h
So, I've fixed all the findings up to 08/23 (inclusive) and pushed it up
till that point. Those patches mostly clean up security driver, fix some
bugs, and prepare the security driver to whatever remembering framework
we come up.
Thank you both guys!
Michal
3258
days inactive
3265
days old
49 comments
3 participants
participants (3)
-
John Ferlan -
Michal Privoznik -
Peter Krempa