On Wed, Jan 28, 2015 at 05:25:00PM -0500, John Ferlan wrote: Makes perfect sense, ACK.

On Wed, Jan 28, 2015 at 05:25:01PM -0500, John Ferlan wrote: We must definitely keep the job until we're done with changing things with the domain. Have I really messed this up so much? ACK whether you squash it in or not.

A gnulib change (commit id 'beae0bdc') causes ENOTCONN to be returned from recvfd which causes us to fall into the throwaway waitpid() call and return ENOTCONN to the caller, this then gets displayed during a 'virsh save' when using a root squashed NFS environment that's trying to save the file as something other than root:root. This patch will add the additional check for ENOTCONN to force the code into the waitpid loop looking for the actual status from the _exit()'d child fork. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virfile.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 5f56005..4024f3d 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2125,7 +2125,8 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, } while (fd < 0 && errno == EINTR); VIR_FORCE_CLOSE(pair[0]); /* NB: this preserves errno */ - if (fd < 0 && errno != EACCES) { + /* gnulib will return ENOTCONN in certain instances */ + if (fd < 0 && !(errno == EACCES || errno == ENOTCONN)) { ret = -errno; while (waitpid(pid, NULL, 0) == -1 && errno == EINTR); return ret; -- 2.1.0

Rather than have a dummy waitpid loop and return of the failure status from recvfd, adjust the logic to save the recvfd error & fd and then in priority order: if waitpid failed, use that if waitpid succeeded, but the child reported failure, use that regardless of recvfd status if waitpid succeeded and reported success, but recvfd failed with anything other than EACCES or ENOTCONN, use recvfd's result otherwise, waitpid and recvfd succeeded, return the fd NOTE: Original logic to retry the open and force owner mode was "documented" as only being attempted if we had already tried opening with the fork+setuid, but checked flags vs. VIR_FILE_OPEN_NOFORK which is counter to how we would get to that point. So that code was removed. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virfile.c | 67 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 4024f3d..cf6819f 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2034,6 +2034,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, { pid_t pid; int waitret, status, ret = 0; + int recvfd_errno; int fd = -1; int pair[2] = { -1, -1 }; gid_t *groups; @@ -2124,15 +2125,11 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, fd = recvfd(pair[0], 0); } while (fd < 0 && errno == EINTR); VIR_FORCE_CLOSE(pair[0]); /* NB: this preserves errno */ + recvfd_errno = errno; - /* gnulib will return ENOTCONN in certain instances */ - if (fd < 0 && !(errno == EACCES || errno == ENOTCONN)) { - ret = -errno; - while (waitpid(pid, NULL, 0) == -1 && errno == EINTR); - return ret; - } - - /* wait for child to complete, and retrieve its exit code */ + /* wait for child to complete, and retrieve its exit code + * if waitpid fails, use that status + */ while ((waitret = waitpid(pid, &status, 0)) == -1 && errno == EINTR); if (waitret == -1) { ret = -errno; @@ -2142,28 +2139,42 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, VIR_FORCE_CLOSE(fd); return ret; } - if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || - fd == -1) { - /* fall back to the simpler method, which works better in - * some cases */ + + /* + * if waitpid succeeded, but the child reported failure, use that + * regardless of recvfd status + */ + if (WIFEXITED(status) && WEXITSTATUS(status) != 0) { + ret = WEXITSTATUS(status); + virReportSystemError(ret, + _("child failed to create '%s'"), + path); VIR_FORCE_CLOSE(fd); - if (flags & VIR_FILE_OPEN_NOFORK) { - /* If we had already tried opening w/o fork+setuid and - * failed, no sense trying again. Just set return the - * original errno that we got at that time (by - * definition, always either EACCES or EPERM - EACCES - * is close enough). - */ - return -EACCES; - } - if ((fd = open(path, openflags, mode)) < 0) - return -errno; - ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags); - if (ret < 0) { - VIR_FORCE_CLOSE(fd); - return ret; - } + return -ret; + } + + /* if waitpid succeeded and reported success, but recvfd failed with + * anything other than EACCES or ENOTCONN, use recvfd's result + */ + if (WIFEXITED(status) && WEXITSTATUS(status) == 0 && fd < 0 && + !(recvfd_errno == EACCES || recvfd_errno == ENOTCONN)) { + virReportSystemError(recvfd_errno, + _("failed recvfd for child creating '%s'"), + path); + return -recvfd_errno; + } + + /* Some sort of abnormal child termination */ + if (!WIFEXITED(status) || fd == -1) { + VIR_FORCE_CLOSE(fd); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("child exited abnormally, failed to create '%s'"), + path); + return -EACCES; } + + /* otherwise, waitpid and recvfd succeeded, return the fd + */ return fd; } -- 2.1.0

If we're expecting to create a file somewhere and that fails for some reason during qemuOpenFileAs, then we unlink the path we're attempting to create leaving no way to determine what the "existing" privileges, protections, or labels are that caused the failure (open, change owner and group, change mode, etc) Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0174c87..89b54c8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2899,6 +2899,9 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, vfoflags |= VIR_FILE_OPEN_FORCE_OWNER; if (stat(path, &sb) == 0) { + /* It already exists, we don't want to delete it on error */ + need_unlink = false; + is_reg = !!S_ISREG(sb.st_mode); /* If the path is regular file which exists * already and dynamic_ownership is off, we don't -- 2.1.0

https://bugzilla.redhat.com/show_bug.cgi?id=1158034 In qemuOpenFileAs if we fall into the path where we'll be opening / creating the file using VIR_FILE_OPEN_FORK, we need to first unlink/delete the file we created in the first path; otherwise, the attempt by the child process to open as some specific user:group may fail because the file was already created using nfsnobody:nfsnobody. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 89b54c8..91fefa9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2954,6 +2954,15 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, goto error; } + /* If we created the file above, then we need to remove it; + * otherwise, the next attempt to create will fail. If the + * file had already existed before we got here, then we also + * don't want to delete it and allow the following to succeed + * or fail based on existing protections + */ + if (need_unlink) + unlink(path); + /* Retry creating the file as qemu user */ if ((fd = virFileOpenAs(path, oflags, -- 2.1.0

In the event we're falling into the code that tries to create the file in a forked environment (VIR_FILE_OPEN_FORK) we pass different mode bits, but those are never set because the virFileOpenForceOwnerMode has a check if the OPEN_FORCE_MODE bit is set before attempting to change the mode. Since this is a special case it seems reasonable to set u+rw,g+rw,o Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 91fefa9..651f84b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2965,6 +2965,9 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, /* Retry creating the file as qemu user */ + /* Since we're passing different modes... */ + vfoflags |= VIR_FILE_OPEN_FORCE_MODE; + if ((fd = virFileOpenAs(path, oflags, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, fallback_uid, fallback_gid, -- 2.1.0