On 7/5/24 10:01, hongmianquan wrote:
> Fix libvirtd hang since fork() was called while another thread had
> security manager locked.
>
> We have the stack security driver, which internally manages other security
> drivers,
> just call them "top" and "nested".
>
> We call virSecurityStackPreFork() to lock the top one, and it also locks
> and then unlocks the nested drivers prior to fork. Then in
> qemuSecurityPostFork(),
> it unlocks the top one, but not the nested ones. Thus, if one of the nested
> drivers ("dac" or "selinux") is still locked, it will cause a deadlock. If we
> always
> surround nested locks with top lock, it is always secure. Because we have got
> top lock
> before fork child libvirtd.
>
> However, it is not always the case in the current code, We discovered this
> case:
> the nested list obtained through the qemuSecurityGetNested() will be locked
> directly
> for subsequent use, such as in virQEMUDriverCreateCapabilities(), where the
> nested list
> is locked using qemuSecurityGetDOI, but the top one is not locked beforehand.
>
> The problem stack is as follows:
>
> libvirtd thread1 libvirtd thread2 child libvirtd
> | | |
> | | |
> virsh capabilities qemuProcessLanuch |
> | | |
> | lock top |
> | | |
> lock nested | |
> | | |
> | fork------------------->|(nested lock
> held by thread1)
> | | |
> | | |
> unlock nested unlock top unlock top
> |
> |
> qemuSecuritySetSocketLabel
> |
> |
> lock nested (deadlock)
>
> In this commit, we ensure that the top lock is acquired before the nested
> lock,
> so during fork, it's not possible for another task to acquire the nested lock.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1303031
>
> Signed-off-by: hongmianquan <[email protected]>
> ---
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_conf.c | 13 +++++++++++--
> src/qemu/qemu_driver.c | 13 +++++++++++--
> src/qemu/qemu_security.h | 2 ++
> src/security/security_manager.c | 22 ++++++++++++++++++++++
> src/security/security_manager.h | 2 ++
> 6 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 124128b3c4..9d4958789c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6216,9 +6216,16 @@ static int qemuDomainGetSecurityLabelList(virDomainPtr
> dom,
> ret = 0;
> } else {
> int len = 0;
> - virSecurityManager ** mgrs =
> qemuSecurityGetNested(driver->securityManager);
> - if (!mgrs)
> + virSecurityManager ** mgrs = NULL;
> +
> + /* Ensure top lock is acquired before nested locks */
> + qemuSecurityStackLock(driver->securityManager);
> +
> + mgrs = qemuSecurityGetNested(driver->securityManager);
> + if (!mgrs) {
> + qemuSecurityStackUnlock(driver->securityManager);
> goto cleanup;
> + }
>
> /* Allocate seclabels array */
> for (i = 0; mgrs[i]; i++)
> @@ -6233,11 +6240,13 @@ static int
> qemuDomainGetSecurityLabelList(virDomainPtr dom,
> &(*seclabels)[i]) < 0) {
> VIR_FREE(mgrs);
> VIR_FREE(*seclabels);
> + qemuSecurityStackUnlock(driver->securityManager);
> goto cleanup;
Nitpick, the unlock can be done before VIR_FREE()-s so that the critical
section is a bit shorter.
> }
> }
> ret = len;
> VIR_FREE(mgrs);
> + qemuSecurityStackUnlock(driver->securityManager);
Same here.
Michal
