[libvirt] [PATCH] Fix potential deadlock across fork() in QEMU driver

[Osier Yang]

On 2013年02月12日 00:12, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> The hook scripts used by virCommand must be careful wrt
> accessing any mutexes that may have been held by other
> threads in the parent process. With the recent refactorigng

s/refactorigng/refactoring/,

> there are 2 potential flaws lurking, which will become real
> deadlock bugs once the global QEMU driver lock is removed.
>
> Remove use of the QEMU driver lock from the hook function
> by passing in the 'virQEMUDriverConfigPtr' instance directly.
>
> Add functions to the virSecurityManager to be invoked before
> and after fork, to ensure the mutex is held by the current
> thread. This allows it to be safely used in the hook script
> in the child procss.

s/procss/process/,

>
> Signed-off-by: Daniel P. Berrange<berrange at redhat.com>
> ---
>   src/libvirt_private.syms        |  2 ++
>   src/qemu/qemu_process.c         | 16 ++++++++++++----
>   src/security/security_manager.c | 20 ++++++++++++++++++++
>   src/security/security_manager.h |  3 +++
>   4 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index cb81497..5f19269 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1054,6 +1054,8 @@ virSecurityManagerGetProcessLabel;
>   virSecurityManagerNew;
>   virSecurityManagerNewDAC;
>   virSecurityManagerNewStack;
> +virSecurityManagerPostFork;
> +virSecurityManagerPreFork;
>   virSecurityManagerReleaseLabel;
>   virSecurityManagerReserveLabel;
>   virSecurityManagerRestoreAllLabel;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 9759332..12e3544 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2773,6 +2773,7 @@ struct qemuProcessHookData {
>       virDomainObjPtr vm;
>       virQEMUDriverPtr driver;
>       virBitmapPtr nodemask;
> +    virQEMUDriverConfigPtr cfg;
>   };
>
>   static int qemuProcessHook(void *data)
> @@ -2780,7 +2781,11 @@ static int qemuProcessHook(void *data)
>       struct qemuProcessHookData *h = data;
>       int ret = -1;
>       int fd;
> -    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(h->driver);
> +    /* This method cannot use any mutexes, which are not
> +     * protected across fork()
> +     */
> +

Useless blank line.

> +    virSecurityManagerPostFork(h->driver->securityManager);

>
>       /* Some later calls want pid present */
>       h->vm->pid = getpid();
> @@ -2796,7 +2801,7 @@ static int qemuProcessHook(void *data)
>       if (virSecurityManagerSetSocketLabel(h->driver->securityManager, h->vm->def)<  0)
>           goto cleanup;
>       if (virDomainLockProcessStart(h->driver->lockManager,
> -                                  cfg->uri,
> +                                  h->cfg->uri,
>                                     h->vm,
>                                     /* QEMU is always paused initially */
>                                     true,
> @@ -2805,7 +2810,7 @@ static int qemuProcessHook(void *data)
>       if (virSecurityManagerClearSocketLabel(h->driver->securityManager, h->vm->def)<  0)
>           goto cleanup;
>
> -    if (qemuProcessLimits(cfg)<  0)
> +    if (qemuProcessLimits(h->cfg)<  0)
>           goto cleanup;
>
>       /* This must take place before exec(), so that all QEMU
> @@ -2831,7 +2836,7 @@ static int qemuProcessHook(void *data)
>       ret = 0;
>
>   cleanup:
> -    virObjectUnref(cfg);
> +    virObjectUnref(h->cfg);
>       VIR_DEBUG("Hook complete ret=%d", ret);
>       return ret;
>   }
> @@ -3642,6 +3647,7 @@ int qemuProcessStart(virConnectPtr conn,
>       hookData.conn = conn;
>       hookData.vm = vm;
>       hookData.driver = driver;
> +    hookData.cfg = virObjectRef(cfg);
>
>       VIR_DEBUG("Beginning VM startup process");
>
> @@ -3973,7 +3979,9 @@ int qemuProcessStart(virConnectPtr conn,
>       virCommandDaemonize(cmd);
>       virCommandRequireHandshake(cmd);
>
> +    virSecurityManagerPreFork(driver->securityManager);
>       ret = virCommandRun(cmd, NULL);
> +    virSecurityManagerPostFork(driver->securityManager);
>
>       /* wait for qemu process to show up */
>       if (ret == 0) {
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 6f8ddbf..50962ba 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -192,6 +192,26 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name,
>                                          requireConfined);
>   }
>
> +
> +/*
> + * Must be called before fork()'ing to ensure mutex state
> + * is sane for the child to use
> + */
> +void virSecurityManagerPreFork(virSecurityManagerPtr mgr)
> +{
> +    virObjectLock(mgr);
> +}
> +
> +
> +/*
> + * Must be called after fork()'ing in both parent and child
> + * to ensure mutex state is sane for the child to use
> + */
> +void virSecurityManagerPostFork(virSecurityManagerPtr mgr)
> +{
> +    virObjectUnlock(mgr);
> +}
> +
>   void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr)
>   {
>       return mgr->privateData;
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index 4d4dc73..8e8accf 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -46,6 +46,9 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver,
>                                                  bool requireConfined,
>                                                  bool dynamicOwnership);
>
> +void virSecurityManagerPreFork(virSecurityManagerPtr mgr);
> +void virSecurityManagerPostFork(virSecurityManagerPtr mgr);
> +
>   void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr);
>
>   const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr);

ACK with the typos and useless blank line fixed.