[libvirt] [PATCH] Fix potential deadlock across fork() in QEMU driver
Osier Yang
jyang at redhat.com
Mon Feb 11 19:05:09 UTC 2013
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.
More information about the libvir-list
mailing list