Two new APIs are added so that security driver can lock and
unlock paths it wishes to touch. These APIs are not for other
drivers to call but security drivers (DAC and SELinux). That is
the reason these APIs are not exposed through our
libvirt_private.syms file.
Three interesting things happen in this commit. The first is the
global @lockManagerMutex. Unfortunately, this has to exist so that
there is only on thread talking to virtlockd at a time. If there
were more threads and one of them closed the connection
prematurely, it would cause virtlockd killing libvirtd. Instead
of complicated code that would handle that, let's have a mutex
and keep the code simple.
The second interesting thing is keeping connection open between
lock and unlock API calls. This is achieved by duplicating client
FD and keeping it open until unlock is called. This trick is used
by regular disk content locking code when the FD is leaked to
qemu.
Finally, the third thing is polling implemented at client side.
Since virtlockd has only one thread that handles locking
requests, all it can do is either acquire lock or error out.
Therefore, the polling has to be implemented in client. The
polling is capped at 60 second timeout, which should be plenty
since the metadata lock is held only for a fraction of a second.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/security/security_manager.c | 135 ++++++++++++++++++++++++++++++++++++++++
src/security/security_manager.h | 7 +++
2 files changed, 142 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 5c8370c159..dd5c3ac7e5 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -29,11 +29,15 @@
#include "virobject.h"
#include "virlog.h"
#include "locking/lock_manager.h"
+#include "virfile.h"
+#include "virtime.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
VIR_LOG_INIT("security.security_manager");
+virMutex lockManagerMutex = VIR_MUTEX_INITIALIZER;
+
struct _virSecurityManager {
virObjectLockable parent;
@@ -43,6 +47,7 @@ struct _virSecurityManager {
void *privateData;
virLockManagerPluginPtr lockPlugin;
+ int fd;
};
static virClassPtr virSecurityManagerClass;
@@ -57,6 +62,7 @@ void virSecurityManagerDispose(void *obj)
mgr->drv->close(mgr);
virObjectUnref(mgr->lockPlugin);
+ VIR_FORCE_CLOSE(mgr->fd);
VIR_FREE(mgr->privateData);
}
@@ -109,6 +115,7 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
mgr->flags = flags;
mgr->virtDriver = virtDriver;
VIR_STEAL_PTR(mgr->privateData, privateData);
+ mgr->fd = -1;
if (drv->open(mgr) < 0)
goto error;
@@ -1263,3 +1270,131 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
return 0;
}
+
+
+static virLockManagerPtr
+virSecurityManagerNewLockManager(virSecurityManagerPtr mgr,
+ const char * const *paths,
+ size_t npaths)
+{
+ virLockManagerPtr lock;
+ virLockManagerParam params[] = {
+ { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UUID,
+ .key = "uuid",
+ },
+ { .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING,
+ .key = "name",
+ .value = { .cstr = "libvirtd-sec" },
+ },
+ { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT,
+ .key = "pid",
+ .value = { .iv = getpid() },
+ },
+ };
+ const unsigned int flags = 0;
+ size_t i;
+
+ if (virGetHostUUID(params[0].value.uuid) < 0)
+ return NULL;
+
+ if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(mgr->lockPlugin),
+ VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON,
+ ARRAY_CARDINALITY(params),
+ params,
+ flags)))
+ return NULL;
+
+ for (i = 0; i < npaths; i++) {
+ if (virLockManagerAddResource(lock,
+ VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA,
+ paths[i], 0, NULL, 0) < 0)
+ goto error;
+ }
+
+ return lock;
+ error:
+ virLockManagerFree(lock);
+ return NULL;
+}
+
+
+/* How many seconds should we try to acquire the lock before
+ * giving up. */
+#define LOCK_ACQUIRE_TIMEOUT 60
+
+int
+virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
+ const char * const *paths,
+ size_t npaths)
+{
+ virLockManagerPtr lock;
+ virTimeBackOffVar timebackoff;
+ int fd = -1;
+ int rv;
+ int ret = -1;
+
+ virMutexLock(&lockManagerMutex);
+
+ if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
+ goto cleanup;
+
+ if (virTimeBackOffStart(&timebackoff, 1, LOCK_ACQUIRE_TIMEOUT * 1000) < 0)
+ goto cleanup;
+ while (virTimeBackOffWait(&timebackoff)) {
+ rv = virLockManagerAcquire(lock, NULL,
+ VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK,
+ VIR_DOMAIN_LOCK_FAILURE_DEFAULT, &fd);
+
+ if (rv >= 0)
+ break;
+
+ if (virGetLastErrorCode() == VIR_ERR_RESOURCE_BUSY)
+ continue;
+
+ goto cleanup;
+ }
+
+ if (rv < 0)
+ goto cleanup;
+
+ mgr->fd = fd;
+ fd = -1;
+
+ ret = 0;
+ cleanup:
+ virLockManagerFree(lock);
+ VIR_FORCE_CLOSE(fd);
+ if (ret < 0)
+ virMutexUnlock(&lockManagerMutex);
+ return ret;
+}
+
+
+int
+virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
+ const char * const *paths,
+ size_t npaths)
+{
+ virLockManagerPtr lock;
+ int fd;
+ int ret = -1;
+
+ /* lockManagerMutex acquired from previous
+ * virSecurityManagerMetadataLock() call. */
+
+ fd = mgr->fd;
+ mgr->fd = -1;
+
+ if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
+ goto cleanup;
+
+ if (virLockManagerRelease(lock, NULL, 0) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ virLockManagerFree(lock);
+ VIR_FORCE_CLOSE(fd);
+ virMutexUnlock(&lockManagerMutex);
+ return ret;
+}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index c537e1c994..10ebe5cc29 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -199,4 +199,11 @@ int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr,
int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
virDomainDefPtr vm);
+int virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
+ const char * const *paths,
+ size_t npaths);
+int virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
+ const char * const *paths,
+ size_t npaths);
+
#endif /* VIR_SECURITY_MANAGER_H__ */
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[...]
VIR_FROM_THIS VIR_FROM_SECURITY
>
> VIR_LOG_INIT("security.security_manager");
>
> +virMutex lockManagerMutex = VIR_MUTEX_INITIALIZER;
> +
> struct _virSecurityManager {
> virObjectLockable parent;
>
> @@ -43,6 +47,7 @@ struct _virSecurityManager {
> void *privateData;
>
> virLockManagerPluginPtr lockPlugin;
> + int fd;
> };
>
> static virClassPtr virSecurityManagerClass;
> @@ -57,6 +62,7 @@ void virSecurityManagerDispose(void *obj)
> mgr->drv->close(mgr);
>
> virObjectUnref(mgr->lockPlugin);
> + VIR_FORCE_CLOSE(mgr->fd);
>
> VIR_FREE(mgr->privateData);
> }
> @@ -109,6 +115,7 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
> mgr->flags = flags;
> mgr->virtDriver = virtDriver;
> VIR_STEAL_PTR(mgr->privateData, privateData);
> + mgr->fd = -1;
>
> if (drv->open(mgr) < 0)
> goto error;
> @@ -1263,3 +1270,131 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
>
> return 0;
> }
> +
> +
> +static virLockManagerPtr
> +virSecurityManagerNewLockManager(virSecurityManagerPtr mgr,
> + const char * const *paths,
> + size_t npaths)
> +{
> + virLockManagerPtr lock;
> + virLockManagerParam params[] = {
> + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UUID,
> + .key = "uuid",
> + },
> + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING,
> + .key = "name",
> + .value = { .cstr = "libvirtd-sec" },
> + },
> + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT,
> + .key = "pid",
> + .value = { .iv = getpid() },
> + },
> + };
> + const unsigned int flags = 0;
> + size_t i;
> +
> + if (virGetHostUUID(params[0].value.uuid) < 0)
> + return NULL;
> +
> + if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(mgr->lockPlugin),
> + VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON,
> + ARRAY_CARDINALITY(params),
> + params,
> + flags)))
> + return NULL;
> +
> + for (i = 0; i < npaths; i++) {
> + if (virLockManagerAddResource(lock,
> + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA,
> + paths[i], 0, NULL, 0) < 0)
> + goto error;
> + }
> +
> + return lock;
> + error:
> + virLockManagerFree(lock);
> + return NULL;
> +}
> +
> +
> +/* How many seconds should we try to acquire the lock before
> + * giving up. */
> +#define LOCK_ACQUIRE_TIMEOUT 60
> +
> +int
> +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
> + const char * const *paths,
> + size_t npaths)
> +{
> + virLockManagerPtr lock;
> + virTimeBackOffVar timebackoff;
> + int fd = -1;
> + int rv;
> + int ret = -1;
> +
> + virMutexLock(&lockManagerMutex);
> +
> + if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
> + goto cleanup;
> +
After seeing it in use in patch 19 and thinking about it for a very
short period of time, would it make more sense to store @lock somewhere
so that virSecurityManagerMetadataUnlock doesn't fail because the
virSecurityManagerNewLockManager fails?
The @mgr is mutex locked so that nothing can change @mgr while this Meta
Lock/Unlock is occurring. It'd be a shame to not call
virLockManagerRelease just because we didn't save @lock
John
> + if (virTimeBackOffStart(&timebackoff, 1, LOCK_ACQUIRE_TIMEOUT * 1000) < 0)
> + goto cleanup;
> + while (virTimeBackOffWait(&timebackoff)) {
> + rv = virLockManagerAcquire(lock, NULL,
> + VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK,
> + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, &fd);
> +
> + if (rv >= 0)
> + break;
> +
> + if (virGetLastErrorCode() == VIR_ERR_RESOURCE_BUSY)
> + continue;
> +
> + goto cleanup;
> + }
> +
> + if (rv < 0)
> + goto cleanup;
> +
> + mgr->fd = fd;
> + fd = -1;
> +
> + ret = 0;
> + cleanup:
> + virLockManagerFree(lock);
> + VIR_FORCE_CLOSE(fd);
> + if (ret < 0)
> + virMutexUnlock(&lockManagerMutex);
> + return ret;
> +}
> +
> +
> +int
> +virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
> + const char * const *paths,
> + size_t npaths)
> +{
> + virLockManagerPtr lock;
> + int fd;
> + int ret = -1;
> +
> + /* lockManagerMutex acquired from previous
> + * virSecurityManagerMetadataLock() call. */
> +
> + fd = mgr->fd;
> + mgr->fd = -1;
> +
> + if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
> + goto cleanup;
> +
> + if (virLockManagerRelease(lock, NULL, 0) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + virLockManagerFree(lock);
> + VIR_FORCE_CLOSE(fd);
> + virMutexUnlock(&lockManagerMutex);
> + return ret;
> +}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/18/2018 12:12 AM, John Ferlan wrote: > [...] > >> + > > After seeing it in use in patch 19 and thinking about it for a very > short period of time, would it make more sense to store @lock somewhere > so that virSecurityManagerMetadataUnlock doesn't fail because the > virSecurityManagerNewLockManager fails? > > The @mgr is mutex locked so that nothing can change @mgr while this Meta > Lock/Unlock is occurring. It'd be a shame to not call > virLockManagerRelease just because we didn't save @lock I will look into this, but quick glance over virSecurityManagerNewLockManager() tells me that it can fail only in OOM case (which means you're in much bigger trouble anyway). (some time passes) So I did take a look and we would need to save both FD and @lock. However, saving those two would not save us from allocating memory. Even then the virLockManagerLockDaemonRelease() allocates memory (for client, connection, etc.). Long story short, I don't think it is worth the effort. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Michal Privoznik <mprivozn@redhat.com> [2018-09-10, 11:36AM +0200]:
> +int
> +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
> + const char * const *paths,
> + size_t npaths)
> +{
> + virLockManagerPtr lock;
> + virTimeBackOffVar timebackoff;
> + int fd = -1;
> + int rv;
gcc complains that rv might be uninitialized.
> + int ret = -1;
> +
> + virMutexLock(&lockManagerMutex);
> +
> + if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
> + goto cleanup;
> +
> + if (virTimeBackOffStart(&timebackoff, 1, LOCK_ACQUIRE_TIMEOUT * 1000) < 0)
> + goto cleanup;
> + while (virTimeBackOffWait(&timebackoff)) {
> + rv = virLockManagerAcquire(lock, NULL,
> + VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK,
> + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, &fd);
> +
> + if (rv >= 0)
> + break;
> +
> + if (virGetLastErrorCode() == VIR_ERR_RESOURCE_BUSY)
> + continue;
> +
> + goto cleanup;
> + }
> +
> + if (rv < 0)
> + goto cleanup;
> +
> + mgr->fd = fd;
> + fd = -1;
> +
> + ret = 0;
> + cleanup:
> + virLockManagerFree(lock);
> + VIR_FORCE_CLOSE(fd);
> + if (ret < 0)
> + virMutexUnlock(&lockManagerMutex);
> + return ret;
> +}
--
IBM Systems
Linux on Z & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
------------------------------------------------------------------------
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/10/2018 02:19 PM, Bjoern Walk wrote:
> Michal Privoznik <mprivozn@redhat.com> [2018-09-10, 11:36AM +0200]:
>> +int
>> +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
>> + const char * const *paths,
>> + size_t npaths)
>> +{
>> + virLockManagerPtr lock;
>> + virTimeBackOffVar timebackoff;
>> + int fd = -1;
>> + int rv;
>
> gcc complains that rv might be uninitialized.
Right, if ..
>
>> + int ret = -1;
>> +
>> + virMutexLock(&lockManagerMutex);
>> +
>> + if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
>> + goto cleanup;
>> +
>> + if (virTimeBackOffStart(&timebackoff, 1, LOCK_ACQUIRE_TIMEOUT * 1000) < 0)
>> + goto cleanup;
>> + while (virTimeBackOffWait(&timebackoff)) {
.. this is never true (which is impossible).
>> + rv = virLockManagerAcquire(lock, NULL,
>> + VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK,
>> + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, &fd);
>> +
>> + if (rv >= 0)
>> + break;
>> +
>> + if (virGetLastErrorCode() == VIR_ERR_RESOURCE_BUSY)
>> + continue;
>> +
>> + goto cleanup;
>> + }
>> +
>> + if (rv < 0)
>> + goto cleanup;
Okay, I'll fix this is my local branch.
Thanks,
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/10/2018 05:36 AM, Michal Privoznik wrote:
> Two new APIs are added so that security driver can lock and
> unlock paths it wishes to touch. These APIs are not for other
> drivers to call but security drivers (DAC and SELinux). That is
> the reason these APIs are not exposed through our
> libvirt_private.syms file.
>
> Three interesting things happen in this commit. The first is the
> global @lockManagerMutex. Unfortunately, this has to exist so that
> there is only on thread talking to virtlockd at a time. If there
s/on/one
> were more threads and one of them closed the connection
> prematurely, it would cause virtlockd killing libvirtd. Instead
> of complicated code that would handle that, let's have a mutex
> and keep the code simple.
>
> The second interesting thing is keeping connection open between
> lock and unlock API calls. This is achieved by duplicating client
> FD and keeping it open until unlock is called. This trick is used
> by regular disk content locking code when the FD is leaked to
> qemu.
>
> Finally, the third thing is polling implemented at client side.
> Since virtlockd has only one thread that handles locking
> requests, all it can do is either acquire lock or error out.
> Therefore, the polling has to be implemented in client. The
> polling is capped at 60 second timeout, which should be plenty
> since the metadata lock is held only for a fraction of a second.
I smell a configurable timeout instead of 60 seconds, but that's mainly
because my senses are more acutely aware of such configurable timeout
changes given some recent on list patches for client lock timeouts when
(as I assume) there is a client gathering stats that takes a long time.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/security/security_manager.c | 135 ++++++++++++++++++++++++++++++++++++++++
> src/security/security_manager.h | 7 +++
> 2 files changed, 142 insertions(+)
>
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 5c8370c159..dd5c3ac7e5 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -29,11 +29,15 @@
> #include "virobject.h"
> #include "virlog.h"
> #include "locking/lock_manager.h"
> +#include "virfile.h"
> +#include "virtime.h"
>
> #define VIR_FROM_THIS VIR_FROM_SECURITY
>
> VIR_LOG_INIT("security.security_manager");
>
> +virMutex lockManagerMutex = VIR_MUTEX_INITIALIZER;
> +
> struct _virSecurityManager {
> virObjectLockable parent;
>
> @@ -43,6 +47,7 @@ struct _virSecurityManager {
> void *privateData;
>
> virLockManagerPluginPtr lockPlugin;
> + int fd;
Would you mind renaming to "clientfd" or at least "noting" in comments
here where this fd is duplicated so that future me doesn't need to go
back and read the commit where this was added to refresh my memory.
Seeing just fd and searching on that is like finding a needle in the
haystack of fd's.
> };
>
[...]
> +
> +/* How many seconds should we try to acquire the lock before
> + * giving up. */
How much wood could a woodchuck chuck wood if a woodchuck could chuck wood?
> +#define LOCK_ACQUIRE_TIMEOUT 60
> +
> +int
> +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
> + const char * const *paths,
> + size_t npaths)
> +{
> + virLockManagerPtr lock;
> + virTimeBackOffVar timebackoff;
> + int fd = -1;
> + int rv;
I know you know already by rv = -1
> + int ret = -1;
> +
> + virMutexLock(&lockManagerMutex);
> +
> + if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
> + goto cleanup;
> +
Just a couple of minor things,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/17/2018 11:14 PM, John Ferlan wrote: > > > On 09/10/2018 05:36 AM, Michal Privoznik wrote: >> Two new APIs are added so that security driver can lock and >> unlock paths it wishes to touch. These APIs are not for other >> drivers to call but security drivers (DAC and SELinux). That is >> the reason these APIs are not exposed through our >> libvirt_private.syms file. >> >> Three interesting things happen in this commit. The first is the >> global @lockManagerMutex. Unfortunately, this has to exist so that >> there is only on thread talking to virtlockd at a time. If there > > s/on/one > >> were more threads and one of them closed the connection >> prematurely, it would cause virtlockd killing libvirtd. Instead >> of complicated code that would handle that, let's have a mutex >> and keep the code simple. >> >> The second interesting thing is keeping connection open between >> lock and unlock API calls. This is achieved by duplicating client >> FD and keeping it open until unlock is called. This trick is used >> by regular disk content locking code when the FD is leaked to >> qemu. >> >> Finally, the third thing is polling implemented at client side. >> Since virtlockd has only one thread that handles locking >> requests, all it can do is either acquire lock or error out. >> Therefore, the polling has to be implemented in client. The >> polling is capped at 60 second timeout, which should be plenty >> since the metadata lock is held only for a fraction of a second. > > I smell a configurable timeout instead of 60 seconds, but that's mainly > because my senses are more acutely aware of such configurable timeout > changes given some recent on list patches for client lock timeouts when > (as I assume) there is a client gathering stats that takes a long time. Hopefully not. This indeed is a lock that guards chown()/setfilecon_raw(). I wouldn't expect them to took very long even if there were dozens of daemons fighting over the lock. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.