[libvirt] [PATCH] qemuDomainSaveMemory: Don't enforce dynamicOwnership

John Ferlan jferlan at redhat.com
Mon Jul 2 14:49:26 UTC 2018



On 06/26/2018 09:57 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1589115
> 
> When doing a memory snapshot qemuOpenFile() is used. This means
> that the file where memory is saved is firstly attempted to be
> created under root:root (because that's what libvirtd is running
> under) and if this fails the second attempt is done under
> domain's uid:gid. This does not make much sense - qemu is given
> opened FD so it does not need to access the file. Moreover, if
> dynamicOwnership is set in qemu.conf and the file lives on a
> squashed NFS this is deadly combination and very likely to fail.
> 
> The fix consists of using:
> 
>   qemuOpenFileAs(fallback_uid = cfg->user,
>                  fallback_gid = cfg->group,
>                  dynamicOwnership = false)
> 
> In other words, dynamicOwnership is turned off for memory
> snapshot (chown() will still be attempted if the file does not
> live on NFS) and instead of using domain DAC label, configured
> user:group is set as fallback.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_driver.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 

I see from reading the bz in a way you hoped to provoke upstream
discussion on this, so...

It seems the primary motivation is to go with dynamicOwnership = false
is because that "fixes" the bz for the "corner case" where someone is
saving their snapshot to a root squashed NFS share. If the same scenario
was played out for core dump or managed save image, wouldn't the same
problem exist?  Although the latter would skip the O_CREAT check in
qemuOpenFlagsAs thus not setting VIR_FILE_OPEN_FORCE_OWNER, but could
perhaps theoretically fail as IIRC the root squash case only had one way
to resolve.

Since @path_shared == 1 (only way to get to the second attempt to
virFileOpenAs), then why doesn't this code try the VIR_FILE_OPEN_FORK?
It's been a while, but IIRC that's what allowed storageBackendCreateRaw
to be successful.

IIUC the proposed solution to clear dynamicOwnership is purely to avoid
setting VIR_FILE_OPEN_FORCE_OWNER since @path_shared == 1 in the O_CREAT
path.

On a secondary note, this is the only path that doesn't pass NULL for
bypassSecuritydriver... If you follow that bypass - all that really
happens is it can alter the returned pointer, but that value is never
checked in the code - so what is it's purpose?  Taking a quick look
through history, I wonder if 23087cfdb was the last real consumer.

John

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 129bacdd34..6af7e6e171 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3223,6 +3223,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>                       unsigned int flags,
>                       qemuDomainAsyncJob asyncJob)
>  {
> +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      bool bypassSecurityDriver = false;
>      bool needUnlink = false;
>      int ret = -1;
> @@ -3241,9 +3242,10 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>              goto cleanup;
>          }
>      }
> -    fd = qemuOpenFile(driver, vm, path,
> -                      O_WRONLY | O_TRUNC | O_CREAT | directFlag,
> -                      &needUnlink, &bypassSecurityDriver);
> +
> +    fd = qemuOpenFileAs(cfg->user, cfg->group, false, path,
> +                        O_WRONLY | O_TRUNC | O_CREAT | directFlag,
> +                        &needUnlink, &bypassSecurityDriver);
>      if (fd < 0)
>          goto cleanup;
>  
> @@ -3283,6 +3285,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>   cleanup:
>      VIR_FORCE_CLOSE(fd);
>      virFileWrapperFdFree(wrapperFd);
> +    virObjectUnref(cfg);
>  
>      if (ret < 0 && needUnlink)
>          unlink(path);
> 




More information about the libvir-list mailing list