Re: [PATCH 1/3] storage: refresh volume before deletion

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[...]

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

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]