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

We will want virtlockd to lock files on behalf of libvirtd and not qemu process, because it is libvirtd that needs an exclusive access not qemu. This requires new lock context. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver.h | 2 + src/locking/lock_driver_lockd.c | 110 +++++++++++++++++++++++++++++++------- src/locking/lock_driver_sanlock.c | 37 ++++++++----- 3 files changed, 117 insertions(+), 32 deletions(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 8b7cccc521..a9d2041c30 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -42,6 +42,8 @@ typedef enum { typedef enum { /* The managed object is a virtual guest domain */ VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN = 0, + /* The managed object is a daemon (e.g. libvirtd) */ + VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON = 1, } virLockManagerObjectType; typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index ca825e6026..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; size_t nresources; virLockManagerLockDaemonResourcePtr resources; @@ -156,10 +167,24 @@ virLockManagerLockDaemonConnectionRegister(virLockManagerPtr lock, memset(&args, 0, sizeof(args)); args.flags = 0; - memcpy(args.owner.uuid, priv->uuid, VIR_UUID_BUFLEN); - args.owner.name = priv->name; - args.owner.id = priv->id; - args.owner.pid = priv->pid; + + switch (priv->type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: + memcpy(args.owner.uuid, priv->t.dom.uuid, VIR_UUID_BUFLEN); + args.owner.name = priv->t.dom.name; + args.owner.id = priv->t.dom.id; + args.owner.pid = priv->t.dom.pid; + break; + + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + memcpy(args.owner.uuid, priv->t.daemon.uuid, VIR_UUID_BUFLEN); + args.owner.name = priv->t.daemon.name; + args.owner.pid = priv->t.daemon.pid; + break; + + default: + return -1; + } if (virNetClientProgramCall(program, client, @@ -391,7 +416,18 @@ virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv) } VIR_FREE(priv->resources); - VIR_FREE(priv->name); + switch (priv->type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: + VIR_FREE(priv->t.dom.name); + break; + + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + VIR_FREE(priv->t.daemon.name); + break; + + default: + break; + } VIR_FREE(priv); } @@ -420,46 +456,82 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, if (VIR_ALLOC(priv) < 0) return -1; - switch (type) { + priv->type = type; + + switch ((virLockManagerObjectType) type) { case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: for (i = 0; i < nparams; i++) { if (STREQ(params[i].key, "uuid")) { - memcpy(priv->uuid, params[i].value.uuid, VIR_UUID_BUFLEN); + memcpy(priv->t.dom.uuid, params[i].value.uuid, VIR_UUID_BUFLEN); } else if (STREQ(params[i].key, "name")) { - if (VIR_STRDUP(priv->name, params[i].value.str) < 0) + if (VIR_STRDUP(priv->t.dom.name, params[i].value.str) < 0) goto cleanup; } else if (STREQ(params[i].key, "id")) { - priv->id = params[i].value.iv; + priv->t.dom.id = params[i].value.iv; } else if (STREQ(params[i].key, "pid")) { - priv->pid = params[i].value.iv; + priv->t.dom.pid = params[i].value.iv; } else if (STREQ(params[i].key, "uri")) { /* ignored */ } else { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected parameter %s for object"), + _("Unexpected parameter %s for domain object"), params[i].key); goto cleanup; } } - if (priv->id == 0) { + if (priv->t.dom.id == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing ID parameter for domain object")); goto cleanup; } - if (priv->pid == 0) + if (priv->t.dom.pid == 0) VIR_DEBUG("Missing PID parameter for domain object"); - if (!priv->name) { + if (!priv->t.dom.name) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing name parameter for domain object")); goto cleanup; } - if (!virUUIDIsValid(priv->uuid)) { + if (!virUUIDIsValid(priv->t.dom.uuid)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing UUID parameter for domain object")); goto cleanup; } break; + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + for (i = 0; i < nparams; i++) { + if (STREQ(params[i].key, "uuid")) { + memcpy(priv->t.daemon.uuid, params[i].value.uuid, VIR_UUID_BUFLEN); + } else if (STREQ(params[i].key, "name")) { + if (VIR_STRDUP(priv->t.daemon.name, params[i].value.str) < 0) + goto cleanup; + } else if (STREQ(params[i].key, "pid")) { + priv->t.daemon.pid = params[i].value.iv; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected parameter %s for daemon object"), + params[i].key); + goto cleanup; + } + } + + if (!virUUIDIsValid(priv->t.daemon.uuid)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing UUID parameter for daemon object")); + goto cleanup; + } + if (!priv->t.daemon.name) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing name parameter for daemon object")); + goto cleanup; + } + if (priv->t.daemon.pid == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing PID parameter for daemon object")); + goto cleanup; + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 39c2f94a76..fe422d3be6 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -513,21 +513,32 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, priv->flags = flags; - for (i = 0; i < nparams; i++) { - param = &params[i]; + switch ((virLockManagerObjectType) type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: + for (i = 0; i < nparams; i++) { + param = &params[i]; - if (STREQ(param->key, "uuid")) { - memcpy(priv->vm_uuid, param->value.uuid, 16); - } else if (STREQ(param->key, "name")) { - if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) - goto error; - } else if (STREQ(param->key, "pid")) { - priv->vm_pid = param->value.iv; - } else if (STREQ(param->key, "id")) { - priv->vm_id = param->value.ui; - } else if (STREQ(param->key, "uri")) { - priv->vm_uri = param->value.cstr; + if (STREQ(param->key, "uuid")) { + memcpy(priv->vm_uuid, param->value.uuid, 16); + } else if (STREQ(param->key, "name")) { + if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) + goto error; + } else if (STREQ(param->key, "pid")) { + priv->vm_pid = param->value.iv; + } else if (STREQ(param->key, "id")) { + priv->vm_id = param->value.ui; + } else if (STREQ(param->key, "uri")) { + priv->vm_uri = param->value.cstr; + } } + break; + + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown lock manager object type %d"), + type); + goto error; } /* Sanlock needs process registration, but the only way how to probe -- 2.16.4

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

The fact whether domain has or hasn't RW disks is specific to VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN and therefore should reside in union specific to it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver_lockd.c | 187 +++++++++++++++++++++------------------- 1 file changed, 100 insertions(+), 87 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 8ca0cf5426..98953500b7 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -63,6 +63,8 @@ struct _virLockManagerLockDaemonPrivate { char *name; int id; pid_t pid; + + bool hasRWDisks; } dom; struct { @@ -74,8 +76,6 @@ struct _virLockManagerLockDaemonPrivate { size_t nresources; virLockManagerLockDaemonResourcePtr resources; - - bool hasRWDisks; }; @@ -566,107 +566,119 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0; - switch (type) { - case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: - if (params || nparams) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unexpected parameters for disk resource")); - goto cleanup; - } - if (!driver->autoDiskLease) { - if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | - VIR_LOCK_MANAGER_RESOURCE_READONLY))) - priv->hasRWDisks = true; - return 0; - } + switch (priv->type) { + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: - /* XXX we should somehow pass in TYPE=BLOCK info - * from the domain_lock code, instead of assuming /dev - */ - if (STRPREFIX(name, "/dev") && - driver->lvmLockSpaceDir) { - VIR_DEBUG("Trying to find an LVM UUID for %s", name); - if (virStorageFileGetLVMKey(name, &newName) < 0) + switch (type) { + case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: + if (params || nparams) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected parameters for disk resource")); goto cleanup; + } + if (!driver->autoDiskLease) { + if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_READONLY))) + priv->t.dom.hasRWDisks = true; + return 0; + } - if (newName) { - VIR_DEBUG("Got an LVM UUID %s for %s", newName, name); - if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0) + /* XXX we should somehow pass in TYPE=BLOCK info + * from the domain_lock code, instead of assuming /dev + */ + if (STRPREFIX(name, "/dev") && + driver->lvmLockSpaceDir) { + VIR_DEBUG("Trying to find an LVM UUID for %s", name); + if (virStorageFileGetLVMKey(name, &newName) < 0) goto cleanup; - autoCreate = true; - break; + + if (newName) { + VIR_DEBUG("Got an LVM UUID %s for %s", newName, name); + if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0) + goto cleanup; + autoCreate = true; + break; + } + virResetLastError(); + /* Fallback to generic non-block code */ } - virResetLastError(); - /* Fallback to generic non-block code */ - } - if (STRPREFIX(name, "/dev") && - driver->scsiLockSpaceDir) { - VIR_DEBUG("Trying to find an SCSI ID for %s", name); - if (virStorageFileGetSCSIKey(name, &newName) < 0) - goto cleanup; + if (STRPREFIX(name, "/dev") && + driver->scsiLockSpaceDir) { + VIR_DEBUG("Trying to find an SCSI ID for %s", name); + if (virStorageFileGetSCSIKey(name, &newName) < 0) + goto cleanup; + + if (newName) { + VIR_DEBUG("Got an SCSI ID %s for %s", newName, name); + if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0) + goto cleanup; + autoCreate = true; + break; + } + virResetLastError(); + /* Fallback to generic non-block code */ + } - if (newName) { - VIR_DEBUG("Got an SCSI ID %s for %s", newName, name); - if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0) + if (driver->fileLockSpaceDir) { + if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0) + goto cleanup; + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) goto cleanup; autoCreate = true; - break; + VIR_DEBUG("Using indirect lease %s for %s", newName, name); + } else { + if (VIR_STRDUP(newLockspace, "") < 0) + goto cleanup; + if (VIR_STRDUP(newName, name) < 0) + goto cleanup; + VIR_DEBUG("Using direct lease for %s", name); } - virResetLastError(); - /* Fallback to generic non-block code */ - } - if (driver->fileLockSpaceDir) { - if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0) - goto cleanup; - if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) + break; + case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: { + size_t i; + char *path = NULL; + char *lockspace = NULL; + for (i = 0; i < nparams; i++) { + if (STREQ(params[i].key, "offset")) { + if (params[i].value.ul != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Offset must be zero for this lock manager")); + goto cleanup; + } + } else if (STREQ(params[i].key, "lockspace")) { + lockspace = params[i].value.str; + } else if (STREQ(params[i].key, "path")) { + path = params[i].value.str; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected parameter %s for lease resource"), + params[i].key); + goto cleanup; + } + } + if (!path || !lockspace) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing path or lockspace for lease resource")); goto cleanup; - autoCreate = true; - VIR_DEBUG("Using indirect lease %s for %s", newName, name); - } else { - if (VIR_STRDUP(newLockspace, "") < 0) + } + if (virAsprintf(&newLockspace, "%s/%s", + path, lockspace) < 0) goto cleanup; if (VIR_STRDUP(newName, name) < 0) goto cleanup; - VIR_DEBUG("Using direct lease for %s", name); - } + } break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown lock manager object type %d for domain lock object"), + type); + goto cleanup; + } break; - case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: { - size_t i; - char *path = NULL; - char *lockspace = NULL; - for (i = 0; i < nparams; i++) { - if (STREQ(params[i].key, "offset")) { - if (params[i].value.ul != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Offset must be zero for this lock manager")); - goto cleanup; - } - } else if (STREQ(params[i].key, "lockspace")) { - lockspace = params[i].value.str; - } else if (STREQ(params[i].key, "path")) { - path = params[i].value.str; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected parameter %s for lease resource"), - params[i].key); - goto cleanup; - } - } - if (!path || !lockspace) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing path or lockspace for lease resource")); - goto cleanup; - } - if (virAsprintf(&newLockspace, "%s/%s", - path, lockspace) < 0) - goto cleanup; - if (VIR_STRDUP(newName, name) < 0) - goto cleanup; - } break; + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON: default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), @@ -711,8 +723,9 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY | VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1); - if (priv->nresources == 0 && - priv->hasRWDisks && + if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN && + priv->nresources == 0 && + priv->t.dom.hasRWDisks && driver->requireLeaseForDisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Read/write, exclusive access, disks were present, but no leases specified")); -- 2.16.4

On 08/30/2018 11:14 PM, John Ferlan wrote: Yes. Because of the 'namespacing' I described in reply to previous patch, hasRWDisks has to go into domain related union. IOW, hasRWDisks has no meaning for DAEMON type of lock. Okay, I'll move it there. Michal

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

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

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

On 08/27/2018 04:08 AM, Michal Privoznik wrote: 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. If "always" storing in @priv this alters to if !priv->client && !(priv->client ==... If "always" using @priv means this isn't necessary as long as @flags is also stored. So one failure this time causes previously stored successes to be thrown away? Or am I reading too much into this? Why is clientRefs not auto-incremented here, but auto-decremented in Release? 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? Always storing in @priv, priv->flags, and using @rv I would think could easily be managed in a helper... This decrements something from 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. 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. John

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 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(a)redhat.com&gt; --- 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. + * 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. + * + * 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) +{ + 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; +} + + static int virLockManagerLockDaemonInquire(virLockManagerPtr lock ATTRIBUTE_UNUSED, char **state, unsigned int flags) @@ -966,5 +988,7 @@ virLockDriver virLockDriverImpl = .drvAcquire = virLockManagerLockDaemonAcquire, .drvRelease = virLockManagerLockDaemonRelease, + .drvCloseConn = virLockManagerLockDaemonCloseConn, + .drvInquire = virLockManagerLockDaemonInquire, }; diff --git a/src/locking/lock_driver_nop.c b/src/locking/lock_driver_nop.c index 26b36061fb..52f78a4721 100644 --- a/src/locking/lock_driver_nop.c +++ b/src/locking/lock_driver_nop.c @@ -102,6 +102,12 @@ static int virLockManagerNopInquire(virLockManagerPtr lock ATTRIBUTE_UNUSED, return 0; } +static int virLockManagerLockNopCloseConn(virLockManagerPtr lock ATTRIBUTE_UNUSED, + unsigned int flags_unused ATTRIBUTE_UNUSED) +{ + return 1; +} + static void virLockManagerNopFree(virLockManagerPtr lock ATTRIBUTE_UNUSED) { } @@ -123,5 +129,7 @@ virLockDriver virLockDriverNop = .drvAcquire = virLockManagerNopAcquire, .drvRelease = virLockManagerNopRelease, + .drvCloseConn = virLockManagerLockNopCloseConn, + .drvInquire = virLockManagerNopInquire, }; diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index 292b142c14..30a0fd996e 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -382,6 +382,17 @@ int virLockManagerRelease(virLockManagerPtr lock, } +int virLockManagerCloseConn(virLockManagerPtr lock, + unsigned int flags) +{ + VIR_DEBUG("lock=%p flags=0x%x", lock, flags); + + CHECK_MANAGER(drvCloseConn, -1); + + return lock->driver->drvCloseConn(lock, flags); +} + + int virLockManagerInquire(virLockManagerPtr lock, char **state, unsigned int flags) diff --git a/src/locking/lock_manager.h b/src/locking/lock_manager.h index 8e0049ce0b..3a0ad12969 100644 --- a/src/locking/lock_manager.h +++ b/src/locking/lock_manager.h @@ -64,6 +64,10 @@ int virLockManagerAcquire(virLockManagerPtr manager, int virLockManagerRelease(virLockManagerPtr manager, char **state, unsigned int flags); + +int virLockManagerCloseConn(virLockManagerPtr lock, + unsigned int flags); + int virLockManagerInquire(virLockManagerPtr manager, char **state, unsigned int flags); -- 2.16.4

On 08/31/2018 03:26 PM, John Ferlan wrote: We have to have the flag for release too. Imagine the following scenario: ThreadA: conn = connect_to_virtlockd(); lock(conn, pathX); chown(pathX); /* context switch to threadB */ ThreadB: conn = connect_to_virtlockd(); /* This will actually reuse the connection */ lock(conn, pathY); chown(pathY); unlock(conn, pathY); close_connection(conn); /* context switch back to threaA */ ThreadA: unlock(conn, pathX); close_connection(conn); We certainly want the unlock() to NOT close the connection when called from threadB because that would trigger the killer code on virtlockd side (as we still have pathX locked from threadA). Anyway, this is internal implementation and can be changed anytime. If we find better solution (e.g. opening connection at virLockManagerLockDaemonNew() time). Actually, the whole file uses it the way I have it. I rather be consistent. Yeah, the clientRefs increment line got lost somewhere. I could swear I had it but maybe I just forgot to add it into the commit. Dunno. I'll do the helper. Michal

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

On 08/27/2018 04:08 AM, Michal Privoznik wrote: Just two? ;-) Should we state that the domain locking is independent of the metadata daemon locking? I know it's obvious to the author (and now the reviewer), but for the first time reader of the config file. And of course that leaves the question on the table for the consumer about what is meant by multiple daemons. As in, which daemons... That little factoid would be lost in the commit message. Assuming some beefed up text, Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John [...]

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

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(a)redhat.com&gt; --- 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; + + while (!force && + lock->pathLocked) { + if (virCondWait(&lock->cond, &lock->parent.lock) < 0) { + VIR_WARN("Unable to wait on metadata condition"); + return; + } + } + + rc = virLockManagerCloseConn(lock->lock, 0); + if (rc < 0) + return; + if (rc > 0) + lock->lock = NULL; + + if (force) { + /* We've closed the connection. Wake up anybody who might be + * waiting. */ + lock->pathLocked = false; + virCondSignal(&lock->cond); + } +} + + +static void +virSecurityManagerLockCloseConn(virSecurityManagerLockPtr lock) +{ + if (!lock) + return; + + virObjectLock(lock); + + if (!lock->lock) + goto cleanup; + + virSecurityManagerLockCloseConnLocked(lock, false); + + cleanup: + virObjectUnlock(lock); +} + + static int virSecurityManagerOnceInit(void) { @@ -334,6 +384,7 @@ virSecurityManagerTransactionStart(virSecurityManagerPtr mgr) virObjectLock(mgr); if (mgr->drv->transactionStart) ret = mgr->drv->transactionStart(mgr); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -362,6 +413,7 @@ virSecurityManagerTransactionCommit(virSecurityManagerPtr mgr, virObjectLock(mgr); if (mgr->drv->transactionCommit) ret = mgr->drv->transactionCommit(mgr, pid); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -379,6 +431,7 @@ virSecurityManagerTransactionAbort(virSecurityManagerPtr mgr) virObjectLock(mgr); if (mgr->drv->transactionAbort) mgr->drv->transactionAbort(mgr); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); } @@ -487,6 +540,7 @@ virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityDiskLabel(mgr, vm, disk); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -515,6 +569,7 @@ virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -532,6 +587,7 @@ virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityDaemonSocketLabel(mgr, vm); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -549,6 +605,7 @@ virSecurityManagerSetSocketLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecuritySocketLabel(mgr, vm); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -566,6 +623,7 @@ virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainClearSecuritySocketLabel(mgr, vm); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -595,6 +653,7 @@ virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityDiskLabel(mgr, vm, disk); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -623,6 +682,7 @@ virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, src); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -642,6 +702,7 @@ virSecurityManagerRestoreHostdevLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityHostdevLabel(mgr, vm, dev, vroot); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -661,6 +722,7 @@ virSecurityManagerSetHostdevLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityHostdevLabel(mgr, vm, dev, vroot); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -679,6 +741,7 @@ virSecurityManagerSetSavedStateLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSavedStateLabel(mgr, vm, savefile); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -696,6 +759,7 @@ virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainRestoreSavedStateLabel(mgr, vm, savefile); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -939,6 +1003,7 @@ virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, virObjectLock(mgr); ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path, chardevStdioLogd); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -959,6 +1024,7 @@ virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated, chardevStdioLogd); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -1140,6 +1206,7 @@ virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetPathLabel(mgr, vm, path, allowSubtree); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -1167,6 +1234,7 @@ virSecurityManagerSetMemoryLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityMemoryLabel(mgr, vm, mem); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -1195,6 +1263,7 @@ virSecurityManagerRestoreMemoryLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityMemoryLabel(mgr, vm, mem); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -1213,6 +1282,7 @@ virSecurityManagerSetInputLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityInputLabel(mgr, vm, input); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -1231,6 +1301,7 @@ virSecurityManagerRestoreInputLabel(virSecurityManagerPtr mgr, int ret; virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityInputLabel(mgr, vm, input); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -1251,6 +1322,7 @@ virSecurityManagerSetChardevLabel(virSecurityManagerPtr mgr, virObjectLock(mgr); ret = mgr->drv->domainSetSecurityChardevLabel(mgr, def, dev_source, chardevStdioLogd); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -1271,6 +1343,7 @@ virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr, virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityChardevLabel(mgr, def, dev_source, chardevStdioLogd); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; } @@ -1289,6 +1362,7 @@ virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr, if (mgr->drv->domainSetSecurityTPMLabels) { virObjectLock(mgr); ret = mgr->drv->domainSetSecurityTPMLabels(mgr, vm); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; @@ -1307,6 +1381,7 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr, if (mgr->drv->domainRestoreSecurityTPMLabels) { virObjectLock(mgr); ret = mgr->drv->domainRestoreSecurityTPMLabels(mgr, vm); + virSecurityManagerLockCloseConn(mgr->lock); virObjectUnlock(mgr); return ret; -- 2.16.4

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

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

This function is going call security manager APIs and therefore it needs pointer to it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b623dcf39..1a33386b84 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -626,12 +626,13 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, static int -virSecurityDACSetOwnership(virSecurityDACDataPtr priv, +virSecurityDACSetOwnership(virSecurityManagerPtr mgr, virStorageSourcePtr src, const char *path, uid_t uid, gid_t gid) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); struct stat sb; if (!path && src && src->path && @@ -731,7 +732,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, return -1; } - return virSecurityDACSetOwnership(priv, src, NULL, user, group); + return virSecurityDACSetOwnership(mgr, src, NULL, user, group); } @@ -847,7 +848,7 @@ virSecurityDACSetHostdevLabelHelper(const char *file, if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(priv, NULL, file, user, group); + return virSecurityDACSetOwnership(mgr, NULL, file, user, group); } @@ -1226,7 +1227,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, switch ((virDomainChrType)dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACSetOwnership(priv, NULL, + ret = virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, user, group); break; @@ -1236,10 +1237,10 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0) goto done; if (virFileExists(in) && virFileExists(out)) { - if (virSecurityDACSetOwnership(priv, NULL, in, user, group) < 0 || - virSecurityDACSetOwnership(priv, NULL, out, user, group) < 0) + if (virSecurityDACSetOwnership(mgr, NULL, in, user, group) < 0 || + virSecurityDACSetOwnership(mgr, NULL, out, user, group) < 0) goto done; - } else if (virSecurityDACSetOwnership(priv, NULL, + } else if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, user, group) < 0) { goto done; @@ -1249,7 +1250,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!dev_source->data.nix.listen) { - if (virSecurityDACSetOwnership(priv, NULL, + if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.nix.path, user, group) < 0) goto done; @@ -1433,7 +1434,7 @@ virSecurityDACSetGraphicsLabel(virSecurityManagerPtr mgr, if (gfx->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && gfx->data.spice.gl == VIR_TRISTATE_BOOL_YES && gfx->data.spice.rendernode) { - if (virSecurityDACSetOwnership(priv, NULL, + if (virSecurityDACSetOwnership(mgr, NULL, gfx->data.spice.rendernode, user, group) < 0) return -1; @@ -1477,7 +1478,7 @@ virSecurityDACSetInputLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - ret = virSecurityDACSetOwnership(priv, NULL, input->source.evdev, user, group); + ret = virSecurityDACSetOwnership(mgr, NULL, input->source.evdev, user, group); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1651,7 +1652,7 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - ret = virSecurityDACSetOwnership(priv, NULL, mem->nvdimmPath, user, group); + ret = virSecurityDACSetOwnership(mgr, NULL, mem->nvdimmPath, user, group); break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: @@ -1739,27 +1740,27 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, return -1; if (def->os.loader && def->os.loader->nvram && - virSecurityDACSetOwnership(priv, NULL, + virSecurityDACSetOwnership(mgr, NULL, def->os.loader->nvram, user, group) < 0) return -1; if (def->os.kernel && - virSecurityDACSetOwnership(priv, NULL, + virSecurityDACSetOwnership(mgr, NULL, def->os.kernel, user, group) < 0) return -1; if (def->os.initrd && - virSecurityDACSetOwnership(priv, NULL, + virSecurityDACSetOwnership(mgr, NULL, def->os.initrd, user, group) < 0) return -1; if (def->os.dtb && - virSecurityDACSetOwnership(priv, NULL, + virSecurityDACSetOwnership(mgr, NULL, def->os.dtb, user, group) < 0) return -1; if (def->os.slic_table && - virSecurityDACSetOwnership(priv, NULL, + virSecurityDACSetOwnership(mgr, NULL, def->os.slic_table, user, group) < 0) return -1; @@ -1782,7 +1783,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0) return -1; - return virSecurityDACSetOwnership(priv, NULL, savefile, user, group); + return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group); } @@ -2102,7 +2103,7 @@ virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(priv, NULL, path, user, group); + return virSecurityDACSetOwnership(mgr, NULL, path, user, group); } virSecurityDriver virSecurityDriverDAC = { -- 2.16.4

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

Firstly, the message that says we're setting uid:gid shouldn't be called from virSecurityDACSetOwnershipInternal() because virSecurityDACRestoreFileLabelInternal() is calling it too. Secondly, there are places between us reporting label restore and us actually doing it where we can quit. Don't say we're doing something until we are actually about to do it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 3d0c8d20cb..1be4ead21c 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -563,9 +563,6 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, else if (rc > 0) return 0; - VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", - NULLSTR(src ? src->path : path), (long)uid, (long)gid); - if (priv && src && priv->chownCallback) { rc = priv->chownCallback(src, uid, gid); /* here path is used only for error messages */ @@ -649,6 +646,9 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, return -1; } + 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); } @@ -663,9 +663,6 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, uid_t uid = 0; /* By default return to root:root */ gid_t gid = 0; - VIR_INFO("Restoring DAC user and group on '%s'", - NULLSTR(src ? src->path : path)); - if (!path && src && src->path && virStorageSourceIsLocalStorage(src)) path = src->path; @@ -678,6 +675,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, return 0; } + 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); } -- 2.16.4

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

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

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