Re: [PATCH 2/3] qemu: get the stackManager lock before getting nested lock

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

On 9/28/22 15:53, Jiang Jiacheng wrote:
> When creating a VM by forking, there is a logic that get the lock of
> StacksecurityManager before get other nested lock to avoid deadlock
> between child process and thread of the parent. While in
> `virQEMUDriverCreateCapabilities` and `qemuDomainGetSecurityLabelList`,
> we get nested lock without getting the StacksecurityManager lock, which
> will result in deadlock in some concurrent scenarios. It is better to keep
> the logical in these funcitons same as others.
> 
> Signed-off-by: Jiang Jiacheng <jiangjiacheng@xxxxxxxxxx>
> ---
>  src/qemu/qemu_conf.c   | 6 +++++-
>  src/qemu/qemu_driver.c | 3 +++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 3b75cdeb95..745d3e6a5e 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1331,6 +1331,7 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriver *driver)
>  
>      caps->host.secModels = g_new0(virCapsHostSecModel, caps->host.nsecModels);
>  
> +    virObjectLock(driver->securityManager);
>      for (i = 0; sec_managers[i]; i++) {
>          virCapsHostSecModel *sm = &caps->host.secModels[i];
>          doi = qemuSecurityGetDOI(sec_managers[i]);
> @@ -1342,14 +1343,17 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriver *driver)
>              lbl = qemuSecurityGetBaseLabel(sec_managers[i], virtTypes[j]);
>              type = virDomainVirtTypeToString(virtTypes[j]);
>              if (lbl &&
> -                virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0)
> +                virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0) {
> +                virObjectUnlock(driver->securityManager);
>                  return NULL;
> +            }
>          }
>  
>          VIR_DEBUG("Initialized caps for security driver \"%s\" with "
>                    "DOI \"%s\"", model, doi);
>      }
>  
> +    virObjectUnlock(driver->securityManager);
>      caps->host.numa = virCapabilitiesHostNUMANewHost();
>      caps->host.cpu = virQEMUDriverGetHostCPU(driver);
>      return g_steal_pointer(&caps);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 707f4cc1bb..9fb5f1d653 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5848,15 +5848,18 @@ static int qemuDomainGetSecurityLabelList(virDomainPtr dom,
>          (*seclabels) = g_new0(virSecurityLabel, len);
>          memset(*seclabels, 0, sizeof(**seclabels) * len);
>  
> +        virObjectLock(driver->securityManager);
>          /* Fill the array */
>          for (i = 0; i < len; i++) {
>              if (qemuSecurityGetProcessLabel(mgrs[i], vm->def, vm->pid,
>                                              &(*seclabels)[i]) < 0) {
>                  VIR_FREE(mgrs);
>                  VIR_FREE(*seclabels);
> +                virObjectUnlock(driver->securityManager);
>                  goto cleanup;
>              }
>          }
> +        virObjectUnlock(driver->securityManager);
>          ret = len;
>          VIR_FREE(mgrs);
>      }

I think the problem here is that virSecurityManagerGetNested() does not
acquire its own lock but yet accesses its own internal data. I believe
proper fix is among these lines:

diff --git i/src/security/security_manager.c w/src/security/security_manager.c
index 572e400a48..82ca748b61 100644
--- i/src/security/security_manager.c
+++ w/src/security/security_manager.c
@@ -973,6 +973,7 @@ virSecurityManagerGetMountOptions(virSecurityManager *mgr,
 virSecurityManager **
 virSecurityManagerGetNested(virSecurityManager *mgr)
 {
+    VIR_LOCK_GUARD lock = virObjectLockGuard(mgr);
     virSecurityManager ** list = NULL;
 
     if (STREQ("stack", mgr->drv->name))


Michal