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

Re: [libvirt] [PATCH v3 04/13] security_manager: Rework metadata locking




On 10/17/18 5:06 AM, Michal Privoznik wrote:
> Trying to use virlockd to lock metadata turns out to be too big
> gun. Since we will always spawn a separate process for relabeling
> we are safe to use thread unsafe POSIX locks and take out
> virtlockd completely out of the picture.
> 
> Signed-off-by: Michal Privoznik <mprivozn redhat com>
> ---
>  src/security/security_dac.c     |  10 +-
>  src/security/security_manager.c | 225 +++++++++++++++++---------------
>  src/security/security_manager.h |  17 ++-
>  src/security/security_selinux.c |   9 +-
>  4 files changed, 139 insertions(+), 122 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 580d800bb1..ad2561a241 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -204,6 +204,7 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>                               void *opaque)
>  {
>      virSecurityDACChownListPtr list = opaque;
> +    virSecurityManagerMetadataLockStatePtr state;
>      const char **paths = NULL;
>      size_t npaths = 0;
>      size_t i;
> @@ -216,14 +217,10 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>      for (i = 0; i < list->nItems; i++) {
>          const char *p = list->items[i]->path;
>  
> -        if (!p ||
> -            virFileIsDir(p))
> -            continue;
> -
>          VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
>      }
>  
> -    if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0)
> +    if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths)))
>          goto cleanup;
>  
>      for (i = 0; i < list->nItems; i++) {
> @@ -246,8 +243,7 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>              break;
>      }
>  
> -    if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0)
> -        goto cleanup;
> +    virSecurityManagerMetadataUnlock(list->manager, &state);
>  
>      if (rv < 0)
>          goto cleanup;
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index c6c80e6165..b675f8ab46 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -21,6 +21,10 @@
>   */
>  #include <config.h>
>  
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +
>  #include "security_driver.h"
>  #include "security_stack.h"
>  #include "security_dac.h"
> @@ -30,14 +34,11 @@
>  #include "virlog.h"
>  #include "locking/lock_manager.h"
>  #include "virfile.h"
> -#include "virtime.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_SECURITY
>  
>  VIR_LOG_INIT("security.security_manager");
>  
> -virMutex lockManagerMutex = VIR_MUTEX_INITIALIZER;
> -
>  struct _virSecurityManager {
>      virObjectLockable parent;
>  
> @@ -47,10 +48,6 @@ struct _virSecurityManager {
>      void *privateData;
>  
>      virLockManagerPluginPtr lockPlugin;
> -    /* This is a FD that represents a connection to virtlockd so
> -     * that connection is kept open in between MetdataLock() and
> -     * MetadataUnlock() calls. */
> -    int clientfd;
>  };
>  
>  static virClassPtr virSecurityManagerClass;
> @@ -66,7 +63,6 @@ void virSecurityManagerDispose(void *obj)
>          mgr->drv->close(mgr);
>  
>      virObjectUnref(mgr->lockPlugin);
> -    VIR_FORCE_CLOSE(mgr->clientfd);
>  
>      VIR_FREE(mgr->privateData);
>  }
> @@ -119,7 +115,6 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
>      mgr->flags = flags;
>      mgr->virtDriver = virtDriver;
>      VIR_STEAL_PTR(mgr->privateData, privateData);
> -    mgr->clientfd = -1;
>  
>      if (drv->open(mgr) < 0)
>          goto error;
> @@ -1276,129 +1271,153 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
>  }
>  
>  
> -static virLockManagerPtr
> -virSecurityManagerNewLockManager(virSecurityManagerPtr mgr,
> -                                 const char * const *paths,
> -                                 size_t npaths)
> +struct _virSecurityManagerMetadataLockState {

LockPrivate?

When I first saw State I had a different thought.

I like this better than the previous model... Keeping track of fds is
like other models used. How do these locks handle restarts? Are the
locks free'd if the daemon dies?

> +    size_t nfds;
> +    int *fds;
> +};
> +
> +
> +static int
> +cmpstringp(const void *p1, const void *p2)
>  {
> -    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;
> -    size_t i;
> +    const char *s1 = *(char * const *) p1;
> +    const char *s2 = *(char * const *) p2;
>  
> -    if (virGetHostUUID(params[0].value.uuid) < 0)
> -        return NULL;
> +    if (!s1 && !s2)
> +        return 0;
>  
> -    if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(mgr->lockPlugin),
> -                                   VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON,
> -                                   ARRAY_CARDINALITY(params),
> -                                   params,
> -                                   flags)))
> -        return NULL;
> +    if (!s1 || !s2)
> +        return s2 ? -1 : 1;
>  
> -    for (i = 0; i < npaths; i++) {
> -        if (virLockManagerAddResource(lock,
> -                                      VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA,
> -                                      paths[i], 0, NULL, 0) < 0)
> -            goto error;
> -    }
> -
> -    return lock;
> - error:
> -    virLockManagerFree(lock);
> -    return NULL;
> +    /* from man 3 qsort */
> +    return strcmp(s1, s2);
>  }
>  
> +#define METADATA_OFFSET 1
> +#define METADATA_LEN 1
>  
> -/* How many seconds should we try to acquire the lock before
> - * giving up. */
> -#define LOCK_ACQUIRE_TIMEOUT 60
> -
> -int
> -virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
> -                               const char * const *paths,
> +/**
> + * virSecurityManagerMetadataLock:
> + * @mgr: security manager object
> + * @paths: paths to lock
> + * @npaths: number of items in @paths array
> + *
> + * Lock passed @paths for metadata change. The returned state
> + * should be passed to virSecurityManagerMetadataUnlock.
> + *
> + * NOTE: this function is not thread safe (because of usage of
> + * POSIX locks).
> + *
> + * Returns: state on success,
> + *          NULL on failure.
> + */
> +virSecurityManagerMetadataLockStatePtr
> +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> +                               const char **paths,
>                                 size_t npaths)
>  {
> -    virLockManagerPtr lock;
> -    virTimeBackOffVar timebackoff;
> -    int fd = -1;
> -    int rv = -1;
> -    int ret = -1;
> +    size_t i = 0;
> +    size_t nfds = 0;
> +    int *fds = NULL;
> +    virSecurityManagerMetadataLockStatePtr ret = NULL;
>  
> -    virMutexLock(&lockManagerMutex);
> +    if (VIR_ALLOC_N(fds, npaths) < 0)
> +        return NULL;
>  
> -    if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
> -        goto cleanup;
> +    /* Sort paths to lock in order to avoid deadlocks. */
> +    qsort(paths, npaths, sizeof(*paths), cmpstringp);

Shouldn't this be the job of the caller to know or set order to avoid
deadlocks?  IOW: Why is it important to sort here?  It's not clear
how/why it avoids deadlocks.

>  
> -    if (virTimeBackOffStart(&timebackoff, 1, LOCK_ACQUIRE_TIMEOUT * 1000) < 0)
> -        goto cleanup;
> -    while (virTimeBackOffWait(&timebackoff)) {
> -        rv = virLockManagerAcquire(lock, NULL,
> -                                   VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK,
> -                                   VIR_DOMAIN_LOCK_FAILURE_DEFAULT, &fd);
> +    for (i = 0; i < npaths; i++) {
> +        const char *p = paths[i];
> +        struct stat sb;
> +        int retries = 10 * 1000;
> +        int fd;
> +
> +        if (!p || stat(p, &sb) < 0)
> +            continue;
> +
> +        if (S_ISDIR(sb.st_mode)) {
> +            /* Directories can't be locked */
> +            continue;
> +        }
> +
> +        if ((fd = open(p, O_RDWR)) < 0) {

Is RDWR required for locking?

> +            if (S_ISSOCK(sb.st_mode)) {
> +                /* Sockets can be opened only if there exists the
> +                 * other side that listens. */
> +                continue;
> +            }
> +
> +            virReportSystemError(errno,
> +                                 _("unable to open %s"),
> +                                 p);
> +            goto cleanup;
> +        }
> +
> +        do {
> +            if (virFileLock(fd, false,
> +                            METADATA_OFFSET, METADATA_LEN, false) < 0) {

If lockd dies somewhere in the middle of this "transaction" - all those
fd's close and unlock, right? Mr doom and gloom thinking what could go
wrong.

> +                if (retries && (errno == EACCES || errno == EAGAIN)) {
> +                    /* File is locked. Try again. */
> +                    retries--;
> +                    usleep(1000);
> +                    continue;
> +                } else {
> +                    virReportSystemError(errno,
> +                                         _("unable to lock %s for metadata change"),
> +                                         p);
> +                    VIR_FORCE_CLOSE(fd);
> +                    goto cleanup;
> +                }
> +            }

I dunno, I liked the virTimeBackOffStart better... This 1 second wait
just seems like it could be a perf. bottleneck someday. I've looked at
the code for a while and probably just need more thinking time on it.

John

(curious about more words from Daniel on this).

>  
> -        if (rv >= 0)
>              break;
> +        } while (1);
>  
> -        if (virGetLastErrorCode() == VIR_ERR_RESOURCE_BUSY)
> -            continue;
> -
> -        goto cleanup;
> +        VIR_APPEND_ELEMENT_COPY_INPLACE(fds, nfds, fd);
>      }
>  
> -    if (rv < 0)
> +    if (VIR_ALLOC(ret) < 0)
>          goto cleanup;
>  
> -    mgr->clientfd = fd;
> -    fd = -1;
> +    VIR_STEAL_PTR(ret->fds, fds);
> +    ret->nfds = nfds;
> +    nfds = 0;
>  
> -    ret = 0;
>   cleanup:
> -    virLockManagerFree(lock);
> -    VIR_FORCE_CLOSE(fd);
> -    if (ret < 0)
> -        virMutexUnlock(&lockManagerMutex);
> +    for (i = nfds; i > 0; i--)
> +        VIR_FORCE_CLOSE(fds[i - 1]);
> +    VIR_FREE(fds);
>      return ret;
>  }
>  
>  
> -int
> -virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
> -                                 const char * const *paths,
> -                                 size_t npaths)
> +void
> +virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> +                                 virSecurityManagerMetadataLockStatePtr *state)
>  {
> -    virLockManagerPtr lock;
> -    int fd;
> -    int ret = -1;
> +    size_t i;
>  
> -    /* lockManagerMutex acquired from previous
> -     * virSecurityManagerMetadataLock() call. */
> +    if (!state)
> +        return;
>  
> -    fd = mgr->clientfd;
> -    mgr->clientfd = -1;
> +    for (i = 0; i < (*state)->nfds; i++) {
> +        char ebuf[1024];
> +        int fd = (*state)->fds[i];
>  
> -    if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
> -        goto cleanup;
> +        /* Technically, unlock is not needed because it will
> +         * happen on VIR_CLOSE() anyway. But let's play it nice. */
> +        if (virFileUnlock(fd, METADATA_OFFSET, METADATA_LEN) < 0) {
> +            VIR_WARN("Unable to unlock fd %d: %s",
> +                     fd, virStrerror(errno, ebuf, sizeof(ebuf)));
> +        }
>  
> -    if (virLockManagerRelease(lock, NULL, 0) < 0)
> -        goto cleanup;
> +        if (VIR_CLOSE(fd) < 0) {
> +            VIR_WARN("Unable to close fd %d: %s",
> +                     fd, virStrerror(errno, ebuf, sizeof(ebuf)));
> +        }
> +    }
>  
> -    ret = 0;
> - cleanup:
> -    virLockManagerFree(lock);
> -    VIR_FORCE_CLOSE(fd);
> -    virMutexUnlock(&lockManagerMutex);
> -    return ret;
> +    VIR_FREE((*state)->fds);
> +    VIR_FREE(*state);
>  }
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index 10ebe5cc29..fe8a1b4a24 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -199,11 +199,16 @@ int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr,
>  int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
>                                         virDomainDefPtr vm);
>  
> -int virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
> -                                   const char * const *paths,
> -                                   size_t npaths);
> -int virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
> -                                     const char * const *paths,
> -                                     size_t npaths);
> +typedef struct _virSecurityManagerMetadataLockState virSecurityManagerMetadataLockState;
> +typedef virSecurityManagerMetadataLockState *virSecurityManagerMetadataLockStatePtr;
> +
> +virSecurityManagerMetadataLockStatePtr
> +virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
> +                               const char **paths,
> +                               size_t npaths);
> +
> +void
> +virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
> +                                 virSecurityManagerMetadataLockStatePtr *state);
>  
>  #endif /* VIR_SECURITY_MANAGER_H__ */
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 0e24b9b3ca..17e24c6ac3 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -214,6 +214,7 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>                                   void *opaque)
>  {
>      virSecuritySELinuxContextListPtr list = opaque;
> +    virSecurityManagerMetadataLockStatePtr state;
>      bool privileged = virSecurityManagerGetPrivileged(list->manager);
>      const char **paths = NULL;
>      size_t npaths = 0;
> @@ -227,13 +228,10 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>      for (i = 0; i < list->nItems; i++) {
>          const char *p = list->items[i]->path;
>  
> -        if (virFileIsDir(p))
> -            continue;
> -
>          VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
>      }
>  
> -    if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0)
> +    if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths)))
>          goto cleanup;
>  
>      rv = 0;
> @@ -250,8 +248,7 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>          }
>      }
>  
> -    if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0)
> -        goto cleanup;
> +    virSecurityManagerMetadataUnlock(list->manager, &state);
>  
>      if (rv < 0)
>          goto cleanup;
> 


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