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

Re: [libvirt] [PATCH] qemuOpenFile: Don't force chown on NFS



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 redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list
>


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