ACK to the idea, but NACK to the exact placement of the fix.

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