The virSecurityStackAddNested() can fail in which case virSecurityManagerNewStack() should fail too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- 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/@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@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- 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:

We will want virtlockd to lock files on behalf of libvirtd and not qemu process, because it is libvirtd that needs an exclusive access not qemu. This requires new lock context.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 + src/locking/lock_driver_lockd.c | 110 +++++++++++++++++++++++++++++++------- src/locking/lock_driver_sanlock.c | 37 ++++++++----- 3 files changed, 117 insertions(+), 32 deletions(-)

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 ;-) [...]

diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index ca825e6026..8ca0cf5426 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;

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.

size_t nresources; virLockManagerLockDaemonResourcePtr resources; @@ -156,10 +167,24 @@ virLockManagerLockDaemonConnectionRegister(virLockManagerPtr lock,

[...]

@@ -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;

So what happens if "id" and/or "uri" are in params? For DOMAIN we ignore "uri", should that be done here (for both)?

+ } 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; +

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).

default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 39c2f94a76..fe422d3be6 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -513,21 +513,32 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,

priv->flags = flags;

- for (i = 0; i < nparams; i++) { - param = ¶ms[i]; + switch ((virLockManagerObjectType) type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: + for (i = 0; i < nparams; i++) { + param = ¶ms[i];

- if (STREQ(param->key, "uuid")) { - memcpy(priv->vm_uuid, param->value.uuid, 16); - } else if (STREQ(param->key, "name")) { - if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) - goto error; - } else if (STREQ(param->key, "pid")) { - priv->vm_pid = param->value.iv; - } else if (STREQ(param->key, "id")) { - priv->vm_id = param->value.ui; - } else if (STREQ(param->key, "uri")) { - priv->vm_uri = param->value.cstr; + if (STREQ(param->key, "uuid")) { + memcpy(priv->vm_uuid, param->value.uuid, 16); + } else if (STREQ(param->key, "name")) { + if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) + goto error; + } else if (STREQ(param->key, "pid")) { + priv->vm_pid = param->value.iv; + } else if (STREQ(param->key, "id")) { + priv->vm_id = param->value.ui; + } else if (STREQ(param->key, "uri")) { + priv->vm_uri = param->value.cstr;

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)

+ } } + break; + + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown lock manager object type %d"),

Technically one is unsupported and the rest are unknown, it it really matters.

+ type); + goto error; }

/* Sanlock needs process registration, but the only way how to probe

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:

This is a new type of object that lock drivers can handle. Currently, it is supported by lockd driver only.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 ++ src/locking/lock_driver_lockd.c | 43 +++++++++++++++++++++++++++++++-------- src/locking/lock_driver_sanlock.c | 3 ++- 3 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index a9d2041c30..9be0abcfba 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -51,6 +51,8 @@ typedef enum { VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK = 0, /* A lease against an arbitrary resource */ VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE = 1, + /* The resource to be locked is a metadata */ + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA = 2, } virLockManagerResourceType;

typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 98953500b7..d7cb183d7a 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -557,6 +557,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, virLockManagerLockDaemonPrivatePtr priv = lock->privateData; char *newName = NULL; char *newLockspace = NULL; + int newFlags = 0; bool autoCreate = false; int ret = -1;

@@ -569,7 +570,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, switch (priv->type) { case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:

- switch (type) { + switch ((virLockManagerResourceType) type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (params || nparams) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -670,6 +671,8 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, goto cleanup;

} break; + + case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:

I'm still conflicted with Unknown and Unsupported.

default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d for domain lock object"), @@ -679,6 +682,29 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, break;

case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + switch ((virLockManagerResourceType) type) { + case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA: + if (params || nparams) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected parameters for metadata resource")); + goto cleanup; + } + if (VIR_STRDUP(newLockspace, "") < 0 || + VIR_STRDUP(newName, name) < 0) + goto cleanup; + newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA; + break; + + case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: + case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE:

Again Unknown and Unsupported...

+ default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown lock manager object type %d for daemon lock object"), + type); + goto cleanup; + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), @@ -686,19 +712,18 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, goto cleanup; }

+ if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED) + newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED; + + if (autoCreate)

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...

+ newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE; + if (VIR_EXPAND_N(priv->resources, priv->nresources, 1) < 0) goto cleanup;

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 |= - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED; - - if (autoCreate) - priv->resources[priv->nresources-1].flags |= - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE; + priv->resources[priv->nresources-1].flags = newFlags;

ret = 0; cleanup: diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index fe422d3be6..9393e7d9a2 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -815,7 +815,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0;

- switch (type) { + switch ((virLockManagerResourceType) type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (driver->autoDiskLease) { if (virLockManagerSanlockAddDisk(driver, lock, name, nparams, params, @@ -839,6 +839,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, return -1; break;

+ case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:

Conflict continues.

default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d for domain lock object"),

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@redhat.com> John

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

This is a counterpart to virLockManagerAddResource. It is going to be handy when using one lock manager to lock multiple files step by step.

OK, sure, but knowing what the purpose is now would perhaps be more a more useful commit message.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/locking/lock_driver.h | 5 +++++ src/locking/lock_driver_lockd.c | 27 ++++++++++++++++++++++++++- src/locking/lock_driver_nop.c | 6 ++++++ src/locking/lock_manager.c | 12 ++++++++++++ src/locking/lock_manager.h | 3 +++ 6 files changed, 53 insertions(+), 1 deletion(-)

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.

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 47ea35f864..42f15f117e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1293,6 +1293,7 @@ virDomainLockProcessStart; # locking/lock_manager.h virLockManagerAcquire; virLockManagerAddResource; +virLockManagerClearResources; virLockManagerFree; virLockManagerInquire; virLockManagerNew; diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 9be0abcfba..59c4c3aac7 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -228,6 +228,10 @@ typedef int (*virLockDriverAddResource)(virLockManagerPtr man, virLockManagerParamPtr params, unsigned int flags);

+

No verbose documentation on this? The others have it and this needs it.

+typedef int (*virLockDriverClearResource)(virLockManagerPtr mgr, + unsigned int flags); + /** * virLockDriverAcquire: * @manager: the lock manager context @@ -313,6 +317,7 @@ struct _virLockDriver { virLockDriverFree drvFree;

virLockDriverAddResource drvAddResource; + virLockDriverClearResource drvClearResources;

virLockDriverAcquire drvAcquire; virLockDriverRelease drvRelease; diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index d7cb183d7a..4883e89ac6 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -402,8 +402,9 @@ static int virLockManagerLockDaemonDeinit(void) return 0; }

+ static void -virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv) +virLockManagerLockDaemonFreeResources(virLockManagerLockDaemonPrivatePtr priv) { size_t i;

@@ -415,6 +416,17 @@ virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv) VIR_FREE(priv->resources[i].name); } VIR_FREE(priv->resources); + priv->nresources = 0; +} + + +static void +virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv) +{ + if (!priv) + return; + + virLockManagerLockDaemonFreeResources(priv);

Theoretically could have been done in a separate patch, but then there's this thing about overkill...;-)

switch (priv->type) { case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: @@ -733,6 +745,18 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, }

+static int virLockManagerLockDaemonClearResources(virLockManagerPtr lock, + unsigned int flags)

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@redhat.com> John [...]

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

After the previous commit we have VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN flag. This is not enough because it will keep connection open for only one instance of drvAcquire + drvRelease call. And when starting up a domain there will be a lot of such calls as there will be a lot of paths to relabel and thus lock. Therfore, VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN flag was introduced which allows us to keep connection open even after

s/was/will be/ (or is) s/which allows us to keep/to allow keeping the/

the drvAcquire + drvRelease pair. In order to close the connection after all locking has been done virLockManagerCloseConn is introduced.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/locking/lock_driver.h | 22 ++++++++++++++++++++++ src/locking/lock_driver_lockd.c | 24 ++++++++++++++++++++++++ src/locking/lock_driver_nop.c | 8 ++++++++ src/locking/lock_manager.c | 11 +++++++++++ src/locking/lock_manager.h | 4 ++++ 6 files changed, 70 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 42f15f117e..bca5a51ba0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1294,6 +1294,7 @@ virDomainLockProcessStart; virLockManagerAcquire; virLockManagerAddResource; virLockManagerClearResources; +virLockManagerCloseConn; virLockManagerFree; virLockManagerInquire; virLockManagerNew; diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 7e3ffc58b5..d81767707b 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -282,6 +282,27 @@ typedef int (*virLockDriverRelease)(virLockManagerPtr man, char **state, unsigned int flags);

+/** + * virLockDriverCloseConn: + * @man: the lock manager context + * @flags: optional flags, currently unused + * + * Close any connection that was saved via + * VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN or + * VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN flags.

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.

+ * However, if there is still a resource locked, do not actually + * close the connection as it would result in killing the + * resource owner. This is similar to refcounting when all + * threads call virLockDriverCloseConn() but only the last one + * actually closes the connection. + *

NB: virObject{Ref|Unlock} uses atomic incr/decr for ref counting.

+ * Returns: 0 on success and connection not actually closed, + * 1 on success and connection closed, + * -1 otherwise + */ +typedef int (*virLockDriverCloseConn)(virLockManagerPtr man, + unsigned int flags); + /** * virLockDriverInquire: * @manager: the lock manager context @@ -328,6 +349,7 @@ struct _virLockDriver {

virLockDriverAcquire drvAcquire; virLockDriverRelease drvRelease; + virLockDriverCloseConn drvCloseConn; virLockDriverInquire drvInquire; };

diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 14f9eae760..aec768b0df 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -937,6 +937,28 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, }

+static int virLockManagerLockDaemonCloseConn(virLockManagerPtr lock, + unsigned int flags)

static int virLock...

+{ + virLockManagerLockDaemonPrivatePtr priv = lock->privateData; + + virCheckFlags(0, -1); + + if (priv->clientRefs) + return 0; + + virNetClientClose(priv->client); + virObjectUnref(priv->client); + virObjectUnref(priv->program); + + priv->client = NULL; + priv->program = NULL; + priv->counter = 0; + + return 1;

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:

In some cases we might want to not load the lock driver config. Alter virLockManagerPluginNew() and the lock drivers to cope with this fact.

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 ;-)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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(-)

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.

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/@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);

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@redhat.com> John

- 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;

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

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@redhat.com> --- 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; \

This I don't really understand - black magic, voodoo stuff ;-)

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")))

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.

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;

using @lock here is really confusing later on when I see lock->lock.

+ + bool pathLocked;

Unused for now...

+}; + 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); +

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?

VIR_FREE(mgr->privateData); }

+static void +virSecurityManagerLockDispose(void *obj) +{ + virSecurityManagerLockPtr lock = obj; + + virCondDestroy(&lock->cond); + + if (lock->lock) + virLockManagerCloseConn(lock->lock, 0);

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.

+ virLockManagerFree(lock->lock);

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.

+ 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; + }

The { } aren't necessary, but understandable.

+ + 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)))

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?

+ 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);

Cannot be NULL, true? Should we ATTRIBUTE_NONNULL it? John

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;

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

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@redhat.com> --- 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

milliseconds

+ * acquired before claiming error. */ +#define METADATA_LOCK_WAIT_MAX (10 * 1000) + +/* What is the maximum sleeping time (in miliseconds) between ^^^^^^^^^^^ consistent at least ;-)

+ * retries. */ +#define METADATA_LOCK_SLEEP_MAX (100)

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.

+ if (virCondWait(&lock->cond, &lock->parent.lock) < 0) {

virCondWaitUntil perhaps?

+ virReportSystemError(errno, "%s", + _("failed to wait on metadata condition")); + goto cleanup; + }

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

+ if (err->code == VIR_ERR_SYSTEM_ERROR && + err->int1 == EPIPE) {

Consider: virLastErrorIsSystemErrno

+ /* 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) {

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 ;-)

+ ret = 0; + cleanup:

Should this code grab/save the current error message if (ret < 0) so that nothing overwrites it in the subsequent calls?

+ if (lock->lock)

Coverity also notes that by checking lock->lock here

+ virLockManagerClearResources(lock->lock, 0); + if (ret < 0)

But not here...

+ virSecurityManagerLockCloseConnLocked(lock, false);

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); + ret = 0; + cleanup: + if (lock->lock) + 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!

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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)) {

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?

+ 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;

Coverity says, please initialize this to false since it's used in cleanup

+ 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)) {

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

+ 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; }

On 08/31/2018 07:35 PM, John Ferlan wrote:

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

...

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@redhat.com> --- 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; \

This I don't really understand - black magic, voodoo stuff ;-)

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.

...

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")))

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.

There is no way you can disable DAC driver. That one is always there. So at least one driver is always enabled.

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.

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.

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 ;-).

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.

Does migration present any strange problems? I guess it's not a true distributed locking mechanism, so perhaps not, but it is a concern.

No problems there in my testing.

...

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;

using @lock here is really confusing later on when I see lock->lock.

...

+ + bool pathLocked;

Unused for now...

...

+}; + struct _virSecurityManager { virObjectLockable parent;

virSecurityDriverPtr drv; unsigned int flags; const char *virtDriver; + + virSecurityManagerLockPtr lock;

so maybe s/lock/metadataLock/ here?

...

+ 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); +

So this is the opposite end (so to speak) of the virObjectRef in virSecurityManagerNewStack and virSecurityManagerStackAddNested, right?

Yes.

Once enough of these are called that then triggers the call to> virSecurityManagerLockDispose, right?

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.

...

VIR_FREE(mgr->privateData); }

+static void +virSecurityManagerLockDispose(void *obj) +{ + virSecurityManagerLockPtr lock = obj; + + virCondDestroy(&lock->cond); + + if (lock->lock) + virLockManagerCloseConn(lock->lock, 0);

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.

This is basically there to clean any stale connection. Any connection where our lock() is not paired with unlock() (should there be such case).

...

+ virLockManagerFree(lock->lock);

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.

Okay, I'll move it respective patches.

...

+ 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; + }

The { } aren't necessary, but understandable.

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.

...

+ + 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)))

You're in a world of hurt if @lockManagerName == NULL

Sure, but such call is broken virtually by definition :-)

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?

I rather segfault than proceed with false sense of safety.

...

+ 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);

Cannot be NULL, true? Should we ATTRIBUTE_NONNULL it?

Sure. Michal

On 08/31/2018 02:32 PM, John Ferlan wrote:

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

...

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@redhat.com> --- 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); + } +

How about a helper method that would do these?

I wonder now if the @priv should keep track of flags as well?

That way you could "always" use @priv to store the client, program, and counter and then use helper methods to determine at close whether @flags had the KEEP_OPEN or not instead of having KEEP_OPEN in various places.

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.

...

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)))

If "always" storing in @priv this alters to if !priv->client && !(priv->client ==...

...

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; + } +

If "always" using @priv means this isn't necessary as long as @flags is also stored.

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).

...

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;

So one failure this time causes previously stored successes to be thrown away?

Yes. The client socket is closed by code below. There's not much point in keeping stale pointer to what used to be connection.

Or am I reading too much into this? Why is clientRefs not auto-incremented here, but auto-decremented in Release?

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

This is where I'd think a helper would be able to know the "last" referenced was removed and thus be able to perform the close and free of resources.

Calling *PrivateFree and finding that there's extra clientRefs would/could mean something is programatically wrong, right?

Yes. We don't want to free client data at this point.

...

+ } + virNetClientClose(client); virObjectUnref(client); virObjectUnref(program);

Always storing in @priv, priv->flags, and using @rv I would think could easily be managed in a helper...

...

@@ -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--;

This decrements something from 0...

...

+ } + + 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; + } +

Again, I'm not clear why when this release fails we go off and clear out everything? Including things that may have been connected before? Certain a properly documented helper would solve this mystery for me.

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.

One that could manage things based on KEEP_OPEN, @priv, and @rv. I would assume the close open when KEEP_OPEN is true would be where the auto-decrement would occur and be checked to determine whether the subsequent closes and unref's happen.

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:

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

...

This is a new type of object that lock drivers can handle. Currently, it is supported by lockd driver only.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 ++ src/locking/lock_driver_lockd.c | 43 +++++++++++++++++++++++++++++++-------- src/locking/lock_driver_sanlock.c | 3 ++- 3 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index a9d2041c30..9be0abcfba 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -51,6 +51,8 @@ typedef enum { VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK = 0, /* A lease against an arbitrary resource */ VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE = 1, + /* The resource to be locked is a metadata */ + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA = 2, } virLockManagerResourceType;

typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 98953500b7..d7cb183d7a 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -557,6 +557,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, virLockManagerLockDaemonPrivatePtr priv = lock->privateData; char *newName = NULL; char *newLockspace = NULL; + int newFlags = 0; bool autoCreate = false; int ret = -1;

@@ -569,7 +570,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, switch (priv->type) { case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:

- switch (type) { + switch ((virLockManagerResourceType) type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (params || nparams) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -670,6 +671,8 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, goto cleanup;

} break; + + case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:

I'm still conflicted with Unknown and Unsupported.

...

default:

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).

...

virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d for domain lock object"), @@ -679,6 +682,29 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, break;

case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + switch ((virLockManagerResourceType) type) { + case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA: + if (params || nparams) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected parameters for metadata resource")); + goto cleanup; + } + if (VIR_STRDUP(newLockspace, "") < 0 || + VIR_STRDUP(newName, name) < 0) + goto cleanup; + newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA; + break; + + case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: + case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE:

Again Unknown and Unsupported...

...

+ default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown lock manager object type %d for daemon lock object"), + type); + goto cleanup; + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), @@ -686,19 +712,18 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, goto cleanup; }

+ if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED) + newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED; + + if (autoCreate)

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...

Okay. Michal

On 08/30/2018 10:40 PM, John Ferlan wrote:

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

...

We will want virtlockd to lock files on behalf of libvirtd and not qemu process, because it is libvirtd that needs an exclusive access not qemu. This requires new lock context.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 + src/locking/lock_driver_lockd.c | 110 +++++++++++++++++++++++++++++++------- src/locking/lock_driver_sanlock.c | 37 ++++++++----- 3 files changed, 117 insertions(+), 32 deletions(-)

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 ;-)

[...]

...

diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index ca825e6026..8ca0cf5426 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;

Something tells me it'd be better if @dom and @daemon were typedef'd types.

I don't see much benefit to that but I can do that change.

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.

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.

...

size_t nresources; virLockManagerLockDaemonResourcePtr resources; @@ -156,10 +167,24 @@ virLockManagerLockDaemonConnectionRegister(virLockManagerPtr lock,

[...]

...

@@ -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;

So what happens if "id" and/or "uri" are in params? For DOMAIN we ignore "uri", should that be done here (for both)?

We error out ..

...

+ } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected parameter %s for daemon object"), + params[i].key); + goto cleanup; + }

.. 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.

...

+ } + + 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; +

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).

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.

...

default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 39c2f94a76..fe422d3be6 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -513,21 +513,32 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,

priv->flags = flags;

- for (i = 0; i < nparams; i++) { - param = ¶ms[i]; + switch ((virLockManagerObjectType) type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: + for (i = 0; i < nparams; i++) { + param = ¶ms[i];

- if (STREQ(param->key, "uuid")) { - memcpy(priv->vm_uuid, param->value.uuid, 16); - } else if (STREQ(param->key, "name")) { - if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) - goto error; - } else if (STREQ(param->key, "pid")) { - priv->vm_pid = param->value.iv; - } else if (STREQ(param->key, "id")) { - priv->vm_id = param->value.ui; - } else if (STREQ(param->key, "uri")) { - priv->vm_uri = param->value.cstr; + if (STREQ(param->key, "uuid")) { + memcpy(priv->vm_uuid, param->value.uuid, 16); + } else if (STREQ(param->key, "name")) { + if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) + goto error; + } else if (STREQ(param->key, "pid")) { + priv->vm_pid = param->value.iv; + } else if (STREQ(param->key, "id")) { + priv->vm_id = param->value.ui; + } else if (STREQ(param->key, "uri")) { + priv->vm_uri = param->value.cstr;

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)

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:

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

...

This is basically just a wrapper over virLockManagerCloseConn() so that no connection is left open when it shouldn't be.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 75 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+)

diff --git a/src/security/security_manager.c b/src/security/security_manager.c index caaff1f703..2238c75a5c 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -91,6 +91,56 @@ virSecurityManagerLockDispose(void *obj) }

+static void +virSecurityManagerLockCloseConnLocked(virSecurityManagerLockPtr lock, + bool force) +{ + int rc; + + if (!lock) + return;

Not possible - if this is true you may just want to abort or let the following code fail miserably. The definition of the API is that it's called when @lock is locked!

...

+ + while (!force && + lock->pathLocked) {

No need to split && across 2 lines. Still waiting for @pathLocked = true ;-)

...

+ if (virCondWait(&lock->cond, &lock->parent.lock) < 0) { + VIR_WARN("Unable to wait on metadata condition"); + return; + } + } + + rc = virLockManagerCloseConn(lock->lock, 0);

still haven't filled in lock->lock either

...

+ if (rc < 0) + return; + if (rc > 0) + lock->lock = NULL; + + if (force) { + /* We've closed the connection. Wake up anybody who might be

s/the/our/

...

+ * waiting. */ + lock->pathLocked = false; + virCondSignal(&lock->cond); + } +} + + +static void +virSecurityManagerLockCloseConn(virSecurityManagerLockPtr lock) +{ + if (!lock) + return;

So this is lock->lock from a few patches ago.

...

+ + virObjectLock(lock); + + if (!lock->lock)

Still not seeing lock->lock, but maybe the next patch exposes that part.

...

+ goto cleanup; + + virSecurityManagerLockCloseConnLocked(lock, false); + + cleanup: + virObjectUnlock(lock); +} + +

I'm staring blankly at the rest of changes wondering, hmmmm... what am I missing.

Beyond that if the mgr->drv->*(mgr) isn't called, then should we call the CloseConn. I'm not seeing why it's called in the first place!

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:

On 08/30/2018 11:34 PM, John Ferlan wrote:

...

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

...

This is a new type of object that lock drivers can handle. Currently, it is supported by lockd driver only.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 ++ src/locking/lock_driver_lockd.c | 43 +++++++++++++++++++++++++++++++-------- src/locking/lock_driver_sanlock.c | 3 ++- 3 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index a9d2041c30..9be0abcfba 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -51,6 +51,8 @@ typedef enum { VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK = 0, /* A lease against an arbitrary resource */ VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE = 1, + /* The resource to be locked is a metadata */ + VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA = 2, } virLockManagerResourceType;

typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 98953500b7..d7cb183d7a 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -557,6 +557,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, virLockManagerLockDaemonPrivatePtr priv = lock->privateData; char *newName = NULL; char *newLockspace = NULL; + int newFlags = 0; bool autoCreate = false; int ret = -1;

@@ -569,7 +570,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, switch (priv->type) { case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:

- switch (type) { + switch ((virLockManagerResourceType) type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (params || nparams) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -670,6 +671,8 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, goto cleanup;

} break; + + case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:

I'm still conflicted with Unknown and Unsupported.

...

default:

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).

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

...

...

virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d for domain lock object"), @@ -679,6 +682,29 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, break;

case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + switch ((virLockManagerResourceType) type) { + case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA: + if (params || nparams) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected parameters for metadata resource")); + goto cleanup; + } + if (VIR_STRDUP(newLockspace, "") < 0 || + VIR_STRDUP(newName, name) < 0) + goto cleanup; + newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA; + break; + + case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: + case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE:

Again Unknown and Unsupported...

...

+ default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown lock manager object type %d for daemon lock object"), + type); + goto cleanup; + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), @@ -686,19 +712,18 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, goto cleanup; }

+ if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED) + newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED; + + if (autoCreate)

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...

Okay.

Michal

On 09/03/2018 11:13 AM, Michal Privoznik wrote:

On 08/31/2018 07:35 PM, John Ferlan wrote:

...

On 08/27/2018 04:08 AM, Michal Privoznik wrote:

...

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@redhat.com> --- 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; \

This I don't really understand - black magic, voodoo stuff ;-)

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.

Still black magic... essentially though you've added "locking" to safe list of tree's that security/* files can access.

...

...

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")))

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.

There is no way you can disable DAC driver. That one is always there. So at least one driver is always enabled.

...

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.

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.

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.

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.

Ah, yes, the subtlety of the core principle for the design... I'll try to keep this in mind for future adjustments/reviews.

...

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 ;-).

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.

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.

...

Does migration present any strange problems? I guess it's not a true distributed locking mechanism, so perhaps not, but it is a concern.

No problems there in my testing.

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.