[libvirt] [PATCH v3 22/28] security_manager: Introduce metadata locking APIs
John Ferlan
jferlan at redhat.com
Fri Aug 31 19:30:02 UTC 2018
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 at 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__ */
>
More information about the libvir-list
mailing list