Re: [libvirt] [PATCH] qemuDomainSaveMemory: Don't enforce dynamicOwnership
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(a)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);