[v2] qemuDomainSaveMemory: Don't enforce dynamicOwnership

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

Posted by Michal Privoznik 5 years, 9 months ago

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>
---

Diff to v1:
- Fix doCoreDump too (raised in review by John).

 src/qemu/qemu_driver.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9a35e04a85..86674e519b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3185,6 +3185,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
                      unsigned int flags,
                      qemuDomainAsyncJob asyncJob)
 {
+    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     bool needUnlink = false;
     int ret = -1;
     int fd = -1;
@@ -3202,9 +3203,10 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
             goto cleanup;
         }
     }
-    fd = qemuOpenFile(driver, vm, path,
-                      O_WRONLY | O_TRUNC | O_CREAT | directFlag,
-                      &needUnlink);
+
+    fd = qemuOpenFileAs(cfg->user, cfg->group, false, path,
+                        O_WRONLY | O_TRUNC | O_CREAT | directFlag,
+                        &needUnlink);
     if (fd < 0)
         goto cleanup;
 
@@ -3244,6 +3246,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
  cleanup:
     VIR_FORCE_CLOSE(fd);
     virFileWrapperFdFree(wrapperFd);
+    virObjectUnref(cfg);
 
     if (ret < 0 && needUnlink)
         unlink(path);
@@ -3793,9 +3796,9 @@ doCoreDump(virQEMUDriverPtr driver,
     /* Core dumps usually imply last-ditch analysis efforts are
      * desired, so we intentionally do not unlink even if a file was
      * created.  */
-    if ((fd = qemuOpenFile(driver, vm, path,
-                           O_CREAT | O_TRUNC | O_WRONLY | directFlag,
-                           NULL)) < 0)
+    if ((fd = qemuOpenFileAs(cfg->user, cfg->group, false, path,
+                             O_CREAT | O_TRUNC | O_WRONLY | directFlag,
+                             NULL)) < 0)
         goto cleanup;
 
     if (!(wrapperFd = virFileWrapperFdNew(&fd, path, flags)))
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Michal Privoznik 5 years, 9 months ago

On 07/09/2018 02:51 PM, Michal Privoznik wrote:
> 

Polite ping.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 5 years, 9 months ago


On 07/09/2018 08:51 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.
> 

for memory snapshot and core files, right?

> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> Diff to v1:
> - Fix doCoreDump too (raised in review by John).
> 
>  src/qemu/qemu_driver.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 

Strange - I had this marked as I replied to it, but obviously I didn't.
Wonder WTF happened ...

and the second qemuOpenFile in qemuDomainSaveMemory to touch up the
header (virQEMUSaveDataFinish) probably could use qemuOpenFileAs too
right?  although perhaps less important since the answer should be the
same, just the journey a little different.

Leaving just one consumer for qemuOpenFile and dynamic_ownership
manipulation.

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Michal Privoznik 5 years, 9 months ago

On 07/24/2018 10:40 PM, John Ferlan wrote:
> 
> 
> On 07/09/2018 08:51 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.
>>
> 
> for memory snapshot and core files, right?
> 
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>
>> Diff to v1:
>> - Fix doCoreDump too (raised in review by John).
>>
>>  src/qemu/qemu_driver.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
> 
> Strange - I had this marked as I replied to it, but obviously I didn't.
> Wonder WTF happened ...
> 
> and the second qemuOpenFile in qemuDomainSaveMemory to touch up the
> header (virQEMUSaveDataFinish) probably could use qemuOpenFileAs too
> right?  although perhaps less important since the answer should be the
> same, just the journey a little different.

Not really. The second time we're opening the file it exists already
(notice we are not passing O_CREAT flag). This means we will not touch
the owner of the file.

> 
> Leaving just one consumer for qemuOpenFile and dynamic_ownership
> manipulation.
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 

Pushed, thanks.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list