[libvirt] [PATCH v3 27/28] security_dac: Move transaction handling up one level

[John Ferlan]

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