Re: [External] Re: [PATCH v3 0/2] security_manager: Fix security manager deadlock after libvirtd fork()

On 7/5/24 10:01, hongmianquan wrote: > 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) > > v3 changes: > Made modifications based on Michal's comments > - ensured matching qemuSecurityStackLock() and qemuSecurityStackUnlock() > - modify the correct order in libvirt_private.syms > - split the code streamlining part into a separate patch > > hongmianquan (2): > security_manager: Ensure top lock is acquired before nested locks > security_manager: Remove redundant qemuSecurityGetNested() call > > src/libvirt_private.syms | 2 ++ > src/qemu/qemu_conf.c | 13 +++++++++++-- > src/qemu/qemu_driver.c | 21 +++++++++++++-------- > src/qemu/qemu_security.h | 2 ++ > src/security/security_manager.c | 22 ++++++++++++++++++++++ > src/security/security_manager.h | 2 ++ > 6 files changed, 52 insertions(+), 10 deletions(-) > Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; and merged. Congratulations on your first libvirt contribution! Michal