On 2013年02月12日 00:12, Daniel P. Berrange wrote:

From: "Daniel P. Berrange"<berrange@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@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.