Even though the current use of the function does not require full implementation with transactions (none of the callers pass a path somewhere under /dev), it doesn't hurt either. Moreover, in future patches the paradigm is going to shift so that any API that touches a file is required to use transactions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 3 +-- src/qemu/qemu_process.c | 15 ++++++--------- src/qemu/qemu_security.c | 30 ++++++++++++++++++++++++++++++ src/qemu/qemu_security.h | 6 +++++- 4 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5329899b13..6425c886a3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -808,8 +808,7 @@ qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver, goto cleanup; } - if (qemuSecurityDomainSetPathLabel(driver->securityManager, - vm->def, path, false) < 0) + if (qemuSecurityDomainSetPathLabel(driver, vm, path, false) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index eb9904b7ba..3820e04f91 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2790,8 +2790,7 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm) virCgroupAddMachineTask(priv->cgroup, cpid) < 0) goto cleanup; - if (qemuSecurityDomainSetPathLabel(driver->securityManager, - vm->def, socketPath, true) < 0) + if (qemuSecurityDomainSetPathLabel(driver, vm, socketPath, true) < 0) goto cleanup; priv->prDaemonRunning = true; @@ -3653,7 +3652,7 @@ qemuProcessNeedMemoryBackingPath(virDomainDefPtr def, static int qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver, - virDomainDefPtr def, + virDomainObjPtr vm, const char *path, bool build) { @@ -3668,8 +3667,7 @@ qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver, return -1; } - if (qemuSecurityDomainSetPathLabel(driver->securityManager, - def, path, true) < 0) + if (qemuSecurityDomainSetPathLabel(driver, vm, path, true) < 0) return -1; } else { if (virFileDeleteTree(path) < 0) @@ -3705,7 +3703,7 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, if (!path) goto cleanup; - if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def, + if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm, path, build) < 0) goto cleanup; @@ -3717,7 +3715,7 @@ qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver, if (qemuGetMemoryBackingDomainPath(vm->def, cfg, &path) < 0) goto cleanup; - if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm->def, + if (qemuProcessBuildDestroyMemoryPathsImpl(driver, vm, path, build) < 0) goto cleanup; @@ -4909,8 +4907,7 @@ qemuProcessMakeDir(virQEMUDriverPtr driver, goto cleanup; } - if (qemuSecurityDomainSetPathLabel(driver->securityManager, - vm->def, path, true) < 0) + if (qemuSecurityDomainSetPathLabel(driver, vm, path, true) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index af3be42854..268def309a 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -493,3 +493,33 @@ qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver, { virSecurityManagerRestoreTPMLabels(driver->securityManager, def); } + + +int +qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, + bool allowSubtree) +{ + int ret = -1; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup; + + if (virSecurityManagerDomainSetPathLabel(driver->securityManager, + vm->def, + path, + allowSubtree) < 0) + goto cleanup; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionCommit(driver->securityManager, + vm->pid) < 0) + goto cleanup; + + ret = 0; + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); + return ret; +} diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index a189b63828..fd11fbdd9d 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -95,12 +95,16 @@ int qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, void qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver, virDomainDefPtr def); +int qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, + bool allowSubtree); + /* Please note that for these APIs there is no wrapper yet. Do NOT blindly add * new APIs here. If an API can touch a /dev file add a proper wrapper instead. */ # define qemuSecurityCheckAllLabel virSecurityManagerCheckAllLabel # define qemuSecurityClearSocketLabel virSecurityManagerClearSocketLabel -# define qemuSecurityDomainSetPathLabel virSecurityManagerDomainSetPathLabel # define qemuSecurityGenLabel virSecurityManagerGenLabel # define qemuSecurityGetBaseLabel virSecurityManagerGetBaseLabel # define qemuSecurityGetDOI virSecurityManagerGetDOI -- 2.16.4

Even though the current use of the functions does not require full implementation with transactions (none of the callers passes a path somewhere under /dev), it doesn't hurt either. Moreover, in future patches the paradigm is going to shift so that any API that touches a file is required to use transactions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 7 +++--- src/qemu/qemu_security.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_security.h | 10 +++++++-- 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2f8d6915e1..6763c8cddc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4043,7 +4043,7 @@ qemuDomainScreenshot(virDomainPtr dom, } unlink_tmp = true; - qemuSecuritySetSavedStateLabel(driver->securityManager, vm->def, tmp); + qemuSecuritySetSavedStateLabel(driver, vm, tmp); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorScreendump(priv->mon, videoAlias, screen, tmp) < 0) { @@ -6662,8 +6662,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, virObjectUnref(cookie); virCommandFree(cmd); VIR_FREE(errbuf); - if (qemuSecurityRestoreSavedStateLabel(driver->securityManager, - vm->def, path) < 0) + if (qemuSecurityRestoreSavedStateLabel(driver, vm, path) < 0) VIR_WARN("failed to restore save state label on %s", path); virObjectUnref(cfg); return ret; @@ -11828,7 +11827,7 @@ qemuDomainMemoryPeek(virDomainPtr dom, goto endjob; } - qemuSecuritySetSavedStateLabel(driver->securityManager, vm->def, tmp); + qemuSecuritySetSavedStateLabel(driver, vm, tmp); priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 268def309a..c64fbdda38 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -523,3 +523,59 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, virSecurityManagerTransactionAbort(driver->securityManager); return ret; } + + +int +qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *savefile) +{ + int ret = -1; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup; + + if (virSecurityManagerSetSavedStateLabel(driver->securityManager, + vm->def, + savefile) < 0) + goto cleanup; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionCommit(driver->securityManager, + vm->pid) < 0) + goto cleanup; + + ret = 0; + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); + return ret; +} + + +int +qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *savefile) +{ + int ret = -1; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup; + + if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager, + vm->def, + savefile) < 0) + goto cleanup; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionCommit(driver->securityManager, + vm->pid) < 0) + goto cleanup; + + ret = 0; + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); + return ret; +} diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index fd11fbdd9d..c57774deba 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -100,6 +100,14 @@ int qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, const char *path, bool allowSubtree); +int qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *savefile); + +int qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *savefile); + /* Please note that for these APIs there is no wrapper yet. Do NOT blindly add * new APIs here. If an API can touch a /dev file add a proper wrapper instead. */ @@ -119,11 +127,9 @@ int qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, # define qemuSecurityPreFork virSecurityManagerPreFork # define qemuSecurityReleaseLabel virSecurityManagerReleaseLabel # define qemuSecurityReserveLabel virSecurityManagerReserveLabel -# define qemuSecurityRestoreSavedStateLabel virSecurityManagerRestoreSavedStateLabel # define qemuSecuritySetChildProcessLabel virSecurityManagerSetChildProcessLabel # define qemuSecuritySetDaemonSocketLabel virSecurityManagerSetDaemonSocketLabel # define qemuSecuritySetImageFDLabel virSecurityManagerSetImageFDLabel -# define qemuSecuritySetSavedStateLabel virSecurityManagerSetSavedStateLabel # define qemuSecuritySetSocketLabel virSecurityManagerSetSocketLabel # define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel # define qemuSecurityStackAddNested virSecurityManagerStackAddNested -- 2.16.4

In the future, the transactions are not going to be optional and they will be run regardless of domain using namespace to collect list of paths to be relabeled. To make sure there won't be an API that goes behind transaction code back update the comment that serves as decision manual whether an API must be fully implemented or plain #define is sufficient. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index c57774deba..ba12eb3caf 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -109,7 +109,7 @@ int qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, const char *savefile); /* Please note that for these APIs there is no wrapper yet. Do NOT blindly add - * new APIs here. If an API can touch a /dev file add a proper wrapper instead. + * new APIs here. If an API can touch a file add a proper wrapper instead. */ # define qemuSecurityCheckAllLabel virSecurityManagerCheckAllLabel # define qemuSecurityClearSocketLabel virSecurityManagerClearSocketLabel -- 2.16.4

It will be desirable to run transactions more often than we currently do. Even if the domain we're relabeling the paths for does not run in a namespace. If that's the case, there is no need to fork() as we are already running in the right namespace. To differentiate whether transaction code should fork() or not the @pid argument now accepts -1 (which means do not fork). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 22 ++++++++++++++-------- src/security/security_manager.c | 14 +++++++++----- src/security/security_selinux.c | 23 +++++++++++++++-------- 3 files changed, 38 insertions(+), 21 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2a5f8639fe..926c9a33c1 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -485,11 +485,14 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr) * @mgr: security manager * @pid: domain's PID * - * Enters the @pid namespace (usually @pid refers to a domain) and - * performs all the chown()-s on the list. Note that the transaction is - * also freed, therefore new one has to be started after successful - * return from this function. Also it is considered as error if there's - * no transaction set and this function is called. + * If @pid is not -1 then enter the @pid namespace (usually @pid refers + * to a domain) and perform all the chown()-s on the list. If @pid is -1 + * then the transaction is performed in the namespace of the caller. + * + * Note that the transaction is also freed, therefore new one has to be + * started after successful return from this function. Also it is + * considered as error if there's no transaction set and this function + * is called. * * Returns: 0 on success, * -1 otherwise. @@ -514,9 +517,12 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto cleanup; } - if (virProcessRunInMountNamespace(pid, - virSecurityDACTransactionRun, - list) < 0) + if ((pid == -1 && + virSecurityDACTransactionRun(pid, list) < 0) || + (pid != -1 && + virProcessRunInMountNamespace(pid, + virSecurityDACTransactionRun, + list) < 0)) goto cleanup; ret = 0; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 21eb6f7452..9f770d8c53 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -267,11 +267,15 @@ virSecurityManagerTransactionStart(virSecurityManagerPtr mgr) * @mgr: security manager * @pid: domain's PID * - * Enters the @pid namespace (usually @pid refers to a domain) and - * performs all the operations on the transaction list. Note that the - * transaction is also freed, therefore new one has to be started after - * successful return from this function. Also it is considered as error - * if there's no transaction set and this function is called. + * If @pid is not -1 then enter the @pid namespace (usually @pid refers + * to a domain) and perform all the operations on the transaction list. + * If @pid is -1 then the transaction is performed in the namespace of + * the caller. + * + * Note that the transaction is also freed, therefore new one has to be + * started after successful return from this function. Also it is + * considered as error if there's no transaction set and this function + * is called. * * Returns: 0 on success, * -1 otherwise. diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 96944d0202..288f3628f7 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1040,11 +1040,15 @@ virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr) * @mgr: security manager * @pid: domain's PID * - * Enters the @pid namespace (usually @pid refers to a domain) and - * performs all the sefilecon()-s on the list. Note that the - * transaction is also freed, therefore new one has to be started after - * successful return from this function. Also it is considered as error - * if there's no transaction set and this function is called. + * If @pis is not -1 then enter the @pid namespace (usually @pid refers + * to a domain) and perform all the sefilecon()-s on the list. If @pid + * is -1 then the transaction is performed in the namespace of the + * caller. + * + * Note that the transaction is also freed, therefore new one has to be + * started after successful return from this function. Also it is + * considered as error if there's no transaction set and this function + * is called. * * Returns: 0 on success, * -1 otherwise. @@ -1066,9 +1070,12 @@ virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto cleanup; } - if (virProcessRunInMountNamespace(pid, - virSecuritySELinuxTransactionRun, - list) < 0) + if ((pid == -1 && + virSecuritySELinuxTransactionRun(pid, list) < 0) || + (pid != -1 && + virProcessRunInMountNamespace(pid, + virSecuritySELinuxTransactionRun, + list) < 0)) goto cleanup; ret = 0; -- 2.16.4

And by "more frequently" I mean always. This is needed so that we have a single place where all the paths a thread wants to relabel are stored. This enables us to lock them all at once (for metadata), do the relabel and unlock at once again. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 216 ++++++++++++++++++++++++++++------------------- 1 file changed, 129 insertions(+), 87 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index c64fbdda38..b538e08616 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -39,9 +39,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + pid_t pid = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetAllLabel(driver->securityManager, @@ -50,9 +53,7 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -69,16 +70,21 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; - /* In contrast to qemuSecuritySetAllLabel, do not use - * secdriver transactions here. This function is called from - * qemuProcessStop() which is meant to do cleanup after qemu - * process died. If it did do, the namespace is gone as qemu - * was the only process running there. We would not succeed - * in entering the namespace then. */ + /* In contrast to qemuSecuritySetAllLabel, do not use vm->pid + * here. This function is called from qemuProcessStop() which + * is meant to do cleanup after qemu process died. The + * domain's namespace is gone as qemu was the only process + * running there. We would not succeed in entering the + * namespace then. */ + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + return; + virSecurityManagerRestoreAllLabel(driver->securityManager, vm->def, migrated, priv->chardevStdioLogd); + + virSecurityManagerTransactionCommit(driver->securityManager, -1); } @@ -87,10 +93,13 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetDiskLabel(driver->securityManager, @@ -98,9 +107,7 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, disk) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -115,10 +122,13 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreDiskLabel(driver->securityManager, @@ -126,9 +136,7 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, disk) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -143,10 +151,13 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetImageLabel(driver->securityManager, @@ -154,9 +165,7 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, src) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -171,10 +180,13 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreImageLabel(driver->securityManager, @@ -182,9 +194,7 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, src) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -199,10 +209,13 @@ qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetHostdevLabel(driver->securityManager, @@ -211,9 +224,7 @@ qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, NULL) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -228,10 +239,13 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, @@ -240,9 +254,7 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, NULL) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -257,10 +269,13 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetMemoryLabel(driver->securityManager, @@ -268,9 +283,7 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver, mem) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -285,10 +298,13 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreMemoryLabel(driver->securityManager, @@ -296,9 +312,7 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver, mem) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -314,10 +328,13 @@ qemuSecuritySetInputLabel(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetInputLabel(driver->securityManager, @@ -325,9 +342,7 @@ qemuSecuritySetInputLabel(virDomainObjPtr vm, input) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -343,10 +358,13 @@ qemuSecurityRestoreInputLabel(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreInputLabel(driver->securityManager, @@ -354,9 +372,7 @@ qemuSecurityRestoreInputLabel(virDomainObjPtr vm, input) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -373,9 +389,12 @@ qemuSecuritySetChardevLabel(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + pid_t pid = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetChardevLabel(driver->securityManager, @@ -384,9 +403,7 @@ qemuSecuritySetChardevLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -403,9 +420,12 @@ qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + pid_t pid = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreChardevLabel(driver->securityManager, @@ -414,9 +434,7 @@ qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -454,10 +472,21 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, int *cmdret) { int ret = -1; + bool transactionStarted = false; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + return -1; + transactionStarted = true; if (virSecurityManagerSetTPMLabels(driver->securityManager, - def) < 0) + def) < 0) { + virSecurityManagerTransactionAbort(driver->securityManager); + return -1; + } + + if (virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) goto cleanup; + transactionStarted = false; if (virSecurityManagerSetChildProcessLabel(driver->securityManager, def, cmd) < 0) @@ -481,8 +510,13 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, return 0; cleanup: + if (!transactionStarted) + virSecurityManagerTransactionStart(driver->securityManager); + virSecurityManagerRestoreTPMLabels(driver->securityManager, def); + virSecurityManagerTransactionCommit(driver->securityManager, -1); + virSecurityManagerTransactionAbort(driver->securityManager); return ret; } @@ -491,7 +525,12 @@ void qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver, virDomainDefPtr def) { + virSecurityManagerTransactionStart(driver->securityManager); + virSecurityManagerRestoreTPMLabels(driver->securityManager, def); + + virSecurityManagerTransactionCommit(driver->securityManager, -1); + virSecurityManagerTransactionAbort(driver->securityManager); } @@ -501,10 +540,13 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, const char *path, bool allowSubtree) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerDomainSetPathLabel(driver->securityManager, @@ -513,9 +555,7 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, allowSubtree) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -530,10 +570,13 @@ qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *savefile) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetSavedStateLabel(driver->securityManager, @@ -541,9 +584,7 @@ qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, savefile) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -558,10 +599,13 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *savefile) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager, @@ -569,9 +613,7 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, savefile) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; -- 2.16.4

So far the virLockSpaceAcquireResource() locks the first byte in the underlying file. But caller might want to lock other range. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/locking/lock_daemon_dispatch.c | 3 +++ src/util/virlockspace.c | 15 ++++++++++----- src/util/virlockspace.h | 4 ++++ tests/virlockspacetest.c | 29 ++++++++++++++++++++++++----- 4 files changed, 41 insertions(+), 10 deletions(-) diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 1b479db55d..10248ec0b5 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -50,6 +50,8 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU virNetServerClientGetPrivateData(client); virLockSpacePtr lockspace; unsigned int newFlags; + off_t start = 0; + off_t len = 1; virMutexLock(&priv->lock); @@ -84,6 +86,7 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU if (virLockSpaceAcquireResource(lockspace, args->name, priv->ownerPid, + start, len, newFlags) < 0) goto cleanup; diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 3364c843aa..60bfef4c5f 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -115,8 +115,10 @@ static void virLockSpaceResourceFree(virLockSpaceResourcePtr res) static virLockSpaceResourcePtr virLockSpaceResourceNew(virLockSpacePtr lockspace, const char *resname, - unsigned int flags, - pid_t owner) + pid_t owner, + off_t start, + off_t len, + unsigned int flags) { virLockSpaceResourcePtr res; bool shared = !!(flags & VIR_LOCK_SPACE_ACQUIRE_SHARED); @@ -157,7 +159,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; } - if (virFileLock(res->fd, shared, 0, 1, false) < 0) { + if (virFileLock(res->fd, shared, start, len, false) < 0) { if (errno == EACCES || errno == EAGAIN) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), @@ -204,7 +206,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; } - if (virFileLock(res->fd, shared, 0, 1, false) < 0) { + if (virFileLock(res->fd, shared, start, len, false) < 0) { if (errno == EACCES || errno == EAGAIN) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), @@ -612,6 +614,8 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace, int virLockSpaceAcquireResource(virLockSpacePtr lockspace, const char *resname, pid_t owner, + off_t start, + off_t len, unsigned int flags) { int ret = -1; @@ -641,7 +645,8 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace, goto cleanup; } - if (!(res = virLockSpaceResourceNew(lockspace, resname, flags, owner))) + if (!(res = virLockSpaceResourceNew(lockspace, resname, + owner, start, len, flags))) goto cleanup; if (virHashAddEntry(lockspace->resources, resname, res) < 0) { diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h index 041cf20396..24f2c89be6 100644 --- a/src/util/virlockspace.h +++ b/src/util/virlockspace.h @@ -22,6 +22,8 @@ #ifndef __VIR_LOCK_SPACE_H__ # define __VIR_LOCK_SPACE_H__ +# include <sys/types.h> + # include "internal.h" # include "virjson.h" @@ -50,6 +52,8 @@ typedef enum { int virLockSpaceAcquireResource(virLockSpacePtr lockspace, const char *resname, pid_t owner, + off_t start, + off_t len, unsigned int flags); int virLockSpaceReleaseResource(virLockSpacePtr lockspace, diff --git a/tests/virlockspacetest.c b/tests/virlockspacetest.c index 75ad98a02c..2409809353 100644 --- a/tests/virlockspacetest.c +++ b/tests/virlockspacetest.c @@ -99,6 +99,8 @@ static int testLockSpaceResourceLockExcl(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; + const off_t start = 0; + const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -111,13 +113,13 @@ static int testLockSpaceResourceLockExcl(const void *args ATTRIBUTE_UNUSED) if (virLockSpaceCreateResource(lockspace, "foo") < 0) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) < 0) + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), start, len, 0) < 0) goto cleanup; if (!virFileExists(LOCKSPACE_DIR "/foo")) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) == 0) + if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), start, len, 0) == 0) goto cleanup; if (virLockSpaceDeleteResource(lockspace, "foo") == 0) @@ -145,6 +147,8 @@ static int testLockSpaceResourceLockExclAuto(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; + const off_t start = 0; + const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -158,6 +162,7 @@ static int testLockSpaceResourceLockExclAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) goto cleanup; @@ -183,6 +188,8 @@ static int testLockSpaceResourceLockShr(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; + const off_t start = 0; + const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -196,13 +203,16 @@ static int testLockSpaceResourceLockShr(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED) < 0) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) == 0) + if (virLockSpaceAcquireResource(lockspace, "foo", + geteuid(), start, len, 0) == 0) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED) < 0) goto cleanup; @@ -237,6 +247,8 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; + const off_t start = 0; + const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -250,6 +262,7 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED | VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) goto cleanup; @@ -258,6 +271,7 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) == 0) goto cleanup; @@ -265,6 +279,7 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) goto cleanup; if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), + start, len, VIR_LOCK_SPACE_ACQUIRE_SHARED | VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0) goto cleanup; @@ -297,6 +312,8 @@ static int testLockSpaceResourceLockPath(const void *args ATTRIBUTE_UNUSED) { virLockSpacePtr lockspace; int ret = -1; + const off_t start = 0; + const off_t len = 1; rmdir(LOCKSPACE_DIR); @@ -309,13 +326,15 @@ static int testLockSpaceResourceLockPath(const void *args ATTRIBUTE_UNUSED) if (virLockSpaceCreateResource(lockspace, LOCKSPACE_DIR "/foo") < 0) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", geteuid(), 0) < 0) + if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", + geteuid(), start, len, 0) < 0) goto cleanup; if (!virFileExists(LOCKSPACE_DIR "/foo")) goto cleanup; - if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", geteuid(), 0) == 0) + if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", + geteuid(), start, len, 0) == 0) goto cleanup; if (virLockSpaceDeleteResource(lockspace, LOCKSPACE_DIR "/foo") == 0) -- 2.16.4

We will want virtlockd to lock files on behalf of libvirtd and not qemu process, because it is libvirtd that needs an exclusive access not qemu. This requires new lock context. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 + src/locking/lock_driver_lockd.c | 291 ++++++++++++++++++++++++-------------- src/locking/lock_driver_sanlock.c | 37 +++-- 3 files changed, 214 insertions(+), 116 deletions(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 8b7cccc521..a9d2041c30 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 a daemon (e.g. libvirtd) */ + VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON = 1, } virLockManagerObjectType; typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index ca825e6026..8580d12340 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -56,10 +56,21 @@ struct _virLockManagerLockDaemonResource { }; struct _virLockManagerLockDaemonPrivate { - unsigned char uuid[VIR_UUID_BUFLEN]; - char *name; - int id; - pid_t pid; + virLockManagerObjectType type; + union { + struct { + unsigned char uuid[VIR_UUID_BUFLEN]; + char *name; + int id; + pid_t pid; + } dom; + + struct { + unsigned char uuid[VIR_UUID_BUFLEN]; + char *name; + pid_t pid; + } daemon; + } t; size_t nresources; virLockManagerLockDaemonResourcePtr resources; @@ -156,10 +167,24 @@ virLockManagerLockDaemonConnectionRegister(virLockManagerPtr lock, memset(&args, 0, sizeof(args)); args.flags = 0; - memcpy(args.owner.uuid, priv->uuid, VIR_UUID_BUFLEN); - args.owner.name = priv->name; - args.owner.id = priv->id; - args.owner.pid = priv->pid; + + switch (priv->type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: + memcpy(args.owner.uuid, priv->t.dom.uuid, VIR_UUID_BUFLEN); + args.owner.name = priv->t.dom.name; + args.owner.id = priv->t.dom.id; + args.owner.pid = priv->t.dom.pid; + break; + + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + memcpy(args.owner.uuid, priv->t.daemon.uuid, VIR_UUID_BUFLEN); + args.owner.name = priv->t.daemon.name; + args.owner.pid = priv->t.daemon.pid; + break; + + default: + return -1; + } if (virNetClientProgramCall(program, client, @@ -391,7 +416,18 @@ virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv) } VIR_FREE(priv->resources); - VIR_FREE(priv->name); + switch (priv->type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: + VIR_FREE(priv->t.dom.name); + break; + + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + VIR_FREE(priv->t.daemon.name); + break; + + default: + break; + } VIR_FREE(priv); } @@ -420,46 +456,82 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, if (VIR_ALLOC(priv) < 0) return -1; - switch (type) { + priv->type = type; + + switch ((virLockManagerObjectType) type) { case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: for (i = 0; i < nparams; i++) { if (STREQ(params[i].key, "uuid")) { - memcpy(priv->uuid, params[i].value.uuid, VIR_UUID_BUFLEN); + memcpy(priv->t.dom.uuid, params[i].value.uuid, VIR_UUID_BUFLEN); } else if (STREQ(params[i].key, "name")) { - if (VIR_STRDUP(priv->name, params[i].value.str) < 0) + if (VIR_STRDUP(priv->t.dom.name, params[i].value.str) < 0) goto cleanup; } else if (STREQ(params[i].key, "id")) { - priv->id = params[i].value.iv; + priv->t.dom.id = params[i].value.iv; } else if (STREQ(params[i].key, "pid")) { - priv->pid = params[i].value.iv; + priv->t.dom.pid = params[i].value.iv; } else if (STREQ(params[i].key, "uri")) { /* ignored */ } else { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected parameter %s for object"), + _("Unexpected parameter %s for domain object"), params[i].key); goto cleanup; } } - if (priv->id == 0) { + if (priv->t.dom.id == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing ID parameter for domain object")); goto cleanup; } - if (priv->pid == 0) + if (priv->t.dom.pid == 0) VIR_DEBUG("Missing PID parameter for domain object"); - if (!priv->name) { + if (!priv->t.dom.name) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing name parameter for domain object")); goto cleanup; } - if (!virUUIDIsValid(priv->uuid)) { + if (!virUUIDIsValid(priv->t.dom.uuid)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing UUID parameter for domain object")); goto cleanup; } break; + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + for (i = 0; i < nparams; i++) { + if (STREQ(params[i].key, "uuid")) { + memcpy(priv->t.daemon.uuid, params[i].value.uuid, VIR_UUID_BUFLEN); + } else if (STREQ(params[i].key, "name")) { + if (VIR_STRDUP(priv->t.daemon.name, params[i].value.str) < 0) + goto cleanup; + } else if (STREQ(params[i].key, "pid")) { + priv->t.daemon.pid = params[i].value.iv; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected parameter %s for daemon object"), + params[i].key); + goto cleanup; + } + } + + if (!virUUIDIsValid(priv->t.daemon.uuid)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing UUID parameter for daemon object")); + goto cleanup; + } + if (!priv->t.daemon.name) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing name parameter for daemon object")); + goto cleanup; + } + if (priv->t.daemon.pid == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing PID parameter for daemon object")); + goto cleanup; + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), @@ -494,107 +566,119 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0; - switch (type) { - case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: - if (params || nparams) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unexpected parameters for disk resource")); - goto cleanup; - } - if (!driver->autoDiskLease) { - if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | - VIR_LOCK_MANAGER_RESOURCE_READONLY))) - priv->hasRWDisks = true; - return 0; - } + switch (priv->type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: - /* XXX we should somehow pass in TYPE=BLOCK info - * from the domain_lock code, instead of assuming /dev - */ - if (STRPREFIX(name, "/dev") && - driver->lvmLockSpaceDir) { - VIR_DEBUG("Trying to find an LVM UUID for %s", name); - if (virStorageFileGetLVMKey(name, &newName) < 0) + switch (type) { + case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: + if (params || nparams) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected parameters for disk resource")); goto cleanup; + } + if (!driver->autoDiskLease) { + if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_READONLY))) + priv->hasRWDisks = true; + return 0; + } - if (newName) { - VIR_DEBUG("Got an LVM UUID %s for %s", newName, name); - if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0) + /* XXX we should somehow pass in TYPE=BLOCK info + * from the domain_lock code, instead of assuming /dev + */ + if (STRPREFIX(name, "/dev") && + driver->lvmLockSpaceDir) { + VIR_DEBUG("Trying to find an LVM UUID for %s", name); + if (virStorageFileGetLVMKey(name, &newName) < 0) goto cleanup; - autoCreate = true; - break; + + if (newName) { + VIR_DEBUG("Got an LVM UUID %s for %s", newName, name); + if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0) + goto cleanup; + autoCreate = true; + break; + } + virResetLastError(); + /* Fallback to generic non-block code */ } - virResetLastError(); - /* Fallback to generic non-block code */ - } - if (STRPREFIX(name, "/dev") && - driver->scsiLockSpaceDir) { - VIR_DEBUG("Trying to find an SCSI ID for %s", name); - if (virStorageFileGetSCSIKey(name, &newName) < 0) - goto cleanup; + if (STRPREFIX(name, "/dev") && + driver->scsiLockSpaceDir) { + VIR_DEBUG("Trying to find an SCSI ID for %s", name); + if (virStorageFileGetSCSIKey(name, &newName) < 0) + goto cleanup; + + if (newName) { + VIR_DEBUG("Got an SCSI ID %s for %s", newName, name); + if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0) + goto cleanup; + autoCreate = true; + break; + } + virResetLastError(); + /* Fallback to generic non-block code */ + } - if (newName) { - VIR_DEBUG("Got an SCSI ID %s for %s", newName, name); - if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0) + if (driver->fileLockSpaceDir) { + if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0) + goto cleanup; + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) goto cleanup; autoCreate = true; - break; + VIR_DEBUG("Using indirect lease %s for %s", newName, name); + } else { + if (VIR_STRDUP(newLockspace, "") < 0) + goto cleanup; + if (VIR_STRDUP(newName, name) < 0) + goto cleanup; + VIR_DEBUG("Using direct lease for %s", name); } - virResetLastError(); - /* Fallback to generic non-block code */ - } - if (driver->fileLockSpaceDir) { - if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0) - goto cleanup; - if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) + break; + case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: { + size_t i; + char *path = NULL; + char *lockspace = NULL; + for (i = 0; i < nparams; i++) { + if (STREQ(params[i].key, "offset")) { + if (params[i].value.ul != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Offset must be zero for this lock manager")); + goto cleanup; + } + } else if (STREQ(params[i].key, "lockspace")) { + lockspace = params[i].value.str; + } else if (STREQ(params[i].key, "path")) { + path = params[i].value.str; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected parameter %s for lease resource"), + params[i].key); + goto cleanup; + } + } + if (!path || !lockspace) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing path or lockspace for lease resource")); goto cleanup; - autoCreate = true; - VIR_DEBUG("Using indirect lease %s for %s", newName, name); - } else { - if (VIR_STRDUP(newLockspace, "") < 0) + } + if (virAsprintf(&newLockspace, "%s/%s", + path, lockspace) < 0) goto cleanup; if (VIR_STRDUP(newName, name) < 0) goto cleanup; - VIR_DEBUG("Using direct lease for %s", name); - } + } break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown lock manager object type %d for domain lock object"), + type); + goto cleanup; + } break; - case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: { - size_t i; - char *path = NULL; - char *lockspace = NULL; - for (i = 0; i < nparams; i++) { - if (STREQ(params[i].key, "offset")) { - if (params[i].value.ul != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Offset must be zero for this lock manager")); - goto cleanup; - } - } else if (STREQ(params[i].key, "lockspace")) { - lockspace = params[i].value.str; - } else if (STREQ(params[i].key, "path")) { - path = params[i].value.str; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected parameter %s for lease resource"), - params[i].key); - goto cleanup; - } - } - if (!path || !lockspace) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing path or lockspace for lease resource")); - goto cleanup; - } - if (virAsprintf(&newLockspace, "%s/%s", - path, lockspace) < 0) - goto cleanup; - if (VIR_STRDUP(newName, name) < 0) - goto cleanup; - } break; + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), @@ -639,7 +723,8 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY | VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1); - if (priv->nresources == 0 && + if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN && + priv->nresources == 0 && priv->hasRWDisks && driver->requireLeaseForDisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 39c2f94a76..fe422d3be6 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -513,21 +513,32 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, priv->flags = flags; - for (i = 0; i < nparams; i++) { - param = ¶ms[i]; + switch ((virLockManagerObjectType) type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: + for (i = 0; i < nparams; i++) { + param = ¶ms[i]; - if (STREQ(param->key, "uuid")) { - memcpy(priv->vm_uuid, param->value.uuid, 16); - } else if (STREQ(param->key, "name")) { - if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) - goto error; - } else if (STREQ(param->key, "pid")) { - priv->vm_pid = param->value.iv; - } else if (STREQ(param->key, "id")) { - priv->vm_id = param->value.ui; - } else if (STREQ(param->key, "uri")) { - priv->vm_uri = param->value.cstr; + if (STREQ(param->key, "uuid")) { + memcpy(priv->vm_uuid, param->value.uuid, 16); + } else if (STREQ(param->key, "name")) { + if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) + goto error; + } else if (STREQ(param->key, "pid")) { + priv->vm_pid = param->value.iv; + } else if (STREQ(param->key, "id")) { + priv->vm_id = param->value.ui; + } else if (STREQ(param->key, "uri")) { + priv->vm_uri = param->value.cstr; + } } + break; + + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown lock manager object type %d"), + type); + goto error; } /* Sanlock needs process registration, but the only way how to probe -- 2.16.4

Soon there will be a virtlockd client that wants to either lock all the resources or none (in order to avoid virtlockd killing the client on connection close). Because on the RPC layer we can only acquire one resource at a time, we have to perform a rollback once we hit a resource that can't be acquired. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 4 ++ src/locking/lock_driver_lockd.c | 84 +++++++++++++++++++++++++++++------------ 2 files changed, 64 insertions(+), 24 deletions(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 9be0abcfba..8d236471d4 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -67,6 +67,10 @@ typedef enum { VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY = (1 << 0), /* Prevent further lock/unlock calls from this process */ VIR_LOCK_MANAGER_ACQUIRE_RESTRICT = (1 << 1), + /* In case when acquiring more resources which one of them + * can't be acquired, perform a rollback and release all + * resources acquired so far. */ + VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK = (1 << 2), } virLockManagerAcquireFlags; typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index cb294ac694..3068a72507 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -729,6 +729,34 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, } +static int virLockManagerLockDaemonReleaseImpl(virNetClientPtr client, + virNetClientProgramPtr program, + int *counter, + virLockManagerLockDaemonResourcePtr res) +{ + virLockSpaceProtocolReleaseResourceArgs args; + + memset(&args, 0, sizeof(args)); + + args.path = res->lockspace; + args.name = res->name; + args.flags = res->flags; + + args.flags &= + ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA); + + return virNetClientProgramCall(program, + client, + (*counter)++, + VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE, + 0, NULL, NULL, NULL, + (xdrproc_t)xdr_virLockSpaceProtocolReleaseResourceArgs, &args, + (xdrproc_t)xdr_void, NULL); +} + + static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, const char *state ATTRIBUTE_UNUSED, unsigned int flags, @@ -739,10 +767,13 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, virNetClientProgramPtr program = NULL; int counter = 0; int rv = -1; + ssize_t i; + ssize_t lastGood = -1; virLockManagerLockDaemonPrivatePtr priv = lock->privateData; virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY | - VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1); + VIR_LOCK_MANAGER_ACQUIRE_RESTRICT | + VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK, -1); if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN && priv->nresources == 0 && @@ -761,7 +792,6 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, goto cleanup; if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) { - size_t i; for (i = 0; i < priv->nresources; i++) { virLockSpaceProtocolAcquireResourceArgs args; @@ -779,6 +809,7 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, (xdrproc_t)xdr_virLockSpaceProtocolAcquireResourceArgs, &args, (xdrproc_t)xdr_void, NULL) < 0) goto cleanup; + lastGood = i; } } @@ -789,8 +820,30 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, rv = 0; cleanup: - if (rv != 0 && fd) - VIR_FORCE_CLOSE(*fd); + if (rv < 0) { + int saved_errno = errno; + virErrorPtr origerr; + + virErrorPreserveLast(&origerr); + if (fd) + VIR_FORCE_CLOSE(*fd); + + if (client && program && + flags & VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK && + !(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) { + for (i = lastGood; i >= 0; i--) { + virLockManagerLockDaemonResourcePtr res = &priv->resources[i]; + + if (virLockManagerLockDaemonReleaseImpl(client, program, + &counter, res) < 0) + VIR_WARN("Unable to release resource lockspace=%s name=%s", + res->lockspace, res->name); + } + } + + virErrorRestore(&origerr); + errno = saved_errno; + } virNetClientClose(client); virObjectUnref(client); virObjectUnref(program); @@ -818,27 +871,10 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, goto cleanup; for (i = 0; i < priv->nresources; i++) { - virLockSpaceProtocolReleaseResourceArgs args; + virLockManagerLockDaemonResourcePtr res = &priv->resources[i]; - memset(&args, 0, sizeof(args)); - - if (priv->resources[i].lockspace) - args.path = priv->resources[i].lockspace; - args.name = priv->resources[i].name; - args.flags = priv->resources[i].flags; - - args.flags &= - ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA); - - if (virNetClientProgramCall(program, - client, - counter++, - VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE, - 0, NULL, NULL, NULL, - (xdrproc_t)xdr_virLockSpaceProtocolReleaseResourceArgs, &args, - (xdrproc_t)xdr_void, NULL) < 0) + if (virLockManagerLockDaemonReleaseImpl(client, program, + &counter, res) < 0) goto cleanup; } -- 2.16.4

In some cases we might want to not load the lock driver config. Alter virLockManagerPluginNew() and the lock drivers to cope with this fact. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/locking/lock_driver.h | 4 ++++ src/locking/lock_driver_lockd.c | 4 +++- src/locking/lock_driver_sanlock.c | 4 +++- src/locking/lock_manager.c | 10 +++++++--- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 8d236471d4..f938b1df2a 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -124,6 +124,7 @@ struct _virLockManagerParam { /** * virLockDriverInit: * @version: the libvirt requested plugin ABI version + * @configFile: path to config file * @flags: the libvirt requested plugin optional extras * * Allow the plugin to validate the libvirt requested @@ -131,6 +132,9 @@ struct _virLockManagerParam { * to block its use in versions of libvirtd which are * too old to support key features. * + * The @configFile variable points to config file that the driver + * should load. If NULL, no config file should be loaded. + * * NB: A plugin may be loaded multiple times, for different * libvirt drivers (eg QEMU, LXC, UML) * diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 3068a72507..7566c4abe1 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -365,8 +365,10 @@ static int virLockManagerLockDaemonInit(unsigned int version, driver->requireLeaseForDisks = true; driver->autoDiskLease = true; - if (virLockManagerLockDaemonLoadConfig(configFile) < 0) + if (configFile && + virLockManagerLockDaemonLoadConfig(configFile) < 0) { goto error; + } if (driver->autoDiskLease) { if (driver->fileLockSpaceDir && diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 9393e7d9a2..66953c70d5 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -450,8 +450,10 @@ static int virLockManagerSanlockInit(unsigned int version, goto error; } - if (virLockManagerSanlockLoadConfig(driver, configFile) < 0) + if (configFile && + virLockManagerSanlockLoadConfig(driver, configFile) < 0) { goto error; + } if (driver->autoDiskLease && !driver->hostID) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index 4ef9f9e692..84d0c30d37 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -105,6 +105,8 @@ static void virLockManagerLogParams(size_t nparams, /** * virLockManagerPluginNew: * @name: the name of the plugin + * @driverName: the hypervisor driver that loads the plugin + * @configDir: path to dir where config files are stored * @flag: optional plugin flags * * Attempt to load the plugin $(libdir)/libvirt/lock-driver/@name.so @@ -130,11 +132,13 @@ virLockManagerPluginPtr virLockManagerPluginNew(const char *name, char *configFile = NULL; VIR_DEBUG("name=%s driverName=%s configDir=%s flags=0x%x", - name, driverName, configDir, flags); + name, NULLSTR(driverName), NULLSTR(configDir), flags); - if (virAsprintf(&configFile, "%s/%s-%s.conf", - configDir, driverName, name) < 0) + if (driverName && configDir && + virAsprintf(&configFile, "%s/%s-%s.conf", + configDir, driverName, name) < 0) { return NULL; + } if (STREQ(name, "nop")) { driver = &virLockDriverNop; -- 2.16.4

Now that we know what metadata lock manager user wishes to use we can load it when initializing security driver. This is achieved by adding new argument to virSecurityManagerNewDriver() and subsequently to all functions that end up calling it. The cfg.mk change is needed in order to allow lock_manager.h inclusion in security driver without 'syntax-check' complaining. This is safe thing to do as locking APIs will always exist (it's only backend implementation that changes). However, instead of allowing the include for all other drivers (like cpu, network, and so on) allow it only for security driver. This will still trigger the error if including from other drivers. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 4 +++- src/lxc/lxc_controller.c | 3 ++- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 3 +++ src/security/security_manager.c | 22 ++++++++++++++++++++++ src/security/security_manager.h | 2 ++ tests/seclabeltest.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- tests/securityselinuxtest.c | 2 +- tests/testutilsqemu.c | 2 +- 10 files changed, 37 insertions(+), 7 deletions(-) diff --git a/cfg.mk b/cfg.mk index 609ae869c2..e0a7b5105a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -787,8 +787,10 @@ sc_prohibit_cross_inclusion: case $$dir in \ util/) safe="util";; \ access/ | conf/) safe="($$dir|conf|util)";; \ - cpu/| network/| node_device/| rpc/| security/| storage/) \ + cpu/| network/| node_device/| rpc/| storage/) \ safe="($$dir|util|conf|storage)";; \ + security/) \ + safe="($$dir|util|conf|storage|locking)";; \ xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";; \ *) safe="($$dir|$(mid_dirs)|util)";; \ esac; \ diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 4e84391bf5..5f957eb1f8 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -2625,7 +2625,8 @@ int main(int argc, char *argv[]) ctrl->handshakeFd = handshakeFd; if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver, - LXC_DRIVER_NAME, 0))) + LXC_DRIVER_NAME, + "nop", 0))) goto cleanup; if (ctrl->def->seclabels) { diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 8867645cdc..93aa25e9e6 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1532,7 +1532,7 @@ lxcSecurityInit(virLXCDriverConfigPtr cfg) flags |= VIR_SECURITY_MANAGER_REQUIRE_CONFINED; virSecurityManagerPtr mgr = virSecurityManagerNew(cfg->securityDriverName, - LXC_DRIVER_NAME, flags); + LXC_DRIVER_NAME, "nop", flags); if (!mgr) goto error; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6763c8cddc..4ac0d86803 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -355,6 +355,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) while (names && *names) { if (!(mgr = qemuSecurityNew(*names, QEMU_DRIVER_NAME, + cfg->metadataLockManagerName, flags))) goto error; if (!stack) { @@ -370,6 +371,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) } else { if (!(mgr = qemuSecurityNew(NULL, QEMU_DRIVER_NAME, + cfg->metadataLockManagerName, flags))) goto error; if (!(stack = qemuSecurityNewStack(mgr))) @@ -386,6 +388,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) cfg->user, cfg->group, flags, + cfg->metadataLockManagerName, qemuSecurityChownCallback))) goto error; if (!stack) { diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 9f770d8c53..5c8370c159 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -28,6 +28,7 @@ #include "viralloc.h" #include "virobject.h" #include "virlog.h" +#include "locking/lock_manager.h" #define VIR_FROM_THIS VIR_FROM_SECURITY @@ -40,6 +41,8 @@ struct _virSecurityManager { unsigned int flags; const char *virtDriver; void *privateData; + + virLockManagerPluginPtr lockPlugin; }; static virClassPtr virSecurityManagerClass; @@ -52,6 +55,9 @@ void virSecurityManagerDispose(void *obj) if (mgr->drv->close) mgr->drv->close(mgr); + + virObjectUnref(mgr->lockPlugin); + VIR_FREE(mgr->privateData); } @@ -71,6 +77,7 @@ VIR_ONCE_GLOBAL_INIT(virSecurityManager); static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr drv, const char *virtDriver, + const char *lockManagerPluginName, unsigned int flags) { virSecurityManagerPtr mgr = NULL; @@ -90,6 +97,14 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv, if (!(mgr = virObjectLockableNew(virSecurityManagerClass))) goto error; + if (!lockManagerPluginName) + lockManagerPluginName = "nop"; + + if (!(mgr->lockPlugin = virLockManagerPluginNew(lockManagerPluginName, + NULL, NULL, 0))) { + goto error; + } + mgr->drv = drv; mgr->flags = flags; mgr->virtDriver = virtDriver; @@ -112,6 +127,7 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary) virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverStack, virSecurityManagerGetDriver(primary), + NULL, primary->flags); if (!mgr) @@ -120,6 +136,8 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary) if (virSecurityStackAddNested(mgr, primary) < 0) goto error; + mgr->lockPlugin = virObjectRef(mgr->lockPlugin); + return mgr; error: virObjectUnref(mgr); @@ -142,6 +160,7 @@ virSecurityManagerNewDAC(const char *virtDriver, uid_t user, gid_t group, unsigned int flags, + const char *lockManagerPluginName, virSecurityManagerDACChownCallback chownCallback) { virSecurityManagerPtr mgr; @@ -152,6 +171,7 @@ virSecurityManagerNewDAC(const char *virtDriver, mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, virtDriver, + lockManagerPluginName, flags & VIR_SECURITY_MANAGER_NEW_MASK); if (!mgr) @@ -173,6 +193,7 @@ virSecurityManagerNewDAC(const char *virtDriver, virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, + const char *lockManagerPluginName, unsigned int flags) { virSecurityDriverPtr drv = virSecurityDriverLookup(name, virtDriver); @@ -201,6 +222,7 @@ virSecurityManagerNew(const char *name, return virSecurityManagerNewDriver(drv, virtDriver, + lockManagerPluginName, flags); } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 1ead369e82..c537e1c994 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -45,6 +45,7 @@ typedef enum { virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, + const char *lockManagerPluginName, unsigned int flags); virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary); @@ -70,6 +71,7 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, uid_t user, gid_t group, unsigned int flags, + const char *lockManagerPluginName, virSecurityManagerDACChownCallback chownCallback); int virSecurityManagerPreFork(virSecurityManagerPtr mgr); diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index 4cda80cec2..6aafc45e64 100644 --- a/tests/seclabeltest.c +++ b/tests/seclabeltest.c @@ -18,7 +18,7 @@ mymain(void) if (virThreadInitialize() < 0) return EXIT_FAILURE; - mgr = virSecurityManagerNew(NULL, "QEMU", VIR_SECURITY_MANAGER_DEFAULT_CONFINED); + mgr = virSecurityManagerNew(NULL, "QEMU", "nop", VIR_SECURITY_MANAGER_DEFAULT_CONFINED); if (mgr == NULL) { fprintf(stderr, "Failed to start security driver"); return EXIT_FAILURE; diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 48fee7cd28..85797411eb 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -349,7 +349,7 @@ mymain(void) if (!rc) return EXIT_AM_SKIP; - if (!(mgr = virSecurityManagerNew("selinux", "QEMU", + if (!(mgr = virSecurityManagerNew("selinux", "QEMU", "nop", VIR_SECURITY_MANAGER_DEFAULT_CONFINED | VIR_SECURITY_MANAGER_PRIVILEGED))) { VIR_TEST_VERBOSE("Unable to initialize security driver: %s\n", diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c index a785e9a7da..652981c895 100644 --- a/tests/securityselinuxtest.c +++ b/tests/securityselinuxtest.c @@ -275,7 +275,7 @@ mymain(void) int ret = 0; virSecurityManagerPtr mgr; - if (!(mgr = virSecurityManagerNew("selinux", "QEMU", + if (!(mgr = virSecurityManagerNew("selinux", "QEMU", "nop", VIR_SECURITY_MANAGER_DEFAULT_CONFINED | VIR_SECURITY_MANAGER_PRIVILEGED))) { fprintf(stderr, "Unable to initialize security driver: %s\n", diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 70bed461b5..f9972f7adc 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -717,7 +717,7 @@ int qemuTestDriverInit(virQEMUDriver *driver) if (qemuTestCapsCacheInsert(driver->qemuCapsCache, NULL) < 0) goto error; - if (!(mgr = virSecurityManagerNew("none", "qemu", + if (!(mgr = virSecurityManagerNew("none", "qemu", "nop", VIR_SECURITY_MANAGER_PRIVILEGED))) goto error; if (!(driver->securityManager = virSecurityManagerNewStack(mgr))) -- 2.16.4

So far the whole transaction handling is done virSecurityDACSetOwnershipInternal(). This needs to change for the sake of security label remembering and locking. Otherwise we would be locking a path when only appending it to transaction list and not when actually relabelling it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 65 ++++++++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 926c9a33c1..52e28b5fda 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -77,12 +77,13 @@ struct _virSecurityDACChownItem { const virStorageSource *src; uid_t uid; gid_t gid; + bool restore; }; typedef struct _virSecurityDACChownList virSecurityDACChownList; typedef virSecurityDACChownList *virSecurityDACChownListPtr; struct _virSecurityDACChownList { - virSecurityDACDataPtr priv; + virSecurityManagerPtr manager; virSecurityDACChownItemPtr *items; size_t nItems; }; @@ -95,7 +96,8 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list, const char *path, const virStorageSource *src, uid_t uid, - gid_t gid) + gid_t gid, + bool restore) { int ret = -1; char *tmp = NULL; @@ -111,6 +113,7 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list, item->src = src; item->uid = uid; item->gid = gid; + item->restore = restore; if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) goto cleanup; @@ -159,25 +162,29 @@ static int virSecurityDACTransactionAppend(const char *path, const virStorageSource *src, uid_t uid, - gid_t gid) + gid_t gid, + bool restore) { virSecurityDACChownListPtr list = virThreadLocalGet(&chownList); if (!list) return 0; - if (virSecurityDACChownListAppend(list, path, src, uid, gid) < 0) + if (virSecurityDACChownListAppend(list, path, src, uid, gid, restore) < 0) return -1; return 1; } -static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, - const virStorageSource *src, - const char *path, - uid_t uid, - gid_t gid); +static int virSecurityDACSetOwnership(virSecurityManagerPtr mgr, + const virStorageSource *src, + const char *path, + uid_t uid, + gid_t gid); +static int virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, + const virStorageSource *src, + const char *path); /** * virSecurityDACTransactionRun: * @pid: process pid @@ -201,11 +208,16 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, virSecurityDACChownItemPtr item = list->items[i]; /* TODO Implement rollback */ - if (virSecurityDACSetOwnershipInternal(list->priv, - item->src, - item->path, - item->uid, - item->gid) < 0) + if ((!item->restore && + virSecurityDACSetOwnership(list->manager, + item->src, + item->path, + item->uid, + item->gid) < 0) || + (item->restore && + virSecurityDACRestoreFileLabelInternal(list->manager, + item->src, + item->path) < 0)) return -1; } @@ -455,7 +467,6 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr) static int virSecurityDACTransactionStart(virSecurityManagerPtr mgr) { - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityDACChownListPtr list; list = virThreadLocalGet(&chownList); @@ -468,7 +479,7 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr) if (VIR_ALLOC(list) < 0) return -1; - list->priv = priv; + list->manager = mgr; if (virThreadLocalSet(&chownList, list) < 0) { virReportSystemError(errno, "%s", @@ -564,11 +575,6 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, /* Be aware that this function might run in a separate process. * Therefore, any driver state changes would be thrown away. */ - if ((rc = virSecurityDACTransactionAppend(path, src, uid, gid)) < 0) - return -1; - else if (rc > 0) - return 0; - VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long)uid, (long)gid); @@ -640,11 +646,20 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); struct stat sb; + int rc; if (!path && src && src->path && virStorageSourceIsLocalStorage(src)) path = src->path; + /* Be aware that this function might run in a separate process. + * Therefore, any driver state changes would be thrown away. */ + + if ((rc = virSecurityDACTransactionAppend(path, src, uid, gid, false)) < 0) + return -1; + else if (rc > 0) + return 0; + if (path) { if (stat(path, &sb) < 0) { virReportSystemError(errno, _("unable to stat: %s"), path); @@ -676,6 +691,14 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, virStorageSourceIsLocalStorage(src)) path = src->path; + /* Be aware that this function might run in a separate process. + * Therefore, any driver state changes would be thrown away. */ + + if ((rv = virSecurityDACTransactionAppend(path, src, uid, gid, true)) < 0) + return -1; + else if (rv > 0) + return 0; + if (path) { rv = virSecurityDACRecallLabel(priv, path, &uid, &gid); if (rv < 0) -- 2.16.4

Lock all the paths we want to relabel to mutually exclude other libvirt daemons. The only culprit here hitch here is that directories can't be locked. Therefore, when relabeling a directory do not lock it (this happens only when setting up some domain private paths anyway, e.g. huge pages directory). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 43 +++++++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f6416010f9..056637e4cb 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -90,7 +90,7 @@ struct _virSecuritySELinuxContextItem { typedef struct _virSecuritySELinuxContextList virSecuritySELinuxContextList; typedef virSecuritySELinuxContextList *virSecuritySELinuxContextListPtr; struct _virSecuritySELinuxContextList { - bool privileged; + virSecurityManagerPtr manager; virSecuritySELinuxContextItemPtr *items; size_t nItems; }; @@ -212,8 +212,29 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, void *opaque) { virSecuritySELinuxContextListPtr list = opaque; + bool privileged = virSecurityManagerGetPrivileged(list->manager); + const char **paths = NULL; + size_t npaths = 0; size_t i; + int rv; + int ret = -1; + if (VIR_ALLOC_N(paths, list->nItems) < 0) + return -1; + + 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) + goto cleanup; + + rv = 0; for (i = 0; i < list->nItems; i++) { virSecuritySELinuxContextItemPtr item = list->items[i]; @@ -221,11 +242,22 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, if (virSecuritySELinuxSetFileconHelper(item->path, item->tcon, item->optional, - list->privileged) < 0) - return -1; + privileged) < 0) { + rv = -1; + break; + } } - return 0; + if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) + goto cleanup; + + if (rv < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(paths); + return ret; } @@ -1010,7 +1042,6 @@ virSecuritySELinuxGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) static int virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr) { - bool privileged = virSecurityManagerGetPrivileged(mgr); virSecuritySELinuxContextListPtr list; list = virThreadLocalGet(&contextList); @@ -1023,7 +1054,7 @@ virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr) if (VIR_ALLOC(list) < 0) return -1; - list->privileged = privileged; + list->manager = mgr; if (virThreadLocalSet(&contextList, list) < 0) { virReportSystemError(errno, "%s", -- 2.16.4