On 03/09/2016 12:38 PM, Cole Robinson wrote:
> file volume deletion, via virFileRemove, has some logic that depends
> on uid/gid owner of the path. This info is cached in the volume XML.
> We need to be sure we are using up to date uid/gid before attempting
> to delete the volume, otherwise we can hit a case like this:
>
> - test.img created with uid=root
> - VM starts up using test.img, owner changed to uid=qemu
> - test.img pool is refreshed, uid=qemu is cached in the volume XML
> - VM shuts down, volume owner changed to gid=root
> - vol-delete test.img thinks uid=qemu, virFileRemove does setuid(qemu),
> fails to delete test.img since it is actually uid=root
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1289327
> ---
> src/storage/storage_driver.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
Coincidentally I was thinking about this one today... Still not
convinced this is the only "root" (ahem) cause...
For the startup/stop path, I assume your reference is the path through
virSecurityDACSetOwnershipInternal from virSecurityDACSetOwnership and
virSecurityDACRestoreFileLabelInternal...
But what if the mode, owner, group are provided in the XML when the
volume is created:
# cat vol.xml
<volume type='file'>
<name>test.img</name>
<source>
</source>
<capacity unit='bytes'>10485760</capacity>
<allocation unit='bytes'>10485760</allocation>
<target>
<path>/home/vm-images/test.img</path>
<format type='raw'/>
<permissions>
<mode>0600</mode>
<owner>107</owner>
<group>107</group>
<label>system_u:object_r:unlabeled_t:s0</label>
</permissions>
</target>
</volume>
# virsh vol-create default vol.xml
Vol test.img created from vol.xml
# virsh vol-delete test.img default
error: Failed to delete vol test.img
error: cannot unlink file '/home/vm-images/test.img': Permission denied
#
Yes, I know the following two patches resolve this case... What's
interesting is the extents we've gone to on creation to set things up,
but only a few bread crumbs are left for deletion. The assumption at
deletion being that no one changed anything from creation, but we see
that's not true.
I've been under the assumption that the owner gets set/changed during
virStorageBackendCreateRaw, virStorageFileBackendFileCreate, or
virStorageBackendCreateExecCommand. The setting of the owner for the
CreateRaw path would occur because the flags had FORCE_OWNER and
FORCE_MODE set. The setting of the owner for the CreateExecCommand in
the non POOL_NETFS path was because they were supplied in the XML and
not taken as the default from the running of the command to create the
file (eg qemu-img) and taking the default file/directory ownership.
Since we run as root, this can all happen without issues.
Then after any of the create processing happens, we can refresh the pool
which calls virStorageBackendUpdateVolTargetInfoFD and changes the
volume uid, gid, mode based on what the lstat(path, &sb) has found...
This perhaps helps in the event
But when we get to delete, we have no idea how things could have been
originally created or perhaps (likely) updated, so we only check what we
can... If we really wanted to be elaborate - save a provided uid, gid,
mode at creation... That would help at deletion to determine if
something changed. Doesn't work well for existing files (daemon start,
restart, reload). We'd need to save volume xml's somewhere. Probably
far more effort...
OK - so a too long winded way to say - I don't think the following patch
matters as anything (including libvirt) could have changed the file's
ownership and/or protections, but not updated the volume XML. The
refresh updates the volume XML to match the file's protection and
furthermore only matters in the current virFileRemove if the change is
back to root:root and only because we compare vs. gete{uid|gid}().
John
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 1d96618..ded54c9 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -1801,6 +1801,16 @@ storageVolDelete(virStorageVolPtr obj,
> goto cleanup;
> }
>
> + /* Need to ensure we are using up to date uid/gid for deletion */
> + if (backend->refreshVol &&
> + backend->refreshVol(obj->conn, pool, vol) < 0) {
> + /* The file may have been removed behind libvirt's back. Don't
> + error here, let the deletion fail or succeed instead */
> + VIR_INFO("Failed to refresh volume before deletion: %s",
> + virGetLastErrorMessage());
> + virResetLastError();
> + }
> +
> if (storageVolDeleteInternal(obj, backend, pool, vol, flags, true) < 0)
> goto cleanup;
>
>