[libvirt] [PATCH] virfile: properly detect NFS storage

[Cole Robinson]

On 02/24/2016 09:31 AM, Pavel Hrdina wrote:
> Commit 35847860 introduced virFileUnlink() to fix an issue with deleting
> volumes on NFS root-squashed environment.  This patch replace the uid
> and gid magic by virFileIsSharedFSType() to correctly detect that the
> volume is on NFS storage.
> 
> This fixes the referenced bug.  To reproduce this bug follow those
> steps on a domain with local storage:
> 
>   virsh start $domain
>   virsh pool-refresh $pool
>   virsh destroy $domain
>   virsh vol-delete $volume $pool
> 
> The thing is, that the pool-refresh will store qemu:qemu as uid:gid for
> that volume and after destroy the volume is relabeled back to root:root.
> Then you run vol-delete and the virFileRemove() function will try to
> unlink the file as a qemu:qemu process based on the uid and gid magic.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1260356
> 

Good catch! Sounds like this fedora bug:

https://bugzilla.redhat.com/show_bug.cgi?id=1289327

And this RHEL bug that jferlan has been looking at:

https://bugzilla.redhat.com/show_bug.cgi?id=1293804

> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/util/virfile.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f45e18f..f9c5bb1 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -2334,13 +2334,16 @@ virFileRemove(const char *path,
>      int status = 0, ret = 0;
>      gid_t *groups;
>      int ngroups;
> +    int rc;
>  
>      /* If not running as root or if a non explicit uid/gid was being used for
>       * the file/volume or the explicit uid/gid matches, then use unlink directly
>       */
> -    if ((geteuid() != 0) ||
> -        ((uid == (uid_t) -1) && (gid == (gid_t) -1)) ||
> -        (uid == geteuid() && gid == getegid())) {
> +    rc = virFileIsSharedFSType(path, VIR_FILE_SHFS_NFS);
> +    if (rc < 0)
> +        return -EINVAL;
> +
> +    if (rc == 0) {
>          if (virFileIsDir(path))
>              return rmdir(path);
>          else
> 

The original code is trying to check for two cases:

- The daemon is unprivileged. In that case we don't want to try any of the
setuid stuff because we know it's not going to work.
- The file is owned by someone other than the daemon's uid/gid. If so, we want
to do the setuid stuff to try and make NFS root squash work.

This change discards the first check, which we want to preserve.

Regarding the second check, is NFS the only reason it exists? Maybe John or
Laine know more.

Thanks,
Cole