[libvirt] [PATCH] qemuOpenFile: Don't force chown on NFS

[Laine Stump]

ACK to the idea, but NACK to the exact placement of the fix.

On 04/11/2012 05:17 AM, Michal Privoznik wrote:
> If dynamic_ownership is off and we are creating a file on NFS
> we force chown. This will fail as chown/chmod are not supported
> on NFS. However, with no dynamic_ownership we are not required
> to do any chown.

This statement isn't exactly correct - chown/chmod *are* supported on
NFS, but won't work if the NFS share is mounted with root-squash,
because only root can chown and all commands issued by root appear to
the server as being from user "nobody".

The fix also isn't correct. If dynamic_ownership is off, that means that
we don't want the DAC security driver to dynamically chown files back
and forth between uid 0 and uid "qemu_user". However, it's very possible
that we would want to create a new file that is permanently owned by uid
"qemu_user", and one way to do that is to create the file, then chown it
(the other way, which is only used if root is unable to create the file,
is to fork, setuid to the qemu_user, then open the file). (NB: here it
is the mainline code that is chowning the file, *not* the DAC security
driver, and it is a one-time change, not something dynamic that will
later be reverted).

If we want a file created that is owned by the qemu user, and that file
is located on a *non*-root-squash NFS, the proper (only?) way to do this
is to set the FORCE_OWNER flag when calling virFileOpenAs(). Its first
attempt will be to open the file then (only if FORCE_OWNER is set!)
chown it.

With this fix in place, we are resetting the FORCE_OWNER flag from the
very beginning, so the file will be created owned by uid 0, and chown
will never be called, leaving us with a file that is inaccessible to a
qemu process running with uid qemu_user. (fortunately, in the case of
doing a domain save, that isn't a problem, since it's libvirt that needs
to open the file, and it only passes an fd to qemu, but the result is
still incorrect, and there may be other uses of qemuOpenFile() that
aren't as forgiving).

Instead, I think what should be done is to allow the first attempt at
calling virFileOpenAs() without resetting the FORCE_OWNER flag. If it is
successful, then we have a new file owned by qemu_user, and we're done.
If it fails, *then* reset the FORCE_OWNER flag just before the 2nd call
to virFileOpenAs (iff dynamic_ownership is false).

I'm starting a build with an alternate patch now, and will try it out in
an hour or so.

> ---
>  src/qemu/qemu_driver.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d9e35be..1b55eb1 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2429,6 +2429,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags,
>      bool bypass_security = false;
>      unsigned int vfoflags = 0;
>      int fd = -1;
> +    int path_shared = virStorageFileIsSharedFS(path);
>      uid_t uid = getuid();
>      gid_t gid = getgid();
>  
> @@ -2437,7 +2438,12 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags,
>       * in the failure case */
>      if (oflags & O_CREAT) {
>          need_unlink = true;
> -        vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
> +
> +        /* Don't force chown on network-shared FS
> +         * as it is likely to fail. */
> +        if (path_shared <= 0 || driver->dynamicOwnership)
> +            vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
> +
>          if (stat(path, &sb) == 0) {
>              is_reg = !!S_ISREG(sb.st_mode);
>              /* If the path is regular file which exists
> @@ -2475,7 +2481,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags,
>              }
>  
>              /* On Linux we can also verify the FS-type of the directory. */
> -            switch (virStorageFileIsSharedFS(path)) {
> +            switch (path_shared) {
>                  case 1:
>                     /* it was on a network share, so we'll continue
>                      * as outlined above