The virSecurityStackAddNested() can fail in which case virSecurityManagerNewStack() should fail too. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_manager.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index e1b571ea52..21eb6f7452 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -117,9 +117,13 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary) if (!mgr) return NULL; - virSecurityStackAddNested(mgr, primary); + if (virSecurityStackAddNested(mgr, primary) < 0) + goto error; return mgr; + error: + virObjectUnref(mgr); + return NULL; } -- 2.16.4

On daemon deinit only fileLockSpaceDir is freed. The other two (scsiLockSpaceDir and lvmLockSpaceDir) are missing even though they are allocated in virLockManagerLockDaemonLoadConfig(). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver_lockd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 957a963a7b..2386b24f40 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -369,6 +369,8 @@ static int virLockManagerLockDaemonDeinit(void) if (!driver) return 0; + VIR_FREE(driver->scsiLockSpaceDir); + VIR_FREE(driver->lvmLockSpaceDir); VIR_FREE(driver->fileLockSpaceDir); VIR_FREE(driver); -- 2.16.4

Currently, there are only two types of resource. So effectively this is a dead code. However, that assumption can change and we shouldn't just silently ignore the error. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver_sanlock.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 3e5f0e37b0..39c2f94a76 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -829,8 +829,10 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, break; default: - /* Ignore other resources, without error */ - break; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown lock manager object type %d for domain lock object"), + type); + return -1; } return 0; -- 2.16.4

This will help in future expansions of the code when it is be harder to track if @newName and/or @newLockspace were already allocated or not and thus whether it is safe to 'return' or we need to 'goto error'. By using the 'cleanup' label those two cases merge into a single one. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver_lockd.c | 44 ++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 3012c71eda..16fce551c3 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -486,6 +486,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, char *newName = NULL; char *newLockspace = NULL; bool autoCreate = false; + int ret = -1; virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | VIR_LOCK_MANAGER_RESOURCE_SHARED, -1); @@ -498,7 +499,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, if (params || nparams) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unexpected parameters for disk resource")); - return -1; + goto cleanup; } if (!driver->autoDiskLease) { if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | @@ -514,12 +515,12 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, driver->lvmLockSpaceDir) { VIR_DEBUG("Trying to find an LVM UUID for %s", name); if (virStorageFileGetLVMKey(name, &newName) < 0) - goto error; + goto cleanup; if (newName) { VIR_DEBUG("Got an LVM UUID %s for %s", newName, name); if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0) - goto error; + goto cleanup; autoCreate = true; break; } @@ -531,12 +532,12 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, driver->scsiLockSpaceDir) { VIR_DEBUG("Trying to find an SCSI ID for %s", name); if (virStorageFileGetSCSIKey(name, &newName) < 0) - goto error; + goto cleanup; if (newName) { VIR_DEBUG("Got an SCSI ID %s for %s", newName, name); if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0) - goto error; + goto cleanup; autoCreate = true; break; } @@ -546,16 +547,16 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, if (driver->fileLockSpaceDir) { if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0) - goto error; + goto cleanup; if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) - goto error; + goto cleanup; autoCreate = true; VIR_DEBUG("Using indirect lease %s for %s", newName, name); } else { if (VIR_STRDUP(newLockspace, "") < 0) - goto error; + goto cleanup; if (VIR_STRDUP(newName, name) < 0) - goto error; + goto cleanup; VIR_DEBUG("Using direct lease for %s", name); } @@ -569,7 +570,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, if (params[i].value.ul != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Offset must be zero for this lock manager")); - return -1; + goto cleanup; } } else if (STREQ(params[i].key, "lockspace")) { lockspace = params[i].value.str; @@ -579,33 +580,33 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected parameter %s for lease resource"), params[i].key); - return -1; + goto cleanup; } } if (!path || !lockspace) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing path or lockspace for lease resource")); - return -1; + goto cleanup; } if (virAsprintf(&newLockspace, "%s/%s", path, lockspace) < 0) - return -1; + goto cleanup; if (VIR_STRDUP(newName, name) < 0) - goto error; + goto cleanup; } break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), type); - return -1; + goto cleanup; } if (VIR_EXPAND_N(priv->resources, priv->nresources, 1) < 0) - goto error; + goto cleanup; - priv->resources[priv->nresources-1].lockspace = newLockspace; - priv->resources[priv->nresources-1].name = newName; + VIR_STEAL_PTR(priv->resources[priv->nresources-1].lockspace, newLockspace); + VIR_STEAL_PTR(priv->resources[priv->nresources-1].name, newName); if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED) priv->resources[priv->nresources-1].flags |= @@ -615,12 +616,11 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, priv->resources[priv->nresources-1].flags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE; - return 0; - - error: + ret = 0; + cleanup: VIR_FREE(newLockspace); VIR_FREE(newName); - return -1; + return ret; } -- 2.16.4

This flag causes virtlockd to use different offset when locking the file. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_daemon_dispatch.c | 10 ++++++++-- src/locking/lock_driver_lockd.c | 3 ++- src/locking/lock_driver_lockd.h | 1 + 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 10248ec0b5..a683ad3d6b 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -37,6 +37,9 @@ VIR_LOG_INIT("locking.lock_daemon_dispatch"); #include "lock_daemon_dispatch_stubs.h" +#define DEFAULT_OFFSET 0 +#define METADATA_OFFSET 1 + static int virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client, @@ -50,13 +53,14 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU virNetServerClientGetPrivateData(client); virLockSpacePtr lockspace; unsigned int newFlags; - off_t start = 0; + off_t start = DEFAULT_OFFSET; off_t len = 1; virMutexLock(&priv->lock); virCheckFlagsGoto(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE, cleanup); + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA, cleanup); if (priv->restricted) { virReportError(VIR_ERR_OPERATION_DENIED, "%s", @@ -82,6 +86,8 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU newFlags |= VIR_LOCK_SPACE_ACQUIRE_SHARED; if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE) newFlags |= VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE; + if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA) + start = METADATA_OFFSET; if (virLockSpaceAcquireResource(lockspace, args->name, diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 16fce551c3..ca825e6026 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -723,7 +723,8 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, args.flags &= ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE); + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA); if (virNetClientProgramCall(program, client, diff --git a/src/locking/lock_driver_lockd.h b/src/locking/lock_driver_lockd.h index 6931fe7425..bebd804365 100644 --- a/src/locking/lock_driver_lockd.h +++ b/src/locking/lock_driver_lockd.h @@ -25,6 +25,7 @@ enum virLockSpaceProtocolAcquireResourceFlags { VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED = (1 << 0), VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE = (1 << 1), + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA = (1 << 2), }; #endif /* __VIR_LOCK_DRIVER_LOCKD_H__ */ -- 2.16.4

The fact whether domain has or hasn't RW disks is specific to VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN and therefore should reside in union specific to it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver_lockd.c | 187 +++++++++++++++++++++------------------- 1 file changed, 100 insertions(+), 87 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 8ca0cf5426..98953500b7 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -63,6 +63,8 @@ struct _virLockManagerLockDaemonPrivate { char *name; int id; pid_t pid; + + bool hasRWDisks; } dom; struct { @@ -74,8 +76,6 @@ struct _virLockManagerLockDaemonPrivate { size_t nresources; virLockManagerLockDaemonResourcePtr resources; - - bool hasRWDisks; }; @@ -566,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->t.dom.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"), @@ -711,8 +723,9 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY | VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1); - if (priv->nresources == 0 && - priv->hasRWDisks && + if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN && + priv->nresources == 0 && + priv->t.dom.hasRWDisks && driver->requireLeaseForDisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Read/write, exclusive access, disks were present, but no leases specified")); -- 2.16.4

At the beginning of each dispatch function we check if owner attributes were registered (these consist of ID, UUID, PID and name). The check then consists of checking if ID is not zero. This is not going to work with VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON which doesn't set ID. Switch to setting PID which is available for both cases. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_daemon_dispatch.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index a683ad3d6b..36a2462592 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -68,7 +68,7 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU goto cleanup; } - if (!priv->ownerId) { + if (!priv->ownerPid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -129,7 +129,7 @@ virLockSpaceProtocolDispatchCreateResource(virNetServerPtr server ATTRIBUTE_UNUS goto cleanup; } - if (!priv->ownerId) { + if (!priv->ownerPid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -178,7 +178,7 @@ virLockSpaceProtocolDispatchDeleteResource(virNetServerPtr server ATTRIBUTE_UNUS goto cleanup; } - if (!priv->ownerId) { + if (!priv->ownerPid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -227,7 +227,7 @@ virLockSpaceProtocolDispatchNew(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - if (!priv->ownerId) { + if (!priv->ownerPid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -282,7 +282,7 @@ virLockSpaceProtocolDispatchRegister(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - if (!args->owner.id) { + if (!args->owner.pid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -329,7 +329,7 @@ virLockSpaceProtocolDispatchReleaseResource(virNetServerPtr server ATTRIBUTE_UNU goto cleanup; } - if (!priv->ownerId) { + if (!priv->ownerPid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -379,7 +379,7 @@ virLockSpaceProtocolDispatchRestrict(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - if (!priv->ownerId) { + if (!priv->ownerPid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; -- 2.16.4

This flag causes connection to be opened when needed (e.g. when calling virLockManagerLockDaemonAcquire for the first time) and instead of closing it at the end of such API store it in privateData so that it can be reused by later calls. This is needed because if a resource is acquired and connection is closed then virtlockd kills the registered PID (that's what virtlockd is designed to do). Therefore we will need the connection to open at drvAcquire and close not any sooner than drvRelease. However, as we will be locking files step-by-step we want to avoid opening new connection for every drvAcquire + drvRelease pair, so the connection is going to be shared even more than that. But more on that in next commit. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver.h | 7 +++++ src/locking/lock_driver_lockd.c | 68 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 59c4c3aac7..7e3ffc58b5 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -67,8 +67,15 @@ 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), + /* Causes driver to keep connection open and reuse it for further use. */ + VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN = (1 << 2), } virLockManagerAcquireFlags; +typedef enum { + /* Reuse previously saved connection. */ + VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN = (1 << 0), +} virLockManagerReleaseFlags; + typedef enum { /* virLockManagerNew called for a freshly started domain */ VIR_LOCK_MANAGER_NEW_STARTED = (1 << 0), diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 4883e89ac6..14f9eae760 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -76,6 +76,11 @@ struct _virLockManagerLockDaemonPrivate { size_t nresources; virLockManagerLockDaemonResourcePtr resources; + + int clientRefs; + virNetClientPtr client; + virNetClientProgramPtr program; + int counter; }; @@ -440,6 +445,13 @@ virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv) default: break; } + + if (priv->client) { + virNetClientClose(priv->client); + virObjectUnref(priv->client); + virObjectUnref(priv->program); + } + VIR_FREE(priv); } @@ -770,7 +782,8 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, 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_KEEP_OPEN, -1); if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN && priv->nresources == 0 && @@ -781,7 +794,14 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, return -1; } - if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter))) + if (flags & VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN) { + client = priv->client; + program = priv->program; + counter = priv->counter; + } + + if (!client && + !(client = virLockManagerLockDaemonConnect(lock, &program, &counter))) goto cleanup; if (fd && @@ -814,11 +834,25 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, virLockManagerLockDaemonConnectionRestrict(lock, client, program, &counter) < 0) goto cleanup; + if (flags & VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN) { + VIR_STEAL_PTR(priv->client, client); + VIR_STEAL_PTR(priv->program, program); + priv->counter = counter; + } + rv = 0; cleanup: - if (rv != 0 && fd) - VIR_FORCE_CLOSE(*fd); + if (rv < 0) { + if (fd) + VIR_FORCE_CLOSE(*fd); + + priv->client = NULL; + priv->program = NULL; + priv->counter = 0; + priv->clientRefs = 0; + } + virNetClientClose(client); virObjectUnref(client); virObjectUnref(program); @@ -837,12 +871,20 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, size_t i; virLockManagerLockDaemonPrivatePtr priv = lock->privateData; - virCheckFlags(0, -1); + virCheckFlags(VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN, -1); if (state) *state = NULL; - if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter))) + if (flags & VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN) { + client = priv->client; + program = priv->program; + counter = priv->counter; + priv->clientRefs--; + } + + if (!client && + !(client = virLockManagerLockDaemonConnect(lock, &program, &counter))) goto cleanup; for (i = 0; i < priv->nresources; i++) { @@ -870,9 +912,23 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, goto cleanup; } + if (flags & VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN) { + /* Avoid freeing in cleanup. */ + client = NULL; + program = NULL; + counter = 0; + } + rv = 0; cleanup: + if (rv < 0) { + priv->client = NULL; + priv->program = NULL; + priv->counter = 0; + priv->clientRefs = 0; + } + virNetClientClose(client); virObjectUnref(client); virObjectUnref(program); -- 2.16.4

In some cases we might want to not load the lock driver config. Alter virLockManagerPluginNew() and the lock drivers to cope with this fact. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver_lockd.c | 4 +++- src/locking/lock_driver_sanlock.c | 4 +++- src/locking/lock_manager.c | 8 ++++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index aec768b0df..c0598e6987 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -370,8 +370,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 30a0fd996e..c2ff7afb70 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 @@ -132,9 +134,11 @@ virLockManagerPluginPtr virLockManagerPluginNew(const char *name, VIR_DEBUG("name=%s driverName=%s configDir=%s flags=0x%x", name, driverName, 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

When creating the security managers stack load the lock plugin too. This is done by creating a single object that all secdrivers take a reference to. We have to have one shared object so that the connection to virlockd can be shared between individual secdrivers. It is important that the connection is shared because if the connection is closed from one driver while other has a file locked, then virtlockd does its job and kills libvirtd. The cfg.mk change is needed in order to allow syntax-check to include lock_manager.h. This is generally safe thing to do as this APIs defined there will always exist. 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/qemu/qemu_driver.c | 12 ++++-- src/security/security_manager.c | 81 ++++++++++++++++++++++++++++++++++++++++- src/security/security_manager.h | 3 +- tests/testutilsqemu.c | 2 +- 5 files changed, 94 insertions(+), 8 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/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index da8c4e8991..e06dee8dfb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -358,7 +358,9 @@ qemuSecurityInit(virQEMUDriverPtr driver) flags))) goto error; if (!stack) { - if (!(stack = qemuSecurityNewStack(mgr))) + if (!(stack = qemuSecurityNewStack(mgr, + cfg->metadataLockManagerName ? + cfg->metadataLockManagerName : "nop"))) goto error; } else { if (qemuSecurityStackAddNested(stack, mgr) < 0) @@ -372,7 +374,9 @@ qemuSecurityInit(virQEMUDriverPtr driver) QEMU_DRIVER_NAME, flags))) goto error; - if (!(stack = qemuSecurityNewStack(mgr))) + if (!(stack = qemuSecurityNewStack(mgr, + cfg->metadataLockManagerName ? + cfg->metadataLockManagerName : "nop"))) goto error; mgr = NULL; } @@ -389,7 +393,9 @@ qemuSecurityInit(virQEMUDriverPtr driver) qemuSecurityChownCallback))) goto error; if (!stack) { - if (!(stack = qemuSecurityNewStack(mgr))) + if (!(stack = qemuSecurityNewStack(mgr, + cfg->metadataLockManagerName ? + cfg->metadataLockManagerName : "nop"))) goto error; } else { if (qemuSecurityStackAddNested(stack, mgr) < 0) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 21eb6f7452..caaff1f703 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -28,21 +28,39 @@ #include "viralloc.h" #include "virobject.h" #include "virlog.h" +#include "locking/lock_manager.h" #define VIR_FROM_THIS VIR_FROM_SECURITY VIR_LOG_INIT("security.security_manager"); +typedef struct _virSecurityManagerLock virSecurityManagerLock; +typedef virSecurityManagerLock *virSecurityManagerLockPtr; +struct _virSecurityManagerLock { + virObjectLockable parent; + + virCond cond; + + virLockManagerPluginPtr lockPlugin; + virLockManagerPtr lock; + + bool pathLocked; +}; + struct _virSecurityManager { virObjectLockable parent; virSecurityDriverPtr drv; unsigned int flags; const char *virtDriver; + + virSecurityManagerLockPtr lock; + void *privateData; }; static virClassPtr virSecurityManagerClass; +static virClassPtr virSecurityManagerLockClass; static @@ -52,16 +70,36 @@ void virSecurityManagerDispose(void *obj) if (mgr->drv->close) mgr->drv->close(mgr); + + virObjectUnref(mgr->lock); + VIR_FREE(mgr->privateData); } +static void +virSecurityManagerLockDispose(void *obj) +{ + virSecurityManagerLockPtr lock = obj; + + virCondDestroy(&lock->cond); + + if (lock->lock) + virLockManagerCloseConn(lock->lock, 0); + virLockManagerFree(lock->lock); + virLockManagerPluginUnref(lock->lockPlugin); +} + + static int virSecurityManagerOnceInit(void) { if (!VIR_CLASS_NEW(virSecurityManager, virClassForObjectLockable())) return -1; + if (!VIR_CLASS_NEW(virSecurityManagerLock, virClassForObjectLockable())) + return -1; + return 0; } @@ -106,8 +144,32 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv, } +static virSecurityManagerLockPtr +virSecurityManagerLockNew(const char *lockManagerName) +{ + virSecurityManagerLockPtr ret; + + if (!(ret = virObjectLockableNew(virSecurityManagerLockClass))) + return NULL; + + if (virCondInit(&ret->cond) < 0) + goto error; + + if (!(ret->lockPlugin = virLockManagerPluginNew(lockManagerName, + NULL, NULL, 0))) { + goto error; + } + + return ret; + error: + virObjectUnref(ret); + return NULL; +} + + virSecurityManagerPtr -virSecurityManagerNewStack(virSecurityManagerPtr primary) +virSecurityManagerNewStack(virSecurityManagerPtr primary, + const char *lockManagerName) { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverStack, @@ -117,9 +179,16 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary) if (!mgr) return NULL; + if (!(mgr->lock = virSecurityManagerLockNew(lockManagerName))) + goto error; + if (virSecurityStackAddNested(mgr, primary) < 0) goto error; + /* Propagate lock manager */ + if (!primary->lock) + primary->lock = virObjectRef(mgr->lock); + return mgr; error: virObjectUnref(mgr); @@ -133,7 +202,15 @@ virSecurityManagerStackAddNested(virSecurityManagerPtr stack, { if (STRNEQ("stack", stack->drv->name)) return -1; - return virSecurityStackAddNested(stack, nested); + + if (virSecurityStackAddNested(stack, nested) < 0) + return -1; + + /* Propagate lock manager */ + if (!nested->lock) + nested->lock = virObjectRef(stack->lock); + + return 0; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 1ead369e82..c589b8808d 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -47,7 +47,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, unsigned int flags); -virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary); +virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary, + const char *lockManagerName); int virSecurityManagerStackAddNested(virSecurityManagerPtr stack, virSecurityManagerPtr nested); diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 8438613f28..2a2a88361b 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -721,7 +721,7 @@ int qemuTestDriverInit(virQEMUDriver *driver) if (!(mgr = virSecurityManagerNew("none", "qemu", VIR_SECURITY_MANAGER_PRIVILEGED))) goto error; - if (!(driver->securityManager = virSecurityManagerNewStack(mgr))) + if (!(driver->securityManager = virSecurityManagerNewStack(mgr, "nop"))) goto error; return 0; -- 2.16.4

Expose two APIs to lock and unlock metadata for given path. As the comment from the header file says, this is somewhat cumbersome, but it does not seem there is a better way. The idea is that a security driver (like DAC or SELinux) will call virSecurityManagerMetadataLock() just before they are about to change the label followed by virSecurityManagerMetadataUnlock() immediately after. Now, because we can not make virlockd multithreaded (it uses process associated POSIX locks where if one thread holds a lock and another one open()+close() the same file it causes the lock to be released), we can't have virtlockd to wait for the lock to be set. There is just one thread so if that one waits for the lock to be set there will not be another one coming to release the lock. Therefore we have to implement 'try-set' at libvirtd side. This is done by calling virLockManagerAcquire() in a loop with possible usleep() until certain timeout is reached. Out of thin air, the deadline was chosen to be 10 seconds with the maximum sleeping time of 100 ms. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_manager.c | 184 ++++++++++++++++++++++++++++++++++++++++ src/security/security_manager.h | 14 +++ 2 files changed, 198 insertions(+) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 2238c75a5c..3ab06e0c4a 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -28,7 +28,10 @@ #include "viralloc.h" #include "virobject.h" #include "virlog.h" +#include "virstring.h" #include "locking/lock_manager.h" +#include "virrandom.h" +#include "virtime.h" #define VIR_FROM_THIS VIR_FROM_SECURITY @@ -1389,3 +1392,184 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr, return 0; } + + +static virLockManagerPtr +virSecurityManagerNewLockManager(virSecurityManagerLockPtr mgrLock) +{ + virLockManagerPtr lock; + virLockManagerParam params[] = { + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UUID, + .key = "uuid", + }, + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING, + .key = "name", + .value = { .cstr = "libvirtd-sec" }, + }, + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT, + .key = "pid", + .value = { .iv = getpid() }, + }, + }; + const unsigned int flags = 0; + + if (virGetHostUUID(params[0].value.uuid) < 0) + return NULL; + + if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(mgrLock->lockPlugin), + VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON, + ARRAY_CARDINALITY(params), + params, + flags))) + return NULL; + + return lock; +} + + +/* How many miliseconds should we wait for the lock to be + * acquired before claiming error. */ +#define METADATA_LOCK_WAIT_MAX (10 * 1000) + +/* What is the maximum sleeping time (in miliseconds) between + * retries. */ +#define METADATA_LOCK_SLEEP_MAX (100) + +int +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr, + const char *path) +{ + virSecurityManagerLockPtr lock = mgr->lock; + unsigned long long now; + unsigned long long then; + int ret = -1; + + VIR_DEBUG("mgr=%p path=%s lock=%p", mgr, path, lock); + + if (!lock) + return 0; + + virObjectLock(lock); + + while (lock->pathLocked) { + if (virCondWait(&lock->cond, &lock->parent.lock) < 0) { + virReportSystemError(errno, "%s", + _("failed to wait on metadata condition")); + goto cleanup; + } + } + + if (!lock->lock && + !(lock->lock = virSecurityManagerNewLockManager(lock))) + goto cleanup; + + if (virLockManagerAddResource(lock->lock, + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA, + path, 0, NULL, 0) < 0) + goto cleanup; + + if (virTimeMillisNowRaw(&now) < 0) { + virReportSystemError(errno, "%s", + _("Unable to get system time")); + goto cleanup; + } + + then = now + METADATA_LOCK_WAIT_MAX; + while (1) { + uint32_t s; + int rc; + + rc = virLockManagerAcquire(lock->lock, NULL, + VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL); + + if (!rc) + break; + + if (rc < 0) { + virErrorPtr err = virGetLastError(); + + if (err->code == VIR_ERR_SYSTEM_ERROR && + err->int1 == EPIPE) { + /* Because we are sharing a connection, virtlockd + * might have been restarted and thus closed our + * connection. Retry. */ + continue; + } else if (err->code != VIR_ERR_RESOURCE_BUSY) { + /* Some regular error. Exit now. */ + goto cleanup; + } + + /* Proceed to waiting & retry. */ + } + + if (now >= then) + goto cleanup; + + s = virRandomInt(METADATA_LOCK_SLEEP_MAX) + 1; + + if (now + s > then) + s = then - now; + + usleep(1000 * s); + + if (virTimeMillisNowRaw(&now) < 0) { + virReportSystemError(errno, "%s", + _("Unable to get system time")); + goto cleanup; + } + } + + lock->pathLocked = true; + ret = 0; + cleanup: + if (lock->lock) + virLockManagerClearResources(lock->lock, 0); + if (ret < 0) + virSecurityManagerLockCloseConnLocked(lock, false); + virObjectUnlock(lock); + return ret; +} + + +int +virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr, + const char *path) +{ + virSecurityManagerLockPtr lock = mgr->lock; + int ret = -1; + + VIR_DEBUG("mgr=%p path=%s lock=%p", mgr, path, lock); + + if (!lock) + return 0; + + virObjectLock(lock); + + /* Shouldn't happen, but doesn't hurt to check. */ + if (!lock->lock) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unlock mismatch")); + goto cleanup; + } + + if (virLockManagerAddResource(lock->lock, + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA, + path, 0, NULL, 0) < 0) + goto cleanup; + + if (virLockManagerRelease(lock->lock, NULL, + VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN) < 0) + goto cleanup; + + lock->pathLocked = false; + virCondSignal(&lock->cond); + ret = 0; + cleanup: + if (lock->lock) + virLockManagerClearResources(lock->lock, 0); + if (ret < 0) + virSecurityManagerLockCloseConnLocked(lock, true); + virObjectUnlock(lock); + return ret; +} diff --git a/src/security/security_manager.h b/src/security/security_manager.h index c589b8808d..d6f36272eb 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -198,4 +198,18 @@ int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr, int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr, virDomainDefPtr vm); +/* Ideally, these APIs wouldn't be here and the security manager + * would call lock and unlock from these APIs above just before + * calling corresponding callback from the driver. However, that + * means we would have to dig out paths from all the possible + * devices that APIs above handle which effectively means + * duplicating code from the driver (which has to do it already + * anyway). + * Therefore, have these APIs and let the driver call them when + * needed. */ +int virSecurityManagerMetadataLock(virSecurityManagerPtr mgr, + const char *path); +int virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr, + const char *path); + #endif /* VIR_SECURITY_MANAGER_H__ */ -- 2.16.4

This function is going call security manager APIs and therefore it needs pointer to it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 43 ++++++++++++++++++------------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 1a33386b84..3d0c8d20cb 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -654,10 +654,11 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, static int -virSecurityDACRestoreFileLabelInternal(virSecurityDACDataPtr priv, +virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, virStorageSourcePtr src, const char *path) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); int rv; uid_t uid = 0; /* By default return to root:root */ gid_t gid = 0; @@ -682,10 +683,10 @@ virSecurityDACRestoreFileLabelInternal(virSecurityDACDataPtr priv, static int -virSecurityDACRestoreFileLabel(virSecurityDACDataPtr priv, +virSecurityDACRestoreFileLabel(virSecurityManagerPtr mgr, const char *path) { - return virSecurityDACRestoreFileLabelInternal(priv, NULL, path); + return virSecurityDACRestoreFileLabelInternal(mgr, NULL, path); } @@ -812,7 +813,7 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecurityDACRestoreFileLabelInternal(priv, src, NULL); + return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL); } @@ -1026,8 +1027,7 @@ virSecurityDACRestorePCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, void *opaque) { virSecurityManagerPtr mgr = opaque; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - return virSecurityDACRestoreFileLabel(priv, file); + return virSecurityDACRestoreFileLabel(mgr, file); } @@ -1037,8 +1037,7 @@ virSecurityDACRestoreUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, void *opaque) { virSecurityManagerPtr mgr = opaque; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - return virSecurityDACRestoreFileLabel(priv, file); + return virSecurityDACRestoreFileLabel(mgr, file); } @@ -1048,8 +1047,7 @@ virSecurityDACRestoreSCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, void *opaque) { virSecurityManagerPtr mgr = opaque; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - return virSecurityDACRestoreFileLabel(priv, file); + return virSecurityDACRestoreFileLabel(mgr, file); } @@ -1059,8 +1057,7 @@ virSecurityDACRestoreHostLabel(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED, void *opaque) { virSecurityManagerPtr mgr = opaque; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - return virSecurityDACRestoreFileLabel(priv, file); + return virSecurityDACRestoreFileLabel(mgr, file); } @@ -1172,8 +1169,7 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr, if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr))) goto done; - ret = virSecurityDACRestoreFileLabel(virSecurityManagerGetPrivateData(mgr), - vfiodev); + ret = virSecurityDACRestoreFileLabel(mgr, vfiodev); VIR_FREE(vfiodev); break; } @@ -1284,7 +1280,6 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, virDomainChrSourceDefPtr dev_source, bool chardevStdioLogd) { - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityDeviceLabelDefPtr chr_seclabel = NULL; char *in = NULL, *out = NULL; int ret = -1; @@ -1303,7 +1298,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, switch ((virDomainChrType)dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACRestoreFileLabel(priv, dev_source->data.file.path); + ret = virSecurityDACRestoreFileLabel(mgr, dev_source->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -1311,10 +1306,10 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0) goto done; if (virFileExists(in) && virFileExists(out)) { - if (virSecurityDACRestoreFileLabel(priv, out) < 0 || - virSecurityDACRestoreFileLabel(priv, in) < 0) + if (virSecurityDACRestoreFileLabel(mgr, out) < 0 || + virSecurityDACRestoreFileLabel(mgr, in) < 0) goto done; - } else if (virSecurityDACRestoreFileLabel(priv, dev_source->data.file.path) < 0) { + } else if (virSecurityDACRestoreFileLabel(mgr, dev_source->data.file.path) < 0) { goto done; } ret = 0; @@ -1497,12 +1492,11 @@ virSecurityDACRestoreInputLabel(virSecurityManagerPtr mgr, virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainInputDefPtr input) { - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); int ret = -1; switch ((virDomainInputType)input->type) { case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: - ret = virSecurityDACRestoreFileLabel(priv, input->source.evdev); + ret = virSecurityDACRestoreFileLabel(mgr, input->source.evdev); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1522,12 +1516,11 @@ virSecurityDACRestoreMemoryLabel(virSecurityManagerPtr mgr, virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainMemoryDefPtr mem) { - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); int ret = -1; switch ((virDomainMemoryModel) mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - ret = virSecurityDACRestoreFileLabel(priv, mem->nvdimmPath); + ret = virSecurityDACRestoreFileLabel(mgr, mem->nvdimmPath); break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: @@ -1612,7 +1605,7 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, } if (def->os.loader && def->os.loader->nvram && - virSecurityDACRestoreFileLabel(priv, def->os.loader->nvram) < 0) + virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram) < 0) rc = -1; return rc; @@ -1797,7 +1790,7 @@ virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0; - return virSecurityDACRestoreFileLabel(priv, savefile); + return virSecurityDACRestoreFileLabel(mgr, savefile); } -- 2.16.4

These two functions (virSecurityDACSetOwnership and virSecurityDACRestoreFileLabelInternal) do not really change @src. Make it const. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 1be4ead21c..7528d8ba7d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -624,7 +624,7 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, static int virSecurityDACSetOwnership(virSecurityManagerPtr mgr, - virStorageSourcePtr src, + const virStorageSource *src, const char *path, uid_t uid, gid_t gid) @@ -655,7 +655,7 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, static int virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, - virStorageSourcePtr src, + const virStorageSource *src, const char *path) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); -- 2.16.4

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 52 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2115e00e07..818548dd22 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -638,6 +638,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); struct stat sb; int rc; + bool locked = false; + int ret = -1; if (!path && src && src->path && virStorageSourceIsLocalStorage(src)) @@ -657,14 +659,28 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, return -1; } + if (!S_ISDIR(sb.st_mode)) { + if (virSecurityManagerMetadataLock(mgr, path) < 0) + return -1; + locked = true; + } + if (virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid) < 0) - return -1; + goto cleanup; } VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long)uid, (long)gid); - return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) + goto cleanup; + + ret = 0; + cleanup: + if (locked && + virSecurityManagerMetadataUnlock(mgr, path) < 0) + VIR_WARN("Unable to unlock metadata on %s", path); + return ret; } @@ -677,6 +693,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, int rv; uid_t uid = 0; /* By default return to root:root */ gid_t gid = 0; + struct stat sb; + bool locked; + int ret = -1; if (!path && src && src->path && virStorageSourceIsLocalStorage(src)) @@ -691,17 +710,38 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, return 0; if (path) { + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("unable to stat: %s"), path); + return -1; + } + + if (!S_ISDIR(sb.st_mode)) { + if (virSecurityManagerMetadataLock(mgr, path) < 0) + return -1; + locked = true; + } + rv = virSecurityDACRecallLabel(priv, path, &uid, &gid); if (rv < 0) - return -1; - if (rv > 0) - return 0; + goto cleanup; + if (rv > 0) { + ret = 0; + goto cleanup; + } } VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld", NULLSTR(src ? src->path : path), (long)uid, (long)gid); - return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) + goto cleanup; + + ret = 0; + cleanup: + if (locked && + virSecurityManagerMetadataUnlock(mgr, path) < 0) + VIR_WARN("Unable to unlock metadata on %s", path); + return ret; } -- 2.16.4

On 08/27/2018 04:08 AM, Michal Privoznik wrote: Caveat to my comments - I didn't read all the conversations in the previous series... So if using unions was something agreed upon, then mea culpa for being too lazy to go read ;-) [...] Something tells me it'd be better if @dom and @daemon were typedef'd types. Still unless the lock can be shared why separate things? An @id == -1 could signify a lock using @uuid, @name, and @pid is not a @dom lock. using @type is fine as well. I see nothing by the end of the series adding a new type and since the members essentially overlap, it's really not clear why a union should be used. [...] So what happens if "id" and/or "uri" are in params? For DOMAIN we ignore "uri", should that be done here (for both)? Yeah, still nothing here really leads me to say we really need a union. Checking for that id == 0 could still happen if we set it to -1. I'm not against the model, just not fully on board (yet). I know it's existing, but doesn't this one make you pause and go, hmmmm... does param->value.cstr ever get free'd anywhere. doesn't seem so from my quick searches (some constcfg->uri a/k/a qemu:///system or qemu:///session) Technically one is unsupported and the rest are unknown, it it really matters. Let's see if I get a response from you before I finish reviewing or if I decide by the end that I've changed my mind for the R-By... John

On 08/27/2018 04:08 AM, Michal Privoznik wrote: I'm still conflicted with Unknown and Unsupported. Again Unknown and Unsupported... Interstingly enough, @newFlags is adjusted in the new case and we could do the same in the existing case instead of setting @autoCreate, just set the newFlags. Of course I'm quite aware that this could have been done in a separate patch too. IOW: I could easily support removing @autoCreate... Conflict continues. So after all that - I guess I can accept that Unknown will be used and a separate Unsupported isn't necessary. However, I do think removing @autoCreate is worthwhile especially since it's specific to TYPE_DOMAIN and TYPE_DISK. So with that, for the logic at least regardless if the exact location changes as a result of motion that may happen prior to this... Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

On 08/27/2018 04:08 AM, Michal Privoznik wrote: OK, sure, but knowing what the purpose is now would perhaps be more a more useful commit message. Not supported for lock_driver_sanlock.c... It doesn't seem it would be difficult to handle, but yeah, that's not our problem and if we're calling it old technology, then I'm fine with not changing. No verbose documentation on this? The others have it and this needs it. Theoretically could have been done in a separate patch, but then there's this thing about overkill...;-) static int virLock... Consistency with new API's vs. consistency with existing API's... Tough one not to say something about. With suggested amendments, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John [...]

On 08/27/2018 04:08 AM, Michal Privoznik wrote: s/was/will be/ (or is) s/which allows us to keep/to allow keeping the/ Hmm, a client that forgets to provide KEEP_OPEN on one or the other would wreak havoc with the algorithm. Upon further thought, the KEEP_OPEN really only matters for Acquire and it should be saved in @priv. That way the @priv would manage it not whether the flag was provided on release. NB: virObject{Ref|Unlock} uses atomic incr/decr for ref counting. static int virLock... The helper concept from the previous patch still could apply here. I would say I'm surprised this did anything in testing since clientRefs wouldn't be 0 if acquire, then release is called w/ the KEEP_OPEN flag. It'd be -1, I believe. The rest seems fine. John [...]

On 08/27/2018 04:08 AM, Michal Privoznik wrote: No current cases, but sometime in the future the requirement that a configFile exists will be removed.... Waiting with baited fingers to see how it's replaced ;-) I note that virLockDriverInit in src/locking/lock_driver.h doesn't even document @configFile (or seem to care if it was NULL). In any a modification there to describe the argument should be added. You'll need to add NULLSTR around @driverName and @configDir then, right? If they can be NULL? With those adjustments, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

On 08/27/2018 04:08 AM, Michal Privoznik wrote: This I don't really understand - black magic, voodoo stuff ;-) This essentially gets called through the driver state initialize function, right? But, wouldn't daemon locks be at a different level? The domain locks would be managed by whatever is managing the domain, the the daemon locks would be managed by whatever is managing the daemon locks, wouldn't they? This also assumes that security_driver is defined/used which wasn't described in the previous patch (while you understand that, it's not necessarily that obvious). If I just uncomment metadata_lock_manager, but not security_driver, then nothing would happen. I'm trying to figure out the "owner" (so to speak) of this lock. If multiple daemons can use it, then someone has to own it. This partially makes it appear that qemu would own it, but I would think virtlockd would need to own it. As it, when something causes virtlockd to start so that it's managing locks, that's where initialization takes place. The fact that qemu has an "interest" in knowing if the running lockd code is/can handle these metadata locks still would seemingly mean that the lockmanager would need to have a way to indicate that it is managing the locks. Maybe it's just patch order that is causing the blank stare I have. Maybe the answer lies in a subsequent patch, but something just feels off and I'm not sure I can describe it well enough. I'm concerned with libvirtd restart too and someone changing metadata_lock_manager (one way or the other) and how that alters reality. Maybe I'm overthinking, who knows, it is Friday though and I can almost hear the glasses clinking a the pub you're at ;-). Does migration present any strange problems? I guess it's not a true distributed locking mechanism, so perhaps not, but it is a concern. using @lock here is really confusing later on when I see lock->lock. Unused for now... So this is the opposite end (so to speak) of the virObjectRef in virSecurityManagerNewStack and virSecurityManagerStackAddNested, right? Once enough of these are called that then triggers the call to virSecurityManagerLockDispose, right? So without looking forward to the next patch[es] - I'm stumped by this. The code is certainly not related to the Ref/Unref of mgr->lock and I haven't seen anything that's filling in lock->lock yet. Likewise for this since all that virSecurityManagerLockNew does is managed ->cond and ->lockPlugin. I get that future code will handle this, but I think it's easier to review so that when future code is added we then adjust this as well. The { } aren't necessary, but understandable. You're in a world of hurt if @lockManagerName == NULL Maybe this is where the conversion to "nop" could take place. I know we're following existing convention for domain lock code, but should we? Cannot be NULL, true? Should we ATTRIBUTE_NONNULL it? John

On 08/27/2018 04:08 AM, Michal Privoznik wrote: milliseconds ^^^^^^^^^^^ consistent at least ;-) or # define METADATA_LOCK_WAIT_MAX (100 * METADATA_LOCK_SLEEP_MAX) Could use a few words of wisdom here - it's not necessary self documenting. > +int > +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr, > + const char *path) > +{ > + virSecurityManagerLockPtr lock = mgr->lock; > + unsigned long long now; > + unsigned long long then; > + int ret = -1; > + VIR_DEBUG("mgr=%p path=%s lock=%p", mgr, path, lock); > + if (!lock) > + return 0; I'm still wondering how this could be true... If this happens and we return 0, couldn't the caller have a false sense of security? > + virObjectLock(lock); > + while (lock->pathLocked) { Someone already operating on the thing. virCondWaitUntil perhaps? If we get here, but considering the previous patch where something else "force"'d the CondSignal, then patchLocked == false now... So if there were more than 1 waiter what's going to happen next... Should this fail? Should that force code set a flag or something to indicate everyone start walking the plank? > + } > + if (!lock->lock && > + !(lock->lock = virSecurityManagerNewLockManager(lock))) > + goto cleanup; Finally we're getting lock->lock filled in, knew it would happen some day! > + if (virLockManagerAddResource(lock->lock, > + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA, > + path, 0, NULL, 0) < 0) > + goto cleanup; > + if (virTimeMillisNowRaw(&now) < 0) { > + virReportSystemError(errno, "%s", > + _("Unable to get system time")); > + goto cleanup; > + } > + then = now + METADATA_LOCK_WAIT_MAX; > + while (1) { > + uint32_t s; > + int rc; > + rc = virLockManagerAcquire(lock->lock, NULL, > + VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN, > + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL); > + if (!rc) > + break; > + if (rc < 0) { > + virErrorPtr err = virGetLastError(); Coverity notes that @err can be NULL at this point and thus the subsequent accesses won't be happen Consider: virLastErrorIsSystemErrno Consider: virGetLastErrorCode > + /* Some regular error. Exit now. */ > + goto cleanup; > + } > + /* Proceed to waiting & retry. */ > + } > + if (now >= then) Might be nice to add a timeout error message... > + goto cleanup; > + s = virRandomInt(METADATA_LOCK_SLEEP_MAX) + 1; > + if (now + s > then) > + s = then - now; > + usleep(1000 * s); > + if (virTimeMillisNowRaw(&now) < 0) { > + virReportSystemError(errno, "%s", > + _("Unable to get system time")); > + goto cleanup; > + } Does this really need to be all that complicated? What about using virTimeBackOff{Start|Wait} > + } > + lock->pathLocked = true; Yay, been waiting for this one too ;-) Should this code grab/save the current error message if (ret < 0) so that nothing overwrites it in the subsequent calls? Coverity also notes that by checking lock->lock here But not here... means it's possible the above blindly derefs lock->lock eventually in virLockManagerCloseConn Beyond that why are we calling virLockManagerClearResources if we have acquired the lock? > + virObjectUnlock(lock); > + return ret; > +} > +int > +virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr, > + const char *path) > +{ > + virSecurityManagerLockPtr lock = mgr->lock; > + int ret = -1; > + VIR_DEBUG("mgr=%p path=%s lock=%p", mgr, path, lock); > + if (!lock) > + return 0; Sigh. > + virObjectLock(lock); > + /* Shouldn't happen, but doesn't hurt to check. */ > + if (!lock->lock) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("unlock mismatch")); > + goto cleanup; > + } > + if (virLockManagerAddResource(lock->lock, > + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA, > + path, 0, NULL, 0) < 0) > + goto cleanup; Shouldn't the resource already be added? If we didn't clear the resources above, then we wouldn't need this would we? I could be missing something subtle... > + if (virLockManagerRelease(lock->lock, NULL, > + VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN) < 0) > + goto cleanup; > + lock->pathLocked = false; > + virCondSignal(&lock->cond); > + virLockManagerClearResources(lock->lock, 0); This would seemingly happen after successful Release wouldn't it? I Add a resource, I lock a resource, I use a resource, I unlock a resource, I clear a resource. John > + if (ret < 0) > + virSecurityManagerLockCloseConnLocked(lock, true); > + virObjectUnlock(lock); > + return ret; > +} > diff --git a/src/security/security_manager.h b/src/security/security_manager.h > index c589b8808d..d6f36272eb 100644 > --- a/src/security/security_manager.h > +++ b/src/security/security_manager.h > @@ -198,4 +198,18 @@ int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr, > int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr, > virDomainDefPtr vm); > > +/* Ideally, these APIs wouldn't be here and the security manager > + * would call lock and unlock from these APIs above just before > + * calling corresponding callback from the driver. However, that > + * means we would have to dig out paths from all the possible > + * devices that APIs above handle which effectively means > + * duplicating code from the driver (which has to do it already > + * anyway). > + * Therefore, have these APIs and let the driver call them when > + * needed. */ > +int virSecurityManagerMetadataLock(virSecurityManagerPtr mgr, > + const char *path); > +int virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr, > + const char *path); > #endif /* VIR_SECURITY_MANAGER_H__ */ >

On 08/27/2018 04:08 AM, Michal Privoznik wrote: You have nothing to say here, wow we got this far and your speechless... Or your fingers were just really tired from all those patches! Seems we leave ourselves open for a few other odd combinations we don't want... Should this be restricted to certain things like S_ISREG? Coverity says, please initialize this to false since it's used in cleanup Similar S_ISREG I'll wait for when there's an update before R_By, but I see nothing inherently wrong that sticks out - I'm just in search of liquid refreshment right now, so this is as good as it gets. John

On 08/31/2018 07:35 PM, John Ferlan wrote: This is a simple bash version of switch-case. Except in bash you'd s/switch/case/ (because why not, right?). So what this says is: if $dir is "util/" then safe="util"; or if $dir is either of "cpu/", "network/", "node_device/" .... then safe is "($dir|util|conf|storage)", of if $dir is "security/" then safe is "($dir|util|conf|storage|locking)". Long story short, this shuts up 'syntax-check' because without it it complains about "unsafe" cross-directory include. Hmpf. There is no way you can disable DAC driver. That one is always there. So at least one driver is always enabled. I'm not sure what you mean by "owner of the lock". There is owner of the lock as recorded in the kernel (this is going to be virtlockd after it fcntl(fd, F_SETLK, ..)-s the file we tell it to). The virtlockd however has its own records which tell on behalf of which PID it has done the locking. Such record is also referred to as the lock owner. Then there is lock driver and all that machinery. Its owner is the security driver because security driver is actually the place where chown()/setfilecon() happen. And we want both security drivers (DAC + SELinux) to have the same lock driver so that they can share one connection to virtlockd. It's important to realize that once a connection to virtlockd is closed and there was at least one path locked (= one metadata type lock acquired) ALL bets are off. virtlockd will trigger client cleanup code (virLockDaemonClientFree) and the registered lock owner (=libvirtd) is killed instantly. With this in mind all the KEEP_OPEN flags and refcounting starts to make sense. Daemon restart should not have any implications. If metadata_lock_manager was set, then virtlockd will release all the metadata locks that restarting daemon might have had (yay, we want this!), or if it wasn't then nothing needs releasing. No problems there in my testing. so maybe s/lock/metadataLock/ here? Yes. Yes. As I say above, the idea is to have only one instance of lock driver for both security drivers. Only then the connection can be shared. If it was two independent lock drivers (one per each sec driver), there is no way we could make them to share one connection. Therefore, the first call that creates a security driver has to create the lock driver, and any subsequent call to sec driver create will just take a reference to already existing lock driver. This is basically there to clean any stale connection. Any connection where our lock() is not paired with unlock() (should there be such case). Okay, I'll move it respective patches. Andrea would disagree. In one other patch that I had on the list recently he made me to put the brackets there arguing that if() is not a single line or something. On a side note, this points to a bug in our coding style. Either we should put brackets everywhere (even for true single line if()-s like those in the context above), or not be picky about "multiline" if()-s like the one we are talking about here. Sure, but such call is broken virtually by definition :-) I rather segfault than proceed with false sense of safety. Sure. Michal

On 08/31/2018 02:32 PM, John Ferlan wrote: I'm not sure we can do this. There are places (e.g. regular disk content locking code) where Acquire() and Release() are called separately and on behalf of other process (domain is the owner of the lock). Definitely we do not want any connection sharing there. Therefore I'm failing to see how would Acquire() determine if it should save the connection for later use or not. The code needs to handle both cases, if the flag is set and if it isn't. I don't want to write the code twice with the only difference being in the variable accessed (priv->client vs client). Yes. The client socket is closed by code below. There's not much point in keeping stale pointer to what used to be connection. Oooops. Yes. Somehow the line that increments the clientRefs counter when grabbing the client got lost. Damn you small dwarfs who make some lines disappear! :-D Yes. We don't want to free client data at this point. Maybe it is too drastic. But rather to be safe then sorry. If something goes wrong, we can't be sure what state the connection is in and it's better for the caller to open new connection. Well, do we want each caller to re-implement this semantic? I say it's better to have it at one place. Michal

On 08/30/2018 11:34 PM, John Ferlan wrote: As I explain in one of my previous replies, users are not really expected to see this message. Is merely for us to avoid broken code pattern. Even if it so happens that broken code slips through review, what difference does it make for users to see "Unsupported lock manager object type" vs "Unknown lock manager object type"? They'll file a bug and we will notice immediately what is the problem when looking into the code (we will notice it because the error message logs type number). Okay. Michal

On 08/30/2018 10:40 PM, John Ferlan wrote: I don't see much benefit to that but I can do that change. The idea is that in virLockManagerLockNew() one specifies which type of lock they want (whether it's a domain or daemon type of lock) and any subsequent call to virLockManager*() will either succeed or fail if they want to work with wrong object. So this can be viewed as namespacing. Basically I want to avoid this code pattern: lock = virLockManagerNew(virLockManagerPluginGetDriver(plugin), VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN, ARRAY_CARDINALITY(params), params, flags))); virLockManagerAddResource(lock, VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, src->path, 0, NULL, 0); virLockManagerAddResource(lock, VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA, src->path, 0, NULL, 0); This is a buggy pattern, because OBJECT_TYPE_DOMAIN is associated with the domain (the owner of the lock is the domain, the ownerPID is set from domain, etc.). However, we want RESOURCE_TYPE_METADATA to be associated with the libvirtd and not the domain [1]. 1 - the reason for the metadata lock to be associated with the libvirtd and not the domain is very simple: it's libvirtd who is changing the metadata so it should grab the lock too. Also, if registered owner of a lock disappears (e.g. due to a crash) virlockd releases all the locks the owner had. And if you view metadata locking from this angle, it is obvious that we want the metadata lock to be released if libvirtd crashes. Not the domain. Consider the following scenario: two hosts (A and B), two domains running (D1 on A and D2 on B) and both hosts want to plug disk X into respective domains. HostA: libvirtd locks path X for metadata HostB: libvirtd locks path X for metadata, but since the path is already locked (by hostA) this call will just wait for unlock HostA: libvirtd wants to proceed to chown() but it crashes instead (e.g. segfault from another thread or something) At this point, we want the lock to be released and allow HostB to proceed with locking. If the lock was associated with domain, then the lock wouldn't be released and HostB wouldn't be able to plug in the disk. We error out .. .. here. The uri is ignored for virtlockd driver because it's used in sanlock. And the whole point of drivers is to have a single API with multiple implementations. Therefore if somebody calls: virLockManagerParam params[] = { ... { .type = VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, .key = "uri", .value = { .cstr = uri }, }, ... }; lock = virLockManagerNew(virLockManagerPluginGetDriver(plugin), VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN, ARRAY_CARDINALITY(params), params, flags); they don't have to care whether driver in use is lockd or sanlock. They have unified APIs to call. Since we don need that luxury for OBJECT_TYPE_DAEMON, we should error right out letting caller know they passed unsupported parameter. Sure, we can have one big struct and use just a portion of it. I don't care that much. To me it's just cosmetics. I've chosen union because it's more memory friendly perhaps? For instance: struct { int A; /* for domain */ int B; /* for daemon */ }; struct { union { int A; int B; }; }; The former struct will be 2 ints big while the latter only one int big. And because we will never use both at the same time (due to my explanation above) I went with the latter. It's used in other areas of the code too. From them all I'd pick _virDomainDeviceDef or _virDomainChrSourceDef as an example. That's right. And it never should! cstr is a const char: struct _virLockManagerParam { int type; const char *key; union { int iv; long long l; unsigned int ui; unsigned long long ul; double d; char *str; const char *cstr; unsigned char uuid[16]; } value; }; (oh look, another union ;-)) Michal

On 08/31/2018 08:42 PM, John Ferlan wrote: Because of the connection sharing (KEEP_OPEN flags) both AcquireResources and ReleaseResources will either use previously opened connection OR open a new one AND leave it open. This is important - that after the last ReleaseResource there is still a connection open to virtlockd. ReleaseResouce can't close the connection because it can't possibly know whether there is new acquire/release call pair waiting or not. It's security driver who knows that. And thus, the last one should close the connection. View from another angle, when relabelling a device we may end up relabelling one, two or even more paths. For instance: virSecurityManagerDomainSetPathLabel() relabels just one path, virSecurityManagerSetChardevLabel() or virSecurityManagerSetHostdevLabel() can relabel one, or many paths, and finally, virSecurityManagerSetAllLabel() will definitely relabel plenty of paths. Having said that, it's clear now that ReleaseResouce can't know if the path its releasing is the last one in the queue and thus if the connection can be closed. But the API knows that. So after security manager API has called the callback it knows no other path needs relabelling (i.e. acquire+release) and thus the connection can be closed. Except if there is not another thread that is already talking to virlockd and locking/unlocking paths for some other domain. Hence the connection refcounting in one of the previous patches. Yes, this is very complicated design. And if possible I'd like to simplify it. But I'm not successful on that front, sorry. Michal

On 09/03/2018 11:13 AM, Michal Privoznik wrote: Consider my your early warning bz then ;-). It doesn't really matter that much... Maybe it's more of an "Unsupported" message regardless of whether it's META or 'default'. At first I considered noting the Enum range error, but it's not the same here. I'll leave it as a design decision and move on. John

On 09/03/2018 11:13 AM, Michal Privoznik wrote: Still black magic... essentially though you've added "locking" to safe list of tree's that security/* files can access. Although we've had the #virt conversation today - I at least wanted to try to provide a bit more context from my thoughts. At first, the owner I was thinking of would be libvirtd w/ PID. Since qemu.conf got changed to add "metadata_lock_manager" and not something perhaps up further and not hypervisor specific. This was perhaps the, ahh, hmm, moment after reading the previous patch leading to self doubt whether the previous one would be right. IIRC, in the long run though what's being developed is something to handle restoring things in the event that the hypervisor neglected to restore/undo or it's untimely death prevented it. Ah, yes, the subtlety of the core principle for the design... I'll try to keep this in mind for future adjustments/reviews. Perhaps overthinking on my part... lock mgmt is a tricky beast. You've had the luxury of more time/iterations to come up with this model. There's some subtleties that I'm more aware of now. I look forward to seeing how things change with the next iteration. OK John [...] ... I also snipped the CC to Andrea out too, since I'm not recommenting on the area you CC'd him for. I agree that the usage of { ... } is one of those gray areas. There's a few other things too, but I fear posting a patch just in case someone asks me to also modify the make check tests too.