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 09/03/2018 11:13 AM, Michal Privoznik wrote:

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.

Remember the caveat - I was going patch by patch. I see a union like this I'm thinking something in the future would then be using a pointer from &x.t.dom or &x.t.daemon rather than typing the whole path.

...

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.

Ok - so I'm not opposed to the design decision of namespacing, only indicating it really didn't seem necessary. Perhaps "common" members between the structs can be common to the @priv, while "specific" members would be in a union like this. Of course, doing that leaves nothing specific for daemon, does it. Although come to think of it, wouldn't eventually the clientRefs, client, program, and counter be more typically associated with daemon instead of dom? IOW: Those are metadata lock concepts and not disk/lease concepts. Still it's a design decision and while I think it's perhaps a bit of overkill, there is some value vis-a-vis namespace and usage of the virLockManagerObjectType.

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

One would think that either this or the subsequent patch wouldn't allow a "domain" type lock to use "metadata", so it would fail because @lock->priv->type doesn't support that mixed usage.

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.

Yep... sounds like the distributed lock manager to me. I don't believe I was concerned about usage of @dom vs. @daemon. I just wasn't convinced common members to both unions need to be separated. Things specific to a lock type would be though.

...

...

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.

OK, that's fine. If someone does implement a sanlock or perhaps some new style in the future, then it'd be up to them to modify this code if necessary. Fair enough. John

...

...

+ } + + 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/27/2018 04:08 AM, Michal Privoznik wrote:

The fact whether domain has or hasn't RW disks is specific to

"or doesn't have"

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

This patch does a bit more than advertised...

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

From the aspect of @dom vs @daemon union, moving @hasRWDisks still has no bearing other than classifying it as a @dom type resource which is fine, don't get me wrong on this - I'm just trying to go one patch at a time here.

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

Hmm. Why wasn't this done in the previous patch? Not that I'm looking for more patches to review, but processing this adjustment could have been easier if the @type switch code is/was put into it's own helper before adding the switch for priv->type. Could mean going back to patch8 and before patch11 somewhere.

- /* 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:

Especially because of this. So we cannot acquire a daemon type lock yet, which is I supposed fine/expected, but that's perhaps more because it's not yet supported vs. unknown. John

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

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

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! John

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;

On 09/04/2018 07:53 AM, Michal Privoznik wrote:

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.

No need to apologize. The problem is complicated, tough to design, and presents some interesting challenges with the various interactions. Hopefully between your chat w/ mkletzan and this review you got feedback that was helpful. Going forward when there is some assumed knowledge, don't be shy in noting that in code comments. In thinking more about a path related transaction solution - I begin to wonder about interactions for/from iSCSI or LVM "devices" on a local system that in the long run resolve to the same file. Is the fnctl using one or the other path "hold true" and get managed somewhere underneath libvirt in such a way that we don't have to "know" or "check" that there's multiple paths the same thing. John

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:

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

Moving this here means the message could be printed before virSecurityDACTransactionAppend is called (success or failure). Besides if it fails, then we print this message, but it really didn't happen. So maybe this changes to "Preparing to set DAC user and group on ..." and the cut lines move and are copied to just before the chown and the chownCallback. That way you know that the chown was done and didn't have a intermediate failure. Or just before return 0 after the (rc < 0) checks move the "Setting..." message and change to "Set the..."

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

This one is fine, but one could change the removed lines to "Preparing to restore the DAC user and group on %s"... then if *Internal prints, then you know it happened; otherwise, something prevented it. John

return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); }

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

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.

relabeling

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 65 ++++++++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 21 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 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 */

^^ Does this need to be removed on the next patch?

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

Logic is OK, just harder to read this way.

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

^^ Still true? You've copied the lines/logic to virSecurityDACSetOwnership and virSecurityDACRestoreFileLabelInternal Things seem OK otherwise, I assume by this point parts of this series will need a new version, so I'll do the re-review then. John

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

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