[PATCH 1/1] vircommand.c: write child pidfile before process tuning in virExec()
When VIR_EXEC_DAEMON is true and cmd->pidfile exists, the parent will expect the pidfile to be written before exiting, sitting tight in a saferead() call waiting. The child then does process tuning (via virProcessSet* functions) before writing the pidfile. Problem is that these tunings can fail, and trigger a 'fork_error' jump, before cmd->pidfile is written. The result is that the process was aborted in the child, but the parent is still hang in the saferead() call. This behavior can be reproduced by trying to create and execute a QEMU guest in user mode (e.g. using qemu:///session as non-root). virProcessSetMaxMemLock() will fail if the spawned libvirtd user process does not have CAP_SYS_RESOURCE capability. setrlimit() will fail, and a 'fork_error' jump is triggered before cmd->pidfile is written. The parent will hung in saferead() indefinitely. From the user perspective, 'virsh start <guest>' will hang up indefinitely. CTRL+C can be used to retrieve the terminal, but any subsequent 'virsh' call will also hang because the previous libvirtd user process is still there. We can fix this by moving all virProcessSet*() tuning functions to be executed after cmd->pidfile is taken care of. In the case mentioned above, this would be the result of 'virsh start' after this patch: error: Failed to start domain vm1 error: internal error: Process exited prior to exec: libvirt: error : cannot limit locked memory to 79691776: Operation not permitted Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1882093 Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/vircommand.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 76f7eb9a3d..0475e22db6 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -787,15 +787,6 @@ virExec(virCommandPtr cmd) } } - if (virProcessSetMaxMemLock(0, cmd->maxMemLock) < 0) - goto fork_error; - if (virProcessSetMaxProcesses(0, cmd->maxProcesses) < 0) - goto fork_error; - if (virProcessSetMaxFiles(0, cmd->maxFiles) < 0) - goto fork_error; - if (cmd->setMaxCore && - virProcessSetMaxCoreSize(0, cmd->maxCore) < 0) - goto fork_error; if (cmd->pidfile) { int pidfilefd = -1; char c; @@ -820,6 +811,16 @@ virExec(virCommandPtr cmd) /* pidfilefd is intentionally leaked. */ } + if (virProcessSetMaxMemLock(0, cmd->maxMemLock) < 0) + goto fork_error; + if (virProcessSetMaxProcesses(0, cmd->maxProcesses) < 0) + goto fork_error; + if (virProcessSetMaxFiles(0, cmd->maxFiles) < 0) + goto fork_error; + if (cmd->setMaxCore && + virProcessSetMaxCoreSize(0, cmd->maxCore) < 0) + goto fork_error; + if (cmd->hook) { VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque); ret = cmd->hook(cmd->opaque); -- 2.26.2
On 10/1/20 4:35 PM, Daniel Henrique Barboza wrote:
When VIR_EXEC_DAEMON is true and cmd->pidfile exists, the parent will expect the pidfile to be written before exiting, sitting tight in a saferead() call waiting.
The child then does process tuning (via virProcessSet* functions) before writing the pidfile. Problem is that these tunings can fail, and trigger a 'fork_error' jump, before cmd->pidfile is written. The result is that the process was aborted in the child, but the parent is still hang in the saferead() call.
This behavior can be reproduced by trying to create and execute a QEMU guest in user mode (e.g. using qemu:///session as non-root). virProcessSetMaxMemLock() will fail if the spawned libvirtd user process does not have CAP_SYS_RESOURCE capability. setrlimit() will fail, and a 'fork_error' jump is triggered before cmd->pidfile is written. The parent will hung in saferead() indefinitely. From the user perspective, 'virsh start <guest>' will hang up indefinitely. CTRL+C can be used to retrieve the terminal, but any subsequent 'virsh' call will also hang because the previous libvirtd user process is still there.
We can fix this by moving all virProcessSet*() tuning functions to be executed after cmd->pidfile is taken care of. In the case mentioned above, this would be the result of 'virsh start' after this patch:
error: Failed to start domain vm1 error: internal error: Process exited prior to exec: libvirt: error : cannot limit locked memory to 79691776: Operation not permitted
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1882093
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/vircommand.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> And congratz onto gaining commit rights :-) Michal
On 10/2/20 1:07 PM, Michal Prívozník wrote:
On 10/1/20 4:35 PM, Daniel Henrique Barboza wrote:
When VIR_EXEC_DAEMON is true and cmd->pidfile exists, the parent [...]
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1882093
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/vircommand.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
And congratz onto gaining commit rights :-)
Thanks! By the way, we have a 'do not push stuff on Friday' rule to avoid breaking things during weekends, right? Guess I'll wait to push this one next Monday. DHB
Michal
On 10/2/20 6:42 PM, Daniel Henrique Barboza wrote:
On 10/2/20 1:07 PM, Michal Prívozník wrote:
On 10/1/20 4:35 PM, Daniel Henrique Barboza wrote:
When VIR_EXEC_DAEMON is true and cmd->pidfile exists, the parent [...]
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1882093
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/vircommand.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
And congratz onto gaining commit rights :-)
Thanks!
By the way, we have a 'do not push stuff on Friday' rule to avoid breaking things during weekends, right? Guess I'll wait to push this one next Monday.
I'm not aware of any such rule. In fact, I've pushed couple of patches today already. I say, go ahead and push them! It doesn't break the build and if there is a subtle bug with a commit, I'm sure it can wait over the weekend anyway (I'm not picking on your commit in particular, just any commit - the sooner we test it the better). Michal
On 10/2/20 2:28 PM, Michal Prívozník wrote:
On 10/2/20 6:42 PM, Daniel Henrique Barboza wrote:
On 10/2/20 1:07 PM, Michal Prívozník wrote:
On 10/1/20 4:35 PM, Daniel Henrique Barboza wrote:
When VIR_EXEC_DAEMON is true and cmd->pidfile exists, the parent [...]
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1882093
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/vircommand.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
And congratz onto gaining commit rights :-)
Thanks!
By the way, we have a 'do not push stuff on Friday' rule to avoid breaking things during weekends, right? Guess I'll wait to push this one next Monday.
I'm not aware of any such rule. In fact, I've pushed couple of patches today already. I say, go ahead and push them! It doesn't break the build and if there is a subtle bug with a commit, I'm sure it can wait over the weekend anyway (I'm not picking on your commit in particular, just any commit - the sooner we test it the better).
Roger that. The deed is done :) Thanks, DHB
Michal
On 10/1/20 10:35 AM, Daniel Henrique Barboza wrote:
When VIR_EXEC_DAEMON is true and cmd->pidfile exists, the parent will expect the pidfile to be written before exiting, sitting tight in a saferead() call waiting.
The child then does process tuning (via virProcessSet* functions) before writing the pidfile. Problem is that these tunings can fail, and trigger a 'fork_error' jump, before cmd->pidfile is written. The result is that the process was aborted in the child, but the parent is still hang in the saferead() call.
This behavior can be reproduced by trying to create and execute a QEMU guest in user mode (e.g. using qemu:///session as non-root). virProcessSetMaxMemLock() will fail if the spawned libvirtd user process does not have CAP_SYS_RESOURCE capability. setrlimit() will fail, and a 'fork_error' jump is triggered before cmd->pidfile is written. The parent will hung in saferead() indefinitely. From the user perspective, 'virsh start <guest>' will hang up indefinitely. CTRL+C can be used to retrieve the terminal, but any subsequent 'virsh' call will also hang because the previous libvirtd user process is still there.
We can fix this by moving all virProcessSet*() tuning functions to be executed after cmd->pidfile is taken care of. In the case mentioned above, this would be the result of 'virsh start' after this patch:
error: Failed to start domain vm1 error: internal error: Process exited prior to exec: libvirt: error : cannot limit locked memory to 79691776: Operation not permitted
This helped me resolve this issue: https://software.intel.com/content/www/us/en/develop/blogs/best-known-method...
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1882093
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
--- src/util/vircommand.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 76f7eb9a3d..0475e22db6 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -787,15 +787,6 @@ virExec(virCommandPtr cmd) } }
- if (virProcessSetMaxMemLock(0, cmd->maxMemLock) < 0) - goto fork_error; - if (virProcessSetMaxProcesses(0, cmd->maxProcesses) < 0) - goto fork_error; - if (virProcessSetMaxFiles(0, cmd->maxFiles) < 0) - goto fork_error; - if (cmd->setMaxCore && - virProcessSetMaxCoreSize(0, cmd->maxCore) < 0) - goto fork_error; if (cmd->pidfile) { int pidfilefd = -1; char c; @@ -820,6 +811,16 @@ virExec(virCommandPtr cmd) /* pidfilefd is intentionally leaked. */ }
+ if (virProcessSetMaxMemLock(0, cmd->maxMemLock) < 0) + goto fork_error; + if (virProcessSetMaxProcesses(0, cmd->maxProcesses) < 0) + goto fork_error; + if (virProcessSetMaxFiles(0, cmd->maxFiles) < 0) + goto fork_error; + if (cmd->setMaxCore && + virProcessSetMaxCoreSize(0, cmd->maxCore) < 0) + goto fork_error; + if (cmd->hook) { VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque); ret = cmd->hook(cmd->opaque);
participants (3)
-
Daniel Henrique Barboza -
Michal Prívozník -
Stefan Berger