Re: [libvirt] [PATCH 1/3] storage: refresh volume before deletion
[...]
> 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}().
>
I don't follow this conclusion really... refreshVol syncs the XML/volume cache
with the current uid/gid/mode on disk, which is then passed down to
virFileRemove. If the cached uid is 'qemu' but the on disk uid is 'cole',
the
internal cache is synced, virFileRemove will setuid(cole) and successfully
remove the disk image. Without this change, libvirt would setuid(qemu) and
fail to remove the disk.
So I don't see how this patch is unneeded, or only works for root:root
I was considering the checks:
+ if (geteuid() != 0)
+ return false;
We're running as root...
+
+ /* uid/gid weren'd specified */
+ if ((uid == (uid_t) -1) && (gid == (gid_t) -1))
+ return false;
We've provided qemu:qemu...
+
+ /* already running as proper uid/gid */
+ if (uid == geteuid() && gid == getegid())
+ return false;
+
At this point geteuid() would return 0 (root)
Maybe I'm wrong... I suppose it cannot hurt, but without patch 3 we'd
go through the setuid path - I think. I could also be missing something
really obvious.
John
- Cole
> 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;
>>
>>