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@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);