security_manager: Rework metadata locking

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@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>

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 &&
+    if (list->lock)
-        virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0)
+        virSecurityManagerMetadataUnlock(list->manager, &state);
-        goto cleanup;
 
     if (rv < 0)
         goto cleanup;

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
+struct _virSecurityManagerMetadataLockState {
-virSecurityManagerNewLockManager(virSecurityManagerPtr mgr,
+    size_t nfds;
-                                 const char * const *paths,
+    int *fds;
-                                 size_t npaths)
+};
+
+
+static int
+cmpstringp(const void *p1, const void *p2)
 {
-    virLockManagerPtr lock;
+    const char *s1 = *(char * const *) p1;
-    virLockManagerParam params[] = {
+    const char *s2 = *(char * const *) p2;
-        { .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)
+    if (!s1 && !s2)
-        return NULL;
+        return 0;
 
-    if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(mgr->lockPlugin),
+    if (!s1 || !s2)
-                                   VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON,
+        return s2 ? -1 : 1;
-                                   ARRAY_CARDINALITY(params),
-                                   params,
-                                   flags)))
-        return NULL;
 
-    for (i = 0; i < npaths; i++) {
+    /* from man 3 qsort */
-        if (virLockManagerAddResource(lock,
+    return strcmp(s1, s2);
-                                      VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA,
-                                      paths[i], 0, NULL, 0) < 0)
-            goto error;
-    }
-
-    return lock;
- error:
-    virLockManagerFree(lock);
-    return NULL;
 }
 
+#define METADATA_OFFSET 1
+#define METADATA_LEN 1
 
-/* How many seconds should we try to acquire the lock before
+/**
- * giving up. */
+ * virSecurityManagerMetadataLock:
-#define LOCK_ACQUIRE_TIMEOUT 60
+ * @mgr: security manager object
-
+ * @paths: paths to lock
-int
+ * @npaths: number of items in @paths array
-virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
+ *
-                               const char * const *paths,
+ * 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;
+    size_t i = 0;
-    virTimeBackOffVar timebackoff;
+    size_t nfds = 0;
-    int fd = -1;
+    int *fds = NULL;
-    int rv = -1;
+    virSecurityManagerMetadataLockStatePtr ret = NULL;
-    int ret = -1;
 
-    virMutexLock(&lockManagerMutex);
+    if (VIR_ALLOC_N(fds, npaths) < 0)
+        return NULL;
 
-    if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
+    /* Sort paths to lock in order to avoid deadlocks. */
-        goto cleanup;
+    qsort(paths, npaths, sizeof(*paths), cmpstringp);
 
-    if (virTimeBackOffStart(&timebackoff, 1, LOCK_ACQUIRE_TIMEOUT * 1000) < 0)
+    for (i = 0; i < npaths; i++) {
-        goto cleanup;
+        const char *p = paths[i];
-    while (virTimeBackOffWait(&timebackoff)) {
+        struct stat sb;
-        rv = virLockManagerAcquire(lock, NULL,
+        int retries = 10 * 1000;
-                                   VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK,
+        int fd;
-                                   VIR_DOMAIN_LOCK_FAILURE_DEFAULT, &fd);
 
-        if (rv >= 0)
+        if (!p || stat(p, &sb) < 0)
-            break;
-
-        if (virGetLastErrorCode() == VIR_ERR_RESOURCE_BUSY)
             continue;
 
-        goto cleanup;
+        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;
+                }
+            }
+
+            break;
+        } while (1);
+
+        VIR_APPEND_ELEMENT_COPY_INPLACE(fds, nfds, fd);
     }
 
-    if (rv < 0)
+    if (VIR_ALLOC(ret) < 0)
         goto cleanup;
 
-    mgr->clientfd = fd;
+    VIR_STEAL_PTR(ret->fds, fds);
-    fd = -1;
+    ret->nfds = nfds;
+    nfds = 0;
 
-    ret = 0;
  cleanup:
-    virLockManagerFree(lock);
+    for (i = nfds; i > 0; i--)
-    VIR_FORCE_CLOSE(fd);
+        VIR_FORCE_CLOSE(fds[i - 1]);
-    if (ret < 0)
+    VIR_FREE(fds);
-        virMutexUnlock(&lockManagerMutex);
     return ret;
 }
 
 
-int
+void
-virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
+virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
-                                 const char * const *paths,
+                                 virSecurityManagerMetadataLockStatePtr *state)
-                                 size_t npaths)
 {
-    virLockManagerPtr lock;
+    size_t i;
-    int fd;
-    int ret = -1;
 
-    /* lockManagerMutex acquired from previous
+    if (!state)
-     * virSecurityManagerMetadataLock() call. */
+        return;
 
-    fd = mgr->clientfd;
+    for (i = 0; i < (*state)->nfds; i++) {
-    mgr->clientfd = -1;
+        char ebuf[1024];
+        int fd = (*state)->fds[i];
 
-    if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
+        /* Technically, unlock is not needed because it will
-        goto cleanup;
+         * 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)
+        if (VIR_CLOSE(fd) < 0) {
-        goto cleanup;
+            VIR_WARN("Unable to close fd %d: %s",
+                     fd, virStrerror(errno, ebuf, sizeof(ebuf)));
+        }
+    }
 
-    ret = 0;
+    VIR_FREE((*state)->fds);
- cleanup:
+    VIR_FREE(*state);
-    virLockManagerFree(lock);
-    VIR_FORCE_CLOSE(fd);
-    virMutexUnlock(&lockManagerMutex);
-    return ret;
 }

src/security/security_manager.h

@@ -200,11 +200,16 @@ int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr,
 int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
                                        virDomainDefPtr vm);
 
-int virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
+typedef struct _virSecurityManagerMetadataLockState virSecurityManagerMetadataLockState;
-                                   const char * const *paths,
+typedef virSecurityManagerMetadataLockState *virSecurityManagerMetadataLockStatePtr;
-                                   size_t npaths);
+
-int virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
+virSecurityManagerMetadataLockStatePtr
-                                     const char * const *paths,
+virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
-                                     size_t npaths);
+                               const char **paths,
+                               size_t npaths);
+
+void
+virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
+                                 virSecurityManagerMetadataLockStatePtr *state);
 
 #endif /* VIR_SECURITY_MANAGER_H__ */

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 &&
+    if (list->lock)
-        virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0)
+        virSecurityManagerMetadataUnlock(list->manager, &state);
-        goto cleanup;
 
     if (rv < 0)
         goto cleanup;