[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 22/28] security_manager: Introduce metadata locking APIs




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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]