[libvirt] [PATCH 0/7] Resolve issues saving domain to NFS root squashed client
Lots of details here: https://bugzilla.redhat.com/show_bug.cgi?id=1158034 During my investigation I uncovered some other "issues" which are covered by the first 4 patches. The issues are error path related. The first two patches could be combined, but for now they're apart so that it's proveable that they resolve a single issue. Hopefully mkletzan can take a look at these to ensure they fit with the "spirit" of the changes they're fixing. With these fixes in place the error from virfile through qemu will be displayed... The third and fourth patches can be combined. Realistically only the third one is necessary as it resolves an issue introduced by the ref'd gnulib change (eblake has already read about it). The fourth takes eblake's suggestion to me in order to take the next step to avoid the throw-away waitpid loop in virFileOpenForked and replace it by more inline logic. Since this is code introduced by laine, I'm hoping he'll be able to take a look-see. It's not required for the series, but nice to have. The third patch allows us to be able to get the "correct" EACCES error back from the vfork'd child, while the forth tries to make it clearer. The fifth and sixth patches resolve the problem from the above ref'd bz. Essentially what's happening is that when there's a root squashed nfs client to which the 'virsh save $dom $file' is being saved, the attempt by the second virOpenFileAs to open or chmod the $file can fail because the file generated by the first call may not be able to be opened, written, or chown'd due to whatever was set by that first call. The fifth patch is there because I found that if a 'save' file existed initially and a 'virsh save $dom $file' failed - the $file was deleted (and of course the subsequent 'virsh save $dom $file' would succeed). Since we didn't create the file, we shouldn't delete it The seventh patch isn't required, but I was hoping it might generate some discussion as to whether it's the right thing to do. If we don't set the VIR_FILE_OPEN_FORCE_MODE flag on the virFileOpenAs call, then the subsequent virFileOpenForceOwnerMode will not change the protection bits. So why pass something different if the end result is not change? I looked at other virOpenFileAs paths that are creating and found only one where the bit was set in virStorageBackendCreateRaw. Perhaps this patch generates other patches to "really" save mode bits when CREAT is being used. An eight patch was considered, but I dropped it. I was going to add an S_IROTH to the bits being passed on that nfs root squash path. This was being considered because the above ref'd bz uses an nfs pool in order to save the file into. That pool used different protections on files when first created and once/if a pool-refresh or libvirtd restart/ reload is done - the pool will fail to start because of the save file's protections. Setting other+read "resolved" or "work-around" that, but I don't really think that's the right thing to do. Altough, it is NFS and well not necessarily secure. John Ferlan (7): qemu: Save/restore error in error path for qemuDomainSaveInternal qemu: Adjust EndAsyncJob for qemuDomainSaveInternal error path virfile: Need to check for ENOTCONN from recvfd failure virfile: Adjust error path for virFileOpenForked qemu: Don't unconditionally delete file in qemuOpenFileAs qemu: remove failed create prior to root squash share path qemu: qemuOpenFileAs - set flag VIR_FILE_OPEN_FORCE_MODE src/qemu/qemu_driver.c | 24 +++++++++++++++--- src/util/virfile.c | 66 +++++++++++++++++++++++++++++--------------------- 2 files changed, 60 insertions(+), 30 deletions(-) -- 2.1.0
If we get to endjob: because of some error earlier such as perhaps failure from qemuDomainSaveMemory to open/create the save file and the attempt to restart the CPU's fails, then we get the error from that restart attempt displayed to the user rather than the error from the failed attempt to create a save file. Upstream commit id '540c339a' changed the flow of the code moving the EndAsyncJob call and thus exposing the issue where an error in restarting CPUs resulted in the following: error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: internal error: unexpected async job 3 where /tmp/pl is a NFS root squashed client where the failure to save the file (EPERM or ENOTCONN) should result in a failure: error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: Error from child process creating '/tmp/pl/rhel70.save': Transport endpoint is not connected or (REVISIT ORDER - that is if I move the virfile check for ENOTCONN to sooner, then we'll never see ENOTCONN) error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: Error from child process creating '/tmp/pl/rhel70.save': Operation not permitted Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 1d3bee6..190d472 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3211,6 +3211,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, qemuDomainObjEndAsyncJob(driver, vm); if (ret != 0) { if (was_running && virDomainObjIsActive(vm)) { + virErrorPtr save_err = virSaveLastError(); rc = qemuProcessStartCPUs(driver, vm, dom->conn, VIR_DOMAIN_RUNNING_SAVE_CANCELED, QEMU_ASYNC_JOB_SAVE); @@ -3220,6 +3221,8 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); } + virSetError(save_err); + virFreeError(save_err); } } else if (!vm->persistent) { qemuDomainRemoveInactive(driver, vm); -- 2.1.0
On Wed, Jan 28, 2015 at 05:25:00PM -0500, John Ferlan wrote:
If we get to endjob: because of some error earlier such as perhaps failure from qemuDomainSaveMemory to open/create the save file and the attempt to restart the CPU's fails, then we get the error from that restart attempt displayed to the user rather than the error from the failed attempt to create a save file.
Upstream commit id '540c339a' changed the flow of the code moving the EndAsyncJob call and thus exposing the issue where an error in restarting CPUs resulted in the following:
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: internal error: unexpected async job 3
where /tmp/pl is a NFS root squashed client where the failure to save the file (EPERM or ENOTCONN) should result in a failure:
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: Error from child process creating '/tmp/pl/rhel70.save': Transport endpoint is not connected
or (REVISIT ORDER - that is if I move the virfile check for ENOTCONN to sooner, then we'll never see ENOTCONN)
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: Error from child process creating '/tmp/pl/rhel70.save': Operation not permitted Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 1d3bee6..190d472 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3211,6 +3211,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, qemuDomainObjEndAsyncJob(driver, vm); if (ret != 0) { if (was_running && virDomainObjIsActive(vm)) { + virErrorPtr save_err = virSaveLastError(); rc = qemuProcessStartCPUs(driver, vm, dom->conn, VIR_DOMAIN_RUNNING_SAVE_CANCELED, QEMU_ASYNC_JOB_SAVE); @@ -3220,6 +3221,8 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); } + virSetError(save_err); + virFreeError(save_err); } } else if (!vm->persistent) { qemuDomainRemoveInactive(driver, vm); -- 2.1.0
Makes perfect sense, ACK.
On 01/28/2015 03:25 PM, John Ferlan wrote:
If we get to endjob: because of some error earlier such as perhaps failure from qemuDomainSaveMemory to open/create the save file and the attempt to restart the CPU's fails, then we get the error from that restart attempt displayed to the user rather than the error from the failed attempt to create a save file.
Upstream commit id '540c339a' changed the flow of the code moving the EndAsyncJob call and thus exposing the issue where an error in restarting CPUs resulted in the following:
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: internal error: unexpected async job 3
where /tmp/pl is a NFS root squashed client where the failure to save the file (EPERM or ENOTCONN) should result in a failure:
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: Error from child process creating '/tmp/pl/rhel70.save': Transport endpoint is not connected
or (REVISIT ORDER - that is if I move the virfile check for ENOTCONN to sooner, then we'll never see ENOTCONN)
Is this a note about the possibility of applying the series in a different order?
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: Error from child process creating '/tmp/pl/rhel70.save': Operation not permitted Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/29/2015 03:13 PM, Eric Blake wrote:
On 01/28/2015 03:25 PM, John Ferlan wrote:
If we get to endjob: because of some error earlier such as perhaps failure from qemuDomainSaveMemory to open/create the save file and the attempt to restart the CPU's fails, then we get the error from that restart attempt displayed to the user rather than the error from the failed attempt to create a save file.
Upstream commit id '540c339a' changed the flow of the code moving the EndAsyncJob call and thus exposing the issue where an error in restarting CPUs resulted in the following:
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: internal error: unexpected async job 3
where /tmp/pl is a NFS root squashed client where the failure to save the file (EPERM or ENOTCONN) should result in a failure:
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: Error from child process creating '/tmp/pl/rhel70.save': Transport endpoint is not connected
or (REVISIT ORDER - that is if I move the virfile check for ENOTCONN to sooner, then we'll never see ENOTCONN)
Is this a note about the possibility of applying the series in a different order?
Yeah - it was... But when I combined 1&2 after mkletzan's ACK - I removed it... John
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: Error from child process creating '/tmp/pl/rhel70.save': Operation not permitted Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+)
ACK
Commit id '540c339a' to fix issues with reference counting and transient domains moved the qemuDomainObjEndAsyncJob call prior to the attempt to restart the guest CPU's resulting in an error: error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: internal error: unexpected async job 3 when (ret != 0) - eg, the error path from qemuDomainSaveMemory. This patch will adjust the logic to only call the EndAsyncJob after we've tried to restart the guest CPUs. It also needs to adjust the test for qemuDomainRemoveInactive to add the ret == 0 condition. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 190d472..0174c87 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3208,7 +3208,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SAVED); endjob: - qemuDomainObjEndAsyncJob(driver, vm); if (ret != 0) { if (was_running && virDomainObjIsActive(vm)) { virErrorPtr save_err = virSaveLastError(); @@ -3224,9 +3223,10 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, virSetError(save_err); virFreeError(save_err); } - } else if (!vm->persistent) { - qemuDomainRemoveInactive(driver, vm); } + qemuDomainObjEndAsyncJob(driver, vm); + if (ret == 0 && !vm->persistent) + qemuDomainRemoveInactive(driver, vm); cleanup: VIR_FREE(xml); -- 2.1.0
On Wed, Jan 28, 2015 at 05:25:01PM -0500, John Ferlan wrote:
Commit id '540c339a' to fix issues with reference counting and transient domains moved the qemuDomainObjEndAsyncJob call prior to the attempt to restart the guest CPU's resulting in an error:
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: internal error: unexpected async job 3
when (ret != 0) - eg, the error path from qemuDomainSaveMemory.
This patch will adjust the logic to only call the EndAsyncJob after we've tried to restart the guest CPUs. It also needs to adjust the test for qemuDomainRemoveInactive to add the ret == 0 condition.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 190d472..0174c87 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3208,7 +3208,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SAVED); endjob: - qemuDomainObjEndAsyncJob(driver, vm); if (ret != 0) { if (was_running && virDomainObjIsActive(vm)) { virErrorPtr save_err = virSaveLastError(); @@ -3224,9 +3223,10 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, virSetError(save_err); virFreeError(save_err); } - } else if (!vm->persistent) { - qemuDomainRemoveInactive(driver, vm); } + qemuDomainObjEndAsyncJob(driver, vm); + if (ret == 0 && !vm->persistent) + qemuDomainRemoveInactive(driver, vm);
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.
cleanup: VIR_FREE(xml); -- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/29/2015 08:43 AM, Martin Kletzander wrote:
On Wed, Jan 28, 2015 at 05:25:01PM -0500, John Ferlan wrote:
Commit id '540c339a' to fix issues with reference counting and transient domains moved the qemuDomainObjEndAsyncJob call prior to the attempt to restart the guest CPU's resulting in an error:
error: Failed to save domain rhel70 to /tmp/pl/rhel70.save error: internal error: unexpected async job 3
when (ret != 0) - eg, the error path from qemuDomainSaveMemory.
This patch will adjust the logic to only call the EndAsyncJob after we've tried to restart the guest CPUs. It also needs to adjust the test for qemuDomainRemoveInactive to add the ret == 0 condition.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 190d472..0174c87 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3208,7 +3208,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SAVED); endjob: - qemuDomainObjEndAsyncJob(driver, vm); if (ret != 0) { if (was_running && virDomainObjIsActive(vm)) { virErrorPtr save_err = virSaveLastError(); @@ -3224,9 +3223,10 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, virSetError(save_err); virFreeError(save_err); } - } else if (!vm->persistent) { - qemuDomainRemoveInactive(driver, vm); } + qemuDomainObjEndAsyncJob(driver, vm); + if (ret == 0 && !vm->persistent) + qemuDomainRemoveInactive(driver, vm);
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.
From my re-review of the referenced commit, I believe this was the only function where endjob:/cleanup: processing needed the job still around (and only because this command stops the CPU's and error recovery is best effort).
I've combined the two patches into one, adjusted the commit message and pushed just these two as one. Tks - John
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@redhat.com> --- 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
On 01/28/2015 03:25 PM, John Ferlan wrote:
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@redhat.com> --- src/util/virfile.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK. It goes away in your later bigger patch, but doing this makes backports of a trivial fix without the risk of a refactor bug easier. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> --- 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
On 01/28/2015 03:25 PM, John Ferlan wrote:
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@redhat.com> --- src/util/virfile.c | 67 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 28 deletions(-)
+ /* + * 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,
waitpid reporting !WIFEXITED(status) is a case of the child reporting failure (well, reporting abnormal exit due to signal), so it might simplify things if we just do[1]: if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) { except that then you have to distinguish between abnormal exit vs. exit encoding a known errno value when constructing the error message.
+ + /* 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; + }
Hmm. It may be sufficient to just treat ALL recvfd failures as the exit status if we get here, since if recvfd failed, then fd == -1...
+ + /* 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;
...and we would be failing anyways, but losing the errno value from recvfd. And if you change point [1] above to catch abnormal exit, then this entire if branch becomes dead. So how about: - if waitpid failed, use that errno value - waitpid succeeded, but if the child exited abnormally, report failure (not sure what errno value to use here) - waitpid succeeded, but if the child reported non-zero status, report failure (use the errno value that the child encoded into exit status) - waitpid succeeded, but if recvfd failed, report recvfd_errno - waitpid and recvfd succeeded, use the fd Sorry for making you churn on this, given my first-round off-list thoughts on the matter, but hopefully it turns out a little easier to read. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> --- 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
On 01/28/2015 03:25 PM, John Ferlan wrote:
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@redhat.com> --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+)
ACK.
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
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> --- 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
On 01/28/2015 03:25 PM, John Ferlan wrote:
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@redhat.com> --- src/qemu/qemu_driver.c | 9 +++++++++ 1 file changed, 9 insertions(+)
Might be worth squashing with 5; up to you. ACK.
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,
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> --- 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
On 01/28/2015 03:25 PM, John Ferlan wrote:
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@redhat.com> --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+)
I'd feel better with more testing before we take this one; Laine is more familiar with testing root-squash setups.
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,
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/28/2015 05:24 PM, John Ferlan wrote:
Lots of details here:
<...snip...>
John Ferlan (7): qemu: Save/restore error in error path for qemuDomainSaveInternal qemu: Adjust EndAsyncJob for qemuDomainSaveInternal error path virfile: Need to check for ENOTCONN from recvfd failure virfile: Adjust error path for virFileOpenForked qemu: Don't unconditionally delete file in qemuOpenFileAs qemu: remove failed create prior to root squash share path qemu: qemuOpenFileAs - set flag VIR_FILE_OPEN_FORCE_MODE
src/qemu/qemu_driver.c | 24 +++++++++++++++--- src/util/virfile.c | 66 +++++++++++++++++++++++++++++--------------------- 2 files changed, 60 insertions(+), 30 deletions(-)
Thanks for the reviews so far... I combined patches 1 & 2, changed the commit text and pushed I combined patches 5 & 6, changed the commit text and pushed along with patch 3 That leaves the refactor of virFileOpenForked (patch 4) and setting VIR_FILE_OPEN_FORCE_MODE (patch 7) as things that will show up in a v2. John
participants (3)
-
Eric Blake -
John Ferlan -
Martin Kletzander