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@xxxxxxxxxx> > --- > 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); > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
