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

On 03/09/2016 12:39 PM, Cole Robinson wrote: > NFS with root-squash is the only reason we need to do setuid/setgid > crazyness in virFileRemove, so limit that behavior to the NFS case. > --- > I'm not sure though if NFS is the only case we care about this here, > or if we want to conditionalize this path on NFS since that makes it > more of a pain to test... It's not required to fix the initial bug > > src/util/virfile.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/src/util/virfile.c b/src/util/virfile.c > index cea2674..3d1b118 100644 > --- a/src/util/virfile.c > +++ b/src/util/virfile.c > @@ -2322,7 +2322,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, > * owned by the passed uid/gid pair. Needed for NFS with root-squash > */ > static bool > -virFileRemoveNeedsSetuid(uid_t uid, gid_t gid) > +virFileRemoveNeedsSetuid(const char *path, uid_t uid, gid_t gid) You added a new parameter to document... ACK with that adjustment.