4:02 a.m.
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(a)redhat.com>
---
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