[libvirt] [PATCHv1.5 5/8] security: DAC: Plumb usage of chown callback

[John Ferlan]

On 07/22/2014 05:20 AM, Peter Krempa wrote:
> Use the callback to set disk and storage image labels by modifying the
> existing functions and adding wrappers to avoid refactoring a lot of the
> code.
> ---
>  src/security/security_dac.c | 89 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 69 insertions(+), 20 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 1fb0c86..989329f 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -230,23 +230,54 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
>  }
> 
>  static int
> -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> +virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
> +                                   virStorageSourcePtr src,
> +                                   const char *path,
> +                                   uid_t uid,
> +                                   gid_t gid)
>  {
> +    int rc;
> +    int chown_errno;
> +
>      VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> -             path, (long) uid, (long) gid);
> +             NULLSTR(src ? src->path : path), (long) uid, (long) gid);
> +
> +    if (priv && src && priv->chownCallback) {

This is the 'src' option and 'path' is NULL

> +        rc = priv->chownCallback(src, uid, gid);
> +
> +        /* on -2 returned an error was already reported */
> +        if (rc == -2)
> +            return -1;
> 
> -    if (chown(path, uid, gid) < 0) {
> +        /* on -1 only errno was set */
> +        chown_errno = errno;

Thus rc == -1, path == NULL

> +    } else {
>          struct stat sb;
> -        int chown_errno = errno;
> 
> -        if (stat(path, &sb) >= 0) {
> +        if (!path) {
> +            if (!src || !src->path)

Maybe a "if !src && !path" return 0 should be before the VIR_INFO...
Maybe with a VIR_DEBUG that'll help someone some day figure out why they
aren't getting what they were expecting. Just a thought...

> +                return 0;
> +
> +            if (!virStorageSourceIsLocalStorage(src))
> +                return 0;
> +
> +            path = src->path;

Reusing a passed parameter is where things get dicey for me.

> +        }
> +
> +        rc = chown(path, uid, gid);
> +        chown_errno = errno;
> +
> +        if (rc < 0 &&
> +            stat(path, &sb) >= 0) {
>              if (sb.st_uid == uid &&
>                  sb.st_gid == gid) {
>                  /* It's alright, there's nothing to change anyway. */
>                  return 0;
>              }
>          }
> +    }
> 
> +    if (rc < 0) {

I think we can get here with path == NULL and rc == -1, resulting in
subsequent usage of '%s' for path to have 'nil', right?

>          if (chown_errno == EOPNOTSUPP || chown_errno == EINVAL) {
>              VIR_INFO("Setting user and group to '%ld:%ld' on '%s' not "
>                       "supported by filesystem",
> @@ -270,13 +301,31 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
>      return 0;
>  }
> 
> +
>  static int
> -virSecurityDACRestoreSecurityFileLabel(const char *path)
> +virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> +{
> +    return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid);
> +}
> +
> +
> +static int
> +virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv,
> +                                               virStorageSourcePtr src,
> +                                               const char *path)
>  {
> -    VIR_INFO("Restoring DAC user and group on '%s'", path);
> +    VIR_INFO("Restoring DAC user and group on '%s'",
> +             NULLSTR(src ? src->path : path));
> 
>      /* XXX record previous ownership */
> -    return virSecurityDACSetOwnership(path, 0, 0);
> +    return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);

I know this just follows the previous code, but in the big picture - how
does this "play" in with future patches where fs & gluster will
seemingly go though this path?


ACK - in general with the 'path' issue above understood.  For this code,
I'm mostly curious.

John
> +}
> +
> +
> +static int
> +virSecurityDACRestoreSecurityFileLabel(const char *path)
> +{
> +    return virSecurityDACRestoreSecurityFileLabelInternal(NULL, NULL, path);
>  }
> 
> 
> @@ -294,10 +343,6 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
>      if (!priv->dynamicOwnership)
>          return 0;
> 
> -    /* XXX: Add support for gluster DAC permissions */
> -    if (!src->path || !virStorageSourceIsLocalStorage(src))
> -        return 0;
> -
>      secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
>      if (secdef && !secdef->relabel)
>          return 0;
> @@ -315,7 +360,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
>              return -1;
>      }
> 
> -    return virSecurityDACSetOwnership(src->path, user, group);
> +    return virSecurityDACSetOwnershipInternal(priv, src, NULL, user, group);
>  }
> 
> 
> @@ -349,9 +394,6 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
>      if (!priv->dynamicOwnership)
>          return 0;
> 
> -    if (!src->path || !virStorageSourceIsLocalStorage(src))
> -        return 0;
> -
>      /* Don't restore labels on readoly/shared disks, because other VMs may
>       * still be accessing these. Alternatively we could iterate over all
>       * running domains and try to figure out if it is in use, but this would
> @@ -373,9 +415,16 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
>       * ownership, because that kills access on the destination host which is
>       * sub-optimal for the guest VM's I/O attempts :-) */
>      if (migrated) {
> -        int rc = virFileIsSharedFS(src->path);
> -        if (rc < 0)
> -            return -1;
> +        int rc = 1;
> +
> +        if (virStorageSourceIsLocalStorage(src)) {
> +            if (!src->path)
> +                return 0;
> +
> +            if ((rc = virFileIsSharedFS(src->path)) < 0)
> +                return -1;
> +        }
> +
>          if (rc == 1) {
>              VIR_DEBUG("Skipping image label restore on %s because FS is shared",
>                        src->path);
> @@ -383,7 +432,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
>          }
>      }
> 
> -    return virSecurityDACRestoreSecurityFileLabel(src->path);
> +    return virSecurityDACRestoreSecurityFileLabelInternal(priv, src, NULL);
>  }
> 
>