[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