Re: [PATCH 1/1] vircommand.c: write child pidfile before process tuning in virExec()

[Stefan Berger <stefanb@xxxxxxxxxxxxx>]

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-methods-for-setting-locked-memory-size.html


> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1882093
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>

Tested-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>


> ---
>   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);