6:44 a.m.
Trying to use virlockd to lock metadata turns out to be too big
gun. Since we will always spawn a separate process for relabeling
we are safe to use thread unsafe POSIX locks and take out
virtlockd completely out of the picture.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 12 +-
src/security/security_manager.c | 225 +++++++++++++++++---------------
src/security/security_manager.h | 17 ++-
src/security/security_selinux.c | 11 +-
4 files changed, 141 insertions(+), 124 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 0e100f7895..6b64d2c07a 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -205,6 +205,7 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
void *opaque)
{
virSecurityDACChownListPtr list = opaque;
+ virSecurityManagerMetadataLockStatePtr state;
const char **paths = NULL;
size_t npaths = 0;
size_t i;
@@ -218,14 +219,10 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
for (i = 0; i < list->nItems; i++) {
const char *p = list->items[i]->path;
- if (!p ||
- virFileIsDir(p))
- continue;
-
VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
}
- if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0)
+ if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths)))
goto cleanup;
}
@@ -249,9 +246,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
break;
}
- if (list->lock &&
- virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0)
- goto cleanup;
+ if (list->lock)
+ virSecurityManagerMetadataUnlock(list->manager, &state);
if (rv < 0)
goto cleanup;
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 712b785ae9..f527e6b5b3 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -21,6 +21,10 @@
*/
#include <config.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
#include "security_driver.h"
#include "security_stack.h"
#include "security_dac.h"
@@ -30,14 +34,11 @@
#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;
@@ -47,10 +48,6 @@ struct _virSecurityManager {
void *privateData;
virLockManagerPluginPtr lockPlugin;
- /* This is a FD that represents a connection to virtlockd so
- * that connection is kept open in between MetdataLock() and
- * MetadataUnlock() calls. */
- int clientfd;
};
static virClassPtr virSecurityManagerClass;
@@ -66,7 +63,6 @@ void virSecurityManagerDispose(void *obj)
mgr->drv->close(mgr);
virObjectUnref(mgr->lockPlugin);
- VIR_FORCE_CLOSE(mgr->clientfd);
VIR_FREE(mgr->privateData);
}
@@ -119,7 +115,6 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
mgr->flags = flags;
mgr->virtDriver = virtDriver;
VIR_STEAL_PTR(mgr->privateData, privateData);
- mgr->clientfd = -1;
if (drv->open(mgr) < 0)
goto error;
@@ -1281,129 +1276,153 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
}
-static virLockManagerPtr
-virSecurityManagerNewLockManager(virSecurityManagerPtr mgr,
- const char * const *paths,
- size_t npaths)
+struct _virSecurityManagerMetadataLockState {
+ size_t nfds;
+ int *fds;
+};
+
+
+static int
+cmpstringp(const void *p1, const void *p2)
{
- 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;
+ const char *s1 = *(char * const *) p1;
+ const char *s2 = *(char * const *) p2;
- if (virGetHostUUID(params[0].value.uuid) < 0)
- return NULL;
+ if (!s1 && !s2)
+ return 0;
- if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(mgr->lockPlugin),
- VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON,
- ARRAY_CARDINALITY(params),
- params,
- flags)))
- return NULL;
+ if (!s1 || !s2)
+ return s2 ? -1 : 1;
- 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;
+ /* from man 3 qsort */
+ return strcmp(s1, s2);
}
+#define METADATA_OFFSET 1
+#define METADATA_LEN 1
-/* 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,
+/**
+ * virSecurityManagerMetadataLock:
+ * @mgr: security manager object
+ * @paths: paths to lock
+ * @npaths: number of items in @paths array
+ *
+ * Lock passed @paths for metadata change. The returned state
+ * should be passed to virSecurityManagerMetadataUnlock.
+ *
+ * NOTE: this function is not thread safe (because of usage of
+ * POSIX locks).
+ *
+ * Returns: state on success,
+ * NULL on failure.
+ */
+virSecurityManagerMetadataLockStatePtr
+virSecurityManagerMetadataLock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
+ const char **paths,
size_t npaths)
{
- virLockManagerPtr lock;
- virTimeBackOffVar timebackoff;
- int fd = -1;
- int rv = -1;
- int ret = -1;
+ size_t i = 0;
+ size_t nfds = 0;
+ int *fds = NULL;
+ virSecurityManagerMetadataLockStatePtr ret = NULL;
- virMutexLock(&lockManagerMutex);
+ if (VIR_ALLOC_N(fds, npaths) < 0)
+ return NULL;
- if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
- goto cleanup;
+ /* Sort paths to lock in order to avoid deadlocks. */
+ qsort(paths, npaths, sizeof(*paths), cmpstringp);
- 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);
+ for (i = 0; i < npaths; i++) {
+ const char *p = paths[i];
+ struct stat sb;
+ int retries = 10 * 1000;
+ int fd;
+
+ if (!p || stat(p, &sb) < 0)
+ continue;
+
+ if (S_ISDIR(sb.st_mode)) {
+ /* Directories can't be locked */
+ continue;
+ }
+
+ if ((fd = open(p, O_RDWR)) < 0) {
+ if (S_ISSOCK(sb.st_mode)) {
+ /* Sockets can be opened only if there exists the
+ * other side that listens. */
+ continue;
+ }
+
+ virReportSystemError(errno,
+ _("unable to open %s"),
+ p);
+ goto cleanup;
+ }
+
+ do {
+ if (virFileLock(fd, false,
+ METADATA_OFFSET, METADATA_LEN, false) < 0) {
+ if (retries && (errno == EACCES || errno == EAGAIN)) {
+ /* File is locked. Try again. */
+ retries--;
+ usleep(1000);
+ continue;
+ } else {
+ virReportSystemError(errno,
+ _("unable to lock %s for metadata
change"),
+ p);
+ VIR_FORCE_CLOSE(fd);
+ goto cleanup;
+ }
+ }
- if (rv >= 0)
break;
+ } while (1);
- if (virGetLastErrorCode() == VIR_ERR_RESOURCE_BUSY)
- continue;
-
- goto cleanup;
+ VIR_APPEND_ELEMENT_COPY_INPLACE(fds, nfds, fd);
}
- if (rv < 0)
+ if (VIR_ALLOC(ret) < 0)
goto cleanup;
- mgr->clientfd = fd;
- fd = -1;
+ VIR_STEAL_PTR(ret->fds, fds);
+ ret->nfds = nfds;
+ nfds = 0;
- ret = 0;
cleanup:
- virLockManagerFree(lock);
- VIR_FORCE_CLOSE(fd);
- if (ret < 0)
- virMutexUnlock(&lockManagerMutex);
+ for (i = nfds; i > 0; i--)
+ VIR_FORCE_CLOSE(fds[i - 1]);
+ VIR_FREE(fds);
return ret;
}
-int
-virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
- const char * const *paths,
- size_t npaths)
+void
+virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
+ virSecurityManagerMetadataLockStatePtr *state)
{
- virLockManagerPtr lock;
- int fd;
- int ret = -1;
+ size_t i;
- /* lockManagerMutex acquired from previous
- * virSecurityManagerMetadataLock() call. */
+ if (!state)
+ return;
- fd = mgr->clientfd;
- mgr->clientfd = -1;
+ for (i = 0; i < (*state)->nfds; i++) {
+ char ebuf[1024];
+ int fd = (*state)->fds[i];
- if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
- goto cleanup;
+ /* Technically, unlock is not needed because it will
+ * happen on VIR_CLOSE() anyway. But let's play it nice. */
+ if (virFileUnlock(fd, METADATA_OFFSET, METADATA_LEN) < 0) {
+ VIR_WARN("Unable to unlock fd %d: %s",
+ fd, virStrerror(errno, ebuf, sizeof(ebuf)));
+ }
- if (virLockManagerRelease(lock, NULL, 0) < 0)
- goto cleanup;
+ if (VIR_CLOSE(fd) < 0) {
+ VIR_WARN("Unable to close fd %d: %s",
+ fd, virStrerror(errno, ebuf, sizeof(ebuf)));
+ }
+ }
- ret = 0;
- cleanup:
- virLockManagerFree(lock);
- VIR_FORCE_CLOSE(fd);
- virMutexUnlock(&lockManagerMutex);
- return ret;
+ VIR_FREE((*state)->fds);
+ VIR_FREE(*state);
}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 04bb54f61e..cacb17174f 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -200,11 +200,16 @@ 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);
+typedef struct _virSecurityManagerMetadataLockState virSecurityManagerMetadataLockState;
+typedef virSecurityManagerMetadataLockState *virSecurityManagerMetadataLockStatePtr;
+
+virSecurityManagerMetadataLockStatePtr
+virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
+ const char **paths,
+ size_t npaths);
+
+void
+virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
+ virSecurityManagerMetadataLockStatePtr *state);
#endif /* VIR_SECURITY_MANAGER_H__ */
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 5e72a3589a..95e9a1b0c7 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -215,6 +215,7 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
void *opaque)
{
virSecuritySELinuxContextListPtr list = opaque;
+ virSecurityManagerMetadataLockStatePtr state;
bool privileged = virSecurityManagerGetPrivileged(list->manager);
const char **paths = NULL;
size_t npaths = 0;
@@ -229,13 +230,10 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
for (i = 0; i < list->nItems; i++) {
const char *p = list->items[i]->path;
- if (virFileIsDir(p))
- continue;
-
VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
}
- if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0)
+ if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths)))
goto cleanup;
}
@@ -253,9 +251,8 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
}
}
- if (list->lock &&
- virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0)
- goto cleanup;
+ if (list->lock)
+ virSecurityManagerMetadataUnlock(list->manager, &state);
if (rv < 0)
goto cleanup;
--
2.18.1