On 04/11/2012 04:21 PM, Laine Stump wrote:
> ACK to the idea, but NACK to the exact placement of the fix.
On further examination (and actually doing a couple tests), I withdraw
my NACK on the placement. I had mixed up usage of qemuOpenFile and
virFileOpen in my memory - qemuOpenFile already hardly ever cares about
the ownership of a file it opens, and even when it does, it always wants
it to be owned by root (well, actually getuid()).
So, although putting the fix in the place I suggested does work, my
reasoning was flawed and your original position also works properly, as
well as keeping the logic for setting the FORCE_OWNER flag in one place
- ACK.
Sorry for the noise :-)
>
> On 04/11/2012 05:17 AM, Michal Privoznik wrote:
>> If dynamic_ownership is off and we are creating a file on NFS
>> we force chown. This will fail as chown/chmod are not supported
>> on NFS. However, with no dynamic_ownership we are not required
>> to do any chown.
> This statement isn't exactly correct - chown/chmod *are* supported on
> NFS, but won't work if the NFS share is mounted with root-squash,
> because only root can chown and all commands issued by root appear to
> the server as being from user "nobody".
>
> The fix also isn't correct. If dynamic_ownership is off, that means that
> we don't want the DAC security driver to dynamically chown files back
> and forth between uid 0 and uid "qemu_user". However, it's very possible
> that we would want to create a new file that is permanently owned by uid
> "qemu_user", and one way to do that is to create the file, then chown it
> (the other way, which is only used if root is unable to create the file,
> is to fork, setuid to the qemu_user, then open the file). (NB: here it
> is the mainline code that is chowning the file, *not* the DAC security
> driver, and it is a one-time change, not something dynamic that will
> later be reverted).
>
> If we want a file created that is owned by the qemu user, and that file
> is located on a *non*-root-squash NFS, the proper (only?) way to do this
> is to set the FORCE_OWNER flag when calling virFileOpenAs(). Its first
> attempt will be to open the file then (only if FORCE_OWNER is set!)
> chown it.
>
> With this fix in place, we are resetting the FORCE_OWNER flag from the
> very beginning, so the file will be created owned by uid 0, and chown
> will never be called, leaving us with a file that is inaccessible to a
> qemu process running with uid qemu_user. (fortunately, in the case of
> doing a domain save, that isn't a problem, since it's libvirt that needs
> to open the file, and it only passes an fd to qemu, but the result is
> still incorrect, and there may be other uses of qemuOpenFile() that
> aren't as forgiving).
>
> Instead, I think what should be done is to allow the first attempt at
> calling virFileOpenAs() without resetting the FORCE_OWNER flag. If it is
> successful, then we have a new file owned by qemu_user, and we're done.
> If it fails, *then* reset the FORCE_OWNER flag just before the 2nd call
> to virFileOpenAs (iff dynamic_ownership is false).
>
> I'm starting a build with an alternate patch now, and will try it out in
> an hour or so.
>
>> ---
>> src/qemu/qemu_driver.c | 10 ++++++++--
>> 1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index d9e35be..1b55eb1 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -2429,6 +2429,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags,
>> bool bypass_security = false;
>> unsigned int vfoflags = 0;
>> int fd = -1;
>> + int path_shared = virStorageFileIsSharedFS(path);
>> uid_t uid = getuid();
>> gid_t gid = getgid();
>>
>> @@ -2437,7 +2438,12 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags,
>> * in the failure case */
>> if (oflags & O_CREAT) {
>> need_unlink = true;
>> - vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
>> +
>> + /* Don't force chown on network-shared FS
>> + * as it is likely to fail. */
>> + if (path_shared <= 0 || driver->dynamicOwnership)
>> + vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
>> +
>> if (stat(path, &sb) == 0) {
>> is_reg = !!S_ISREG(sb.st_mode);
>> /* If the path is regular file which exists
>> @@ -2475,7 +2481,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags,
>> }
>>
>> /* On Linux we can also verify the FS-type of the directory. */
>> - switch (virStorageFileIsSharedFS(path)) {
>> + switch (path_shared) {
>> case 1:
>> /* it was on a network share, so we'll continue
>> * as outlined above
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list