[libvirt] [PATCH] qemuOpenFile: Don't force chown on NFS
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. --- 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 -- 1.7.8.5
On 04/11/2012 03: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. --- src/qemu/qemu_driver.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/11/2012 08:41 AM, Eric Blake wrote:
On 04/11/2012 03: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. --- src/qemu/qemu_driver.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
ACK.
My ACK may be premature. I think we need to worry about multiple scenarios - it is true that root-squash NFS won't allow a chown, but not all NFS are root-squash, so this may be breaking something. I'll let Laine chime in with a review, since he has historically had experience with testing multiple NFS setups. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
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 :-)
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
On 04/11/2012 05:17 AM, Michal Privoznik wrote: 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
On 12.04.2012 13:16, Laine Stump wrote:
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 :-)
Not at all. In fact, I find this useful as somebody else has tested it. Pushed. Thanks. Michal
participants (3)
-
Eric Blake -
Laine Stump -
Michal Privoznik