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(a)redhat.com&gt; --- 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(a)redhat.com&gt; --- 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(a)redhat.com&gt; --- 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(a)redhat.com&gt; --- 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(a)redhat.com&gt; --- 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(a)redhat.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- 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(a)redhat.com&gt; --- 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 = &params[i]; + switch ((virLockManagerObjectType) type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: + for (i = 0; i < nparams; i++) { + param = &params[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

On 09/17/2018 06:17 PM, John Ferlan wrote: The 'default' label exists merely to shut gcc up. However, I'll copy the error message from virLockManagerLockDaemonNew(). Michal

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(a)redhat.com&gt; --- 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

On 09/10/2018 05:36 AM, Michal Privoznik wrote: It would if daemon used @pid for ID ;-) Looking through a bit of history and came across an oh shinola moment, see: commit id ee3eddb332 You may need to implement what I suggest above and move on from there. John (shinola is a reference to a movie called "The Jerk" and the main character not knowing sh!t from shinola)

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(a)redhat.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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/(a)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(a)redhat.com&gt; --- 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

On 09/17/2018 09:56 PM, John Ferlan wrote: Because that would require virtlockd running every time the test is run. Well, not right now but after the last patch in the series. Maybe. But I'll save that for a follow up patch. Michal

On 09/10/2018 05:36 AM, Michal Privoznik wrote: s/on/one I smell a configurable timeout instead of 60 seconds, but that's mainly because my senses are more acutely aware of such configurable timeout changes given some recent on list patches for client lock timeouts when (as I assume) there is a client gathering stats that takes a long time. Would you mind renaming to "clientfd" or at least "noting" in comments here where this fd is duplicated so that future me doesn't need to go back and read the commit where this was added to refresh my memory. Seeing just fd and searching on that is like finding a needle in the haystack of fd's. [...] How much wood could a woodchuck chuck wood if a woodchuck could chuck wood? I know you know already by rv = -1 Just a couple of minor things, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

[...] VIR_FROM_THIS VIR_FROM_SECURITY After seeing it in use in patch 19 and thinking about it for a very short period of time, would it make more sense to store @lock somewhere so that virSecurityManagerMetadataUnlock doesn't fail because the virSecurityManagerNewLockManager fails? The @mgr is mutex locked so that nothing can change @mgr while this Meta Lock/Unlock is occurring. It'd be a shame to not call virLockManagerRelease just because we didn't save @lock John

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(a)redhat.com&gt; --- 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

On 09/17/2018 11:47 PM, John Ferlan wrote: I hear you, but I don't think that ref/unref is necessary here. It's actually @mgr who called these driver APIs. Michal

On 09/10/2018 05:36 AM, Michal Privoznik wrote: reread the above and fix and fix ;-) The description for this function needs some updating to describe this extra step and what/how/why it's done this way. Yeah, I know go back to the commit message and find it... If you'd used the non || construct, I think this would look cleaner: if (!item->restore) rv = virSecurityDACSetOwnership else rv = virSecurityDACRestoreFileLabelInternal if (rv < 0) break; So much easier to read. See my second note in patch 16 - Perhaps it's better to not repeat the same sequence since paths/npaths doesn't change. In any case, for this code... With a couple of adjustments, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

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(a)redhat.com&gt; --- 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

Michal Privoznik <mprivozn(a)redhat.com&gt; [2018-09-12, 11:32AM +0200]: On the same host. (gdb) thread apply all bt Thread 17 (Thread 0x3ff48dff910 (LWP 193353)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff6678c5a8 in udevEventHandleThread (opaque=<optimized out>) at node_device/node_device_udev.c:1603 #3 0x000003ff8c6bad16 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 16 (Thread 0x3ff4acfe910 (LWP 193312)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 15 (Thread 0x3ff4b4ff910 (LWP 193311)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 14 (Thread 0x3ff64efd910 (LWP 193310)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 13 (Thread 0x3ff656fe910 (LWP 193309)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 12 (Thread 0x3ff65eff910 (LWP 193308)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 11 (Thread 0x3ff67fff910 (LWP 193307)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbba0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 10 (Thread 0x3ff84bf7910 (LWP 193306)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbba0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 9 (Thread 0x3ff853f8910 (LWP 193305)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbba0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 8 (Thread 0x3ff85bf9910 (LWP 193304)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbba0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 7 (Thread 0x3ff863fa910 (LWP 193303)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbba0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 6 (Thread 0x3ff86bfb910 (LWP 193302)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 5 (Thread 0x3ff873fc910 (LWP 193301)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 4 (Thread 0x3ff87bfd910 (LWP 193300)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 3 (Thread 0x3ff883fe910 (LWP 193299)): #0 0x000003ff8b7113d4 in read () from /lib64/libpthread.so.0 #1 0x000003ff8c65a648 in saferead () from /lib64/libvirt.so.0 #2 0x000003ff8c65a718 in saferead_lim () from /lib64/libvirt.so.0 #3 0x000003ff8c65abe4 in virFileReadHeaderFD () from /lib64/libvirt.so.0 #4 0x000003ff8c69cb5a in virProcessRunInMountNamespace () from /lib64/libvirt.so.0 #5 0x000003ff8c767bd6 in virSecuritySELinuxTransactionCommit () from /lib64/libvirt.so.0 #6 0x000003ff8c75fb1c in virSecurityManagerTransactionCommit () from /lib64/libvirt.so.0 #7 0x000003ff8c75bb70 in virSecurityStackTransactionCommit () from /lib64/libvirt.so.0 #8 0x000003ff8c75fb1c in virSecurityManagerTransactionCommit () from /lib64/libvirt.so.0 #9 0x000003ff6612c492 in qemuSecuritySetAllLabel () from /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so #10 0x000003ff660bcfd6 in qemuProcessLaunch () from /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so #11 0x000003ff660c0916 in qemuProcessStart () from /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so #12 0x000003ff66124a00 in qemuDomainObjStart.constprop.52 () from /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so #13 0x000003ff66125030 in qemuDomainCreateWithFlags () from /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so #14 0x000003ff8c87bfca in virDomainCreate () from /lib64/libvirt.so.0 #15 0x000002aa1f2d516c in remoteDispatchDomainCreate (server=<optimized out>, msg=0x2aa4c488b00, args=<optimized out>, rerr=0x3ff883fdae8, client=<optimized out>) at remote/remote_daemon_dispatch_stubs.h:4434 #16 remoteDispatchDomainCreateHelper (server=<optimized out>, client=<optimized out>, msg=0x2aa4c488b00, rerr=0x3ff883fdae8, args=<optimized out>, ret=0x3ff78004c80) at remote/remote_daemon_dispatch_stubs.h:4410 #17 0x000003ff8c78bc80 in virNetServerProgramDispatch () from /lib64/libvirt.so.0 #18 0x000003ff8c792aba in virNetServerHandleJob () from /lib64/libvirt.so.0 #19 0x000003ff8c6bbb16 in virThreadPoolWorker () from /lib64/libvirt.so.0 #20 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #21 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #22 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 2 (Thread 0x3ff88bff910 (LWP 193298)): #0 0x000003ff8b70d7b8 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 #1 0x000003ff8c6baf92 in virCondWait () from /lib64/libvirt.so.0 #2 0x000003ff8c6bbbe0 in virThreadPoolWorker () from /lib64/libvirt.so.0 #3 0x000003ff8c6bace6 in virThreadHelper () from /lib64/libvirt.so.0 #4 0x000003ff8b7079a8 in start_thread () from /lib64/libpthread.so.0 #5 0x000003ff8b5f9706 in thread_start () from /lib64/libc.so.6 Thread 1 (Thread 0x3ff8cbd7970 (LWP 193297)): #0 0x000003ff8b5ee502 in poll () from /lib64/libc.so.6 #1 0x000003ff8c65629c in virEventPollRunOnce () from /lib64/libvirt.so.0 #2 0x000003ff8c654caa in virEventRunDefaultImpl () from /lib64/libvirt.so.0 #3 0x000003ff8c7921de in virNetDaemonRun () from /lib64/libvirt.so.0 #4 0x000002aa1f2a4c12 in main (argc=<optimized out>, argv=<optimized out>) at remote/remote_daemon.c:1461 Yes, I get the same (expected) behaviour, just with the timeout. It's entirely possible that my setup is broken, this was just a quick test. If you can't reproduce it or make sense out of the backtrace then it's fine, I will try to put some time to debug it next week. I was just interested if this would fix a long-standing bug where this scenario would crash the first domain because the label changed. Bjoern -- IBM Systems Linux on Z & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 ------------------------------------------------------------------------ Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 09/27/2018 11:11 AM, Bjoern Walk wrote: When there are two threads fighting over the lock. Imagine two threads trying to set security label over the same file. Or imagine two daemons on two distant hosts trying to set label on the same file on NFS. virtlockd implements try-or-fail approach. So we need to call lock() repeatedly until we succeed (or timeout). Agreed. I will look into this. But this should not clash, because metadata lock use different bytes: the regular locking uses offset=0, metadata lock uses offset=1. Both locks lock just one byte in length. However, from the logs it looks like virtlockd doesn't try to actually acquire the lock because it found in internal records that the path is locked (even though it is locked with different offset). Anyway, now I am finally able to reproduce the issue and I'll look into this too. Quick and dirty patch might look something like this: diff --git i/src/util/virlockspace.c w/src/util/virlockspace.c index 79fa48d365..3c51d7926b 100644 --- i/src/util/virlockspace.c +++ w/src/util/virlockspace.c @@ -630,8 +630,9 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace, virMutexLock(&lockspace->lock); if ((res = virHashLookup(lockspace->resources, resname))) { - if ((res->flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) && - (flags & VIR_LOCK_SPACE_ACQUIRE_SHARED)) { + if ((res->flags & VIR_LOCK_SPACE_ACQUIRE_SHARED && + flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) || + start == 1) { if (VIR_EXPAND_N(res->owners, res->nOwners, 1) < 0) goto cleanup; But as I say, this is very dirty hack. I need to find a better solution. At least you might have something to test. Michal

On 09/27/2018 01:16 PM, Bjoern Walk wrote: The point of metadata locking is not to prevent metadata change, but to be able to atomically change it. As I said in other thread, there is not much point in this feature alone since chown() and setfcon() are atomic themselves. But this is preparing the code for original label remembering which will be stored in XATTRs at which point the whole operation is not going to be atomic. https://www.redhat.com/archives/libvir-list/2018-September/msg01349.html Long story short, at the first chown() libvirt will record the original owner of the file into XATTRs and then on the last restore it will use that owner to return the file to instead of root:root. The locks are there because reading XATTR, parsing it, increasing/decreasing refcounter and possibly doing chown() is not atomic. But with locks it is. You should see it if you run lslocks at the right moment. But since metadata locking happens only for a fraction of a second, it is very unlikely that you'll be able to run it at the right moment. Metadata locking is not supposed to prevent you from running two domains with the same disk. Nor to prevent chown() on the file. But since you are using virtlockd to lock the disk contents, it should prevent running such domains, not qemu. I wonder if this is a misconfiguration or something. Perhaps one domain has disk RW and the other has it RO + shared? Michal