[PATCH v1 2/2] security_dac: Lock label store file

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



If there are two or more libvirtds fighting over the label store file
a data corruption is likely to occur. Therefore we need to enforce an
exclusive access to the file. With a little help from virtlockd we can
achieve that.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
 src/Makefile.am                  |  3 +-
 src/locking/lock_driver.h        |  2 +
 src/locking/lock_driver_lockd.c  | 25 ++++++++++++
 src/lxc/lxc_controller.c         |  2 +-
 src/lxc/lxc_driver.c             |  3 +-
 src/qemu/qemu_driver.c           |  9 +++--
 src/security/security_dac.c      | 85 ++++++++++++++++++++++++++++++++++++++--
 src/security/security_manager.c  | 24 +++++++++---
 src/security/security_manager.h  |  7 +++-
 tests/qemuhotplugtest.c          |  2 +-
 tests/seclabeltest.c             |  2 +-
 tests/securityselinuxlabeltest.c |  3 +-
 tests/securityselinuxtest.c      |  2 +-
 13 files changed, 147 insertions(+), 22 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index d4d7b2b..eab1dbf 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1555,7 +1555,7 @@ libvirt_security_manager_la_SOURCES = $(SECURITY_DRIVER_SOURCES)
 noinst_LTLIBRARIES += libvirt_security_manager.la
 libvirt_la_BUILT_LIBADD += libvirt_security_manager.la
 libvirt_security_manager_la_CFLAGS = \
-		-I$(top_srcdir)/src/conf $(AM_CFLAGS)
+		-I$(top_srcdir)/src/conf -I$(top_srcdir)/src/locking $(AM_CFLAGS)
 libvirt_security_manager_la_LDFLAGS = $(AM_LDFLAGS)
 libvirt_security_manager_la_LIBADD = $(SECDRIVER_LIBS)
 if WITH_SECDRIVER_SELINUX
@@ -2512,6 +2512,7 @@ libvirt_lxc_LDADD =			\
 		libvirt_security_manager.la \
 		libvirt_conf.la \
 		libvirt_util.la \
+		libvirt_driver.la \
 		../gnulib/lib/libgnu.la
 if WITH_DTRACE_PROBES
 libvirt_lxc_LDADD += libvirt_probes.lo
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index a7d9782..6bcb3cc 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 DAC driver label store file */
+    VIR_LOCK_MANAGER_OBJECT_TYPE_DAC = (1 << 0),
 } virLockManagerObjectType;
 
 typedef enum {
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 1b92d68..53f8ed6 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -494,6 +494,31 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock,
         }
         break;
 
+    case VIR_LOCK_MANAGER_OBJECT_TYPE_DAC:
+        for (i = 0; i < nparams; i++) {
+            if (STREQ(params[i].key, "name")) {
+                if (VIR_STRDUP(priv->name, params[i].value.str) < 0)
+                    return -1;
+            } else if (STREQ(params[i].key, "pid")) {
+                priv->pid = params[i].value.iv;
+            } else {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("Unexpected parameter %s for object"),
+                               params[i].key);
+            }
+        }
+        if (!priv->name) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Missing name parameter for DAC object"));
+            return -1;
+        }
+        if (priv->pid == 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Missing PID parameter for DAC object"));
+            return -1;
+        }
+        break;
+
     default:
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Unknown lock manager object type %d"),
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 5ca960f..1a85ebc 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -2392,7 +2392,7 @@ int main(int argc, char *argv[])
 
     if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver,
                                                         LXC_DRIVER_NAME,
-                                                        false, false, false)))
+                                                        false, false, false, NULL)))
         goto cleanup;
 
     if (ctrl->def->seclabels) {
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 0ab1ba2..9b4d3ff 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1485,7 +1485,8 @@ lxcSecurityInit(virLXCDriverConfigPtr cfg)
                                                       LXC_DRIVER_NAME,
                                                       false,
                                                       cfg->securityDefaultConfined,
-                                                      cfg->securityRequireConfined);
+                                                      cfg->securityRequireConfined,
+                                                      NULL);
     if (!mgr)
         goto error;
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ba470a1..25c1c57 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -338,7 +338,8 @@ qemuSecurityInit(virQEMUDriverPtr driver)
                                               QEMU_DRIVER_NAME,
                                               cfg->allowDiskFormatProbing,
                                               cfg->securityDefaultConfined,
-                                              cfg->securityRequireConfined)))
+                                              cfg->securityRequireConfined,
+                                              driver->lockManager)))
                 goto error;
             if (!stack) {
                 if (!(stack = virSecurityManagerNewStack(mgr)))
@@ -355,7 +356,8 @@ qemuSecurityInit(virQEMUDriverPtr driver)
                                           QEMU_DRIVER_NAME,
                                           cfg->allowDiskFormatProbing,
                                           cfg->securityDefaultConfined,
-                                          cfg->securityRequireConfined)))
+                                          cfg->securityRequireConfined,
+                                          driver->lockManager)))
             goto error;
         if (!(stack = virSecurityManagerNewStack(mgr)))
             goto error;
@@ -369,7 +371,8 @@ qemuSecurityInit(virQEMUDriverPtr driver)
                                              cfg->allowDiskFormatProbing,
                                              cfg->securityDefaultConfined,
                                              cfg->securityRequireConfined,
-                                             cfg->dynamicOwnership)))
+                                             cfg->dynamicOwnership,
+                                             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 b119825..5527287 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -35,6 +35,8 @@
 #include "virstring.h"
 #include "virutil.h"
 #include "configmake.h"
+#include "lock_driver.h"
+#include "lock_manager.h"
 
 #define VIR_FROM_THIS VIR_FROM_SECURITY
 #define SECURITY_DAC_NAME "dac"
@@ -50,6 +52,7 @@ struct _virSecurityDACData {
     bool dynamicOwnership;
     char *baselabel;
     char *labelStore;
+    virLockManagerPluginPtr plugin;
 };
 
 typedef struct _virDACLabel virDACLabel;
@@ -183,6 +186,75 @@ cleanup:
     return ret;
 }
 
+static int
+virSecurityDACLabelStoreLock(virSecurityDACDataPtr priv,
+                             bool lock)
+{
+    int ret = -1;
+    const unsigned int max_retries = 10;
+    unsigned int retries;
+    virLockManagerPtr lockMgr = NULL;
+    virLockManagerParam params[] = {
+        { .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING,
+            .key = "name",
+            .value = { .str = priv->labelStore },
+        },
+        { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT,
+            .key = "pid",
+            .value = { .iv = getppid() },
+        },
+    };
+
+    if (!(lockMgr = virLockManagerNew(virLockManagerPluginGetDriver(priv->plugin),
+                                      VIR_LOCK_MANAGER_OBJECT_TYPE_DAC,
+                                      ARRAY_CARDINALITY(params),
+                                      params,
+                                      0)))
+        goto cleanup;
+
+    if (virLockManagerAddResource(lockMgr,
+                                  VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
+                                  priv->labelStore,
+                                  0,
+                                  NULL,
+                                  0) < 0)
+        goto cleanup;
+
+    if (lock) {
+        for (retries = 0; retries < max_retries; retries++) {
+            struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
+            int rv;
+
+            rv = virLockManagerAcquire(lockMgr, NULL, 0,
+                                       VIR_DOMAIN_LOCK_FAILURE_IGNORE, NULL);
+
+            if (rv == 0)
+                break;
+
+            nanosleep(&ts, NULL);
+        }
+
+        if (retries == max_retries) {
+            virReportError(VIR_ERR_OPERATION_TIMEOUT,
+                           _("unable to lock %s"),
+                           priv->labelStore);
+            goto cleanup;
+        }
+    } else {
+        if (virLockManagerRelease(lockMgr, NULL, 0) < 0) {
+            virReportError(VIR_ERR_OPERATION_FAILED,
+                           _("unable to unlock %s"),
+                           priv->labelStore);
+            goto cleanup;
+        }
+    }
+
+    ret = 0;
+cleanup:
+    virLockManagerFree(lockMgr);
+    return ret;
+}
+
 /* returns -1 on error, 0 on success */
 int
 virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr,
@@ -350,6 +422,7 @@ virSecurityDACOpen(virSecurityManagerPtr mgr)
     if (VIR_STRDUP(priv->labelStore, LOCALSTATEDIR "/run/libvirt/labelStore.xml") < 0)
         return -1;
 
+    priv->plugin = virSecurityManagerGetLockPlugin(mgr);
     return 0;
 }
 
@@ -401,7 +474,9 @@ virSecurityDACRememberLabel(virSecurityManagerPtr mgr,
     struct stat sb;
     virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
 
-    /* TODO lock the labelStore file */
+    if (virSecurityDACLabelStoreLock(priv, true) < 0)
+        return ret;
+
     if (virSecurityDACLabelStoreParse(priv, &labels, &labels_size) < 0)
         goto cleanup;
 
@@ -433,7 +508,7 @@ virSecurityDACRememberLabel(virSecurityManagerPtr mgr,
 
     ret = 0;
 cleanup:
-    /* TODO unlock the labelStore file */
+    virSecurityDACLabelStoreLock(priv, false);
     virSecurityDACLabelStoreFree(labels, labels_size);
     return ret;
 }
@@ -450,7 +525,9 @@ virSecurityDACRecallLabel(virSecurityManagerPtr mgr,
     size_t labels_size = 0;
     virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
 
-    /* TODO lock the labelStore file */
+    if (virSecurityDACLabelStoreLock(priv, true) < 0)
+        return ret;
+
     if (virSecurityDACLabelStoreParse(priv, &labels, &labels_size) < 0)
         goto cleanup;
 
@@ -477,7 +554,7 @@ virSecurityDACRecallLabel(virSecurityManagerPtr mgr,
 
     ret = 0;
 cleanup:
-    /* TODO unlock the labelStore file */
+    virSecurityDACLabelStoreLock(priv, false);
     virSecurityDACLabelStoreFree(labels, labels_size);
     return ret;
 }
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 5ecf72f..e5a67f2 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -43,6 +43,7 @@ struct _virSecurityManager {
     bool requireConfined;
     const char *virtDriver;
     void *privateData;
+    void *lockPlugin;
 };
 
 static virClassPtr virSecurityManagerClass;
@@ -66,7 +67,8 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr
                                                          const char *virtDriver,
                                                          bool allowDiskFormatProbing,
                                                          bool defaultConfined,
-                                                         bool requireConfined)
+                                                         bool requireConfined,
+                                                         void *lockPlugin)
 {
     virSecurityManagerPtr mgr;
     char *privateData;
@@ -94,6 +96,7 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr
     mgr->requireConfined = requireConfined;
     mgr->virtDriver = virtDriver;
     mgr->privateData = privateData;
+    mgr->lockPlugin = lockPlugin;
 
     if (drv->open(mgr) < 0) {
         virObjectUnref(mgr);
@@ -110,7 +113,8 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary)
                                     virSecurityManagerGetDriver(primary),
                                     virSecurityManagerGetAllowDiskFormatProbing(primary),
                                     virSecurityManagerGetDefaultConfined(primary),
-                                    virSecurityManagerGetRequireConfined(primary));
+                                    virSecurityManagerGetRequireConfined(primary),
+                                    virSecurityManagerGetLockPlugin(primary));
 
     if (!mgr)
         return NULL;
@@ -134,14 +138,16 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver,
                                                bool allowDiskFormatProbing,
                                                bool defaultConfined,
                                                bool requireConfined,
-                                               bool dynamicOwnership)
+                                               bool dynamicOwnership,
+                                               void *lockPlugin)
 {
     virSecurityManagerPtr mgr =
         virSecurityManagerNewDriver(&virSecurityDriverDAC,
                                     virtDriver,
                                     allowDiskFormatProbing,
                                     defaultConfined,
-                                    requireConfined);
+                                    requireConfined,
+                                    lockPlugin);
 
     if (!mgr)
         return NULL;
@@ -159,7 +165,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name,
                                             const char *virtDriver,
                                             bool allowDiskFormatProbing,
                                             bool defaultConfined,
-                                            bool requireConfined)
+                                            bool requireConfined,
+                                            void *lockPlugin)
 {
     virSecurityDriverPtr drv = virSecurityDriverLookup(name, virtDriver);
     if (!drv)
@@ -189,7 +196,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name,
                                        virtDriver,
                                        allowDiskFormatProbing,
                                        defaultConfined,
-                                       requireConfined);
+                                       requireConfined,
+                                       lockPlugin);
 }
 
 
@@ -229,6 +237,10 @@ void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr)
     return mgr->privateData;
 }
 
+void *virSecurityManagerGetLockPlugin(virSecurityManagerPtr mgr)
+{
+    return mgr->lockPlugin;
+}
 
 static void virSecurityManagerDispose(void *obj)
 {
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 81d3160..52b2ff0 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -33,7 +33,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name,
                                             const char *virtDriver,
                                             bool allowDiskFormatProbing,
                                             bool defaultConfined,
-                                            bool requireConfined);
+                                            bool requireConfined,
+                                            void *lockPlugin);
 
 virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary);
 int virSecurityManagerStackAddNested(virSecurityManagerPtr stack,
@@ -45,12 +46,14 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver,
                                                bool allowDiskFormatProbing,
                                                bool defaultConfined,
                                                bool requireConfined,
-                                               bool dynamicOwnership);
+                                               bool dynamicOwnership,
+                                               void *lockPlugin);
 
 int virSecurityManagerPreFork(virSecurityManagerPtr mgr);
 void virSecurityManagerPostFork(virSecurityManagerPtr mgr);
 
 void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr);
+void *virSecurityManagerGetLockPlugin(virSecurityManagerPtr mgr);
 
 const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr);
 const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr);
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 8036adc..041cf36 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -353,7 +353,7 @@ mymain(void)
     if (!driver.lockManager)
         return EXIT_FAILURE;
 
-    if (!(mgr = virSecurityManagerNew("none", "qemu", false, false, false)))
+    if (!(mgr = virSecurityManagerNew("none", "qemu", false, false, false, NULL)))
         return EXIT_FAILURE;
     if (!(driver.securityManager = virSecurityManagerNewStack(mgr)))
         return EXIT_FAILURE;
diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c
index cd34b6b..e21a8c1 100644
--- a/tests/seclabeltest.c
+++ b/tests/seclabeltest.c
@@ -17,7 +17,7 @@ main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED)
     if (virThreadInitialize() < 0)
         return EXIT_FAILURE;
 
-    mgr = virSecurityManagerNew(NULL, "QEMU", false, true, false);
+    mgr = virSecurityManagerNew(NULL, "QEMU", false, true, false, NULL);
     if (mgr == NULL) {
         fprintf(stderr, "Failed to start security driver");
         return EXIT_FAILURE;
diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index 3505f8c..8bd2a5b 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -320,7 +320,8 @@ mymain(void)
 {
     int ret = 0;
 
-    if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false))) {
+    if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false,
+                                      true, false, NULL))) {
         virErrorPtr err = virGetLastError();
         fprintf(stderr, "Unable to initialize security driver: %s\n",
                 err->message);
diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c
index feb5366..99980f4 100644
--- a/tests/securityselinuxtest.c
+++ b/tests/securityselinuxtest.c
@@ -270,7 +270,7 @@ mymain(void)
     int ret = 0;
     virSecurityManagerPtr mgr;
 
-    if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false))) {
+    if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false, NULL))) {
         virErrorPtr err = virGetLastError();
         fprintf(stderr, "Unable to initialize security driver: %s\n",
                 err->message);
-- 
1.9.0

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]