Re: [libvirt] [PATCH] qemuDomainSaveMemory: Don't enforce dynamicOwnership
On 07/02/2018 04:49 PM, John Ferlan wrote:
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.
Yes.
If the same scenario
was played out for core dump or managed save image, wouldn't the same
problem exist?
Ah, good catch. So managed save uses qemuDomainSaveMemory() which I'm
fixing here. But coredump uses doCoreDump() which needs the same treatment.
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.
It does, but with fallback UID:GID (which is currently whatever DAC
label domain has). But at the same time chown() to the fallback UID:GID
pair is enforced because dynamicOwnership is set. And it is actually the
chown() that fails.
Now, as absurd as it may sound, we don't need the memory image to be the
same UID:GID as the running domain. Qemu is given a FD to migrate to (at
which point perms are not checked anymore), so for instance even an
unprivileged qemu process can write into a file owned by root:root.
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.
Exactly.
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.
Ah, good point. I'll post a patch to remove it.
Michal