Re: [PATCH] qemu: Fix potential crash during driver cleanup

2023-04-12 Thread Martin Kletzander

On Tue, Apr 11, 2023 at 09:47:28AM -0600, Jim Fehlig wrote:

During qemu driver shutdown, objects are freed in qemuStateCleanup that
could still be used by active worker threads, resulting in crashes. E.g.
a worker thread could be processing a monitor EOF event after the
security manager is already disposed

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x7fd9a9a1e1fe in virSecurityManagerMoveImageMetadata 
(mgr=0x7fd948012160, pid=-1, src=src@entry=0x7fd98c072c90, dst=dst@entry=0x0)
   at ../../src/security/security_manager.c:468
#1  0x7fd9646ff0f0 in qemuSecurityMoveImageMetadata 
(driver=driver@entry=0x7fd948043830, vm=vm@entry=0x7fd98c066db0, 
src=src@entry=0x7fd98c072c90,
   dst=dst@entry=0x0) at ../../src/qemu/qemu_security.c:182
#2  0x7fd96462c7b0 in qemuBlockRemoveImageMetadata 
(driver=driver@entry=0x7fd948043830, vm=vm@entry=0x7fd98c066db0, 
diskTarget=0x7fd98c072530 "vda",
   src=) at ../../src/qemu/qemu_block.c:2628
#3  0x7fd9646929d6 in qemuProcessStop (driver=driver@entry=0x7fd948043830, 
vm=vm@entry=0x7fd98c066db0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN,
   asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=) at 
../../src/qemu/qemu_process.c:7585
#4  0x7fd9646fc842 in processMonitorEOFEvent (vm=0x7fd98c066db0, 
driver=0x7fd948043830) at ../../src/qemu/qemu_driver.c:4794
#5  qemuProcessEventHandler (data=0x561a93febb60, opaque=0x7fd948043830) at 
../../src/qemu/qemu_driver.c:4900
#6  0x7fd9a9971a31 in virThreadPoolWorker 
(opaque=opaque@entry=0x561a93fb58e0) at ../../src/util/virthreadpool.c:163
(gdb) p mgr->drv
$2 = (virSecurityDriverPtr) 0x0

Prior to commit 7cf76d4e3ab, the worker thread pool was freed before
disposing any driver objects. Let's return to that pattern, but leave
the other changes made by 7cf76d4e3ab.

Signed-off-by: Jim Fehlig 


Reviewed-by: Martin Kletzander 


---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b4fb7ec1df..28e470e4a2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1062,6 +1062,7 @@ qemuStateCleanup(void)
if (!qemu_driver)
return -1;

+virThreadPoolFree(qemu_driver->workerPool);
virObjectUnref(qemu_driver->migrationErrors);
virLockManagerPluginUnref(qemu_driver->lockManager);
virSysinfoDefFree(qemu_driver->hostsysinfo);
@@ -1078,7 +1079,6 @@ qemuStateCleanup(void)
ebtablesContextFree(qemu_driver->ebtables);
VIR_FREE(qemu_driver->qemuImgBinary);
virObjectUnref(qemu_driver->domains);
-virThreadPoolFree(qemu_driver->workerPool);

if (qemu_driver->lockFD != -1)
virPidFileRelease(qemu_driver->config->stateDir, "driver", 
qemu_driver->lockFD);
--
2.40.0



signature.asc
Description: PGP signature


[PATCH] qemu: Fix potential crash during driver cleanup

2023-04-11 Thread Jim Fehlig
During qemu driver shutdown, objects are freed in qemuStateCleanup that
could still be used by active worker threads, resulting in crashes. E.g.
a worker thread could be processing a monitor EOF event after the
security manager is already disposed

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x7fd9a9a1e1fe in virSecurityManagerMoveImageMetadata 
(mgr=0x7fd948012160, pid=-1, src=src@entry=0x7fd98c072c90, dst=dst@entry=0x0)
at ../../src/security/security_manager.c:468
#1  0x7fd9646ff0f0 in qemuSecurityMoveImageMetadata 
(driver=driver@entry=0x7fd948043830, vm=vm@entry=0x7fd98c066db0, 
src=src@entry=0x7fd98c072c90,
dst=dst@entry=0x0) at ../../src/qemu/qemu_security.c:182
#2  0x7fd96462c7b0 in qemuBlockRemoveImageMetadata 
(driver=driver@entry=0x7fd948043830, vm=vm@entry=0x7fd98c066db0, 
diskTarget=0x7fd98c072530 "vda",
src=) at ../../src/qemu/qemu_block.c:2628
#3  0x7fd9646929d6 in qemuProcessStop (driver=driver@entry=0x7fd948043830, 
vm=vm@entry=0x7fd98c066db0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN,
asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=) at 
../../src/qemu/qemu_process.c:7585
#4  0x7fd9646fc842 in processMonitorEOFEvent (vm=0x7fd98c066db0, 
driver=0x7fd948043830) at ../../src/qemu/qemu_driver.c:4794
#5  qemuProcessEventHandler (data=0x561a93febb60, opaque=0x7fd948043830) at 
../../src/qemu/qemu_driver.c:4900
#6  0x7fd9a9971a31 in virThreadPoolWorker 
(opaque=opaque@entry=0x561a93fb58e0) at ../../src/util/virthreadpool.c:163
(gdb) p mgr->drv
$2 = (virSecurityDriverPtr) 0x0

Prior to commit 7cf76d4e3ab, the worker thread pool was freed before
disposing any driver objects. Let's return to that pattern, but leave
the other changes made by 7cf76d4e3ab.

Signed-off-by: Jim Fehlig 
---
 src/qemu/qemu_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b4fb7ec1df..28e470e4a2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1062,6 +1062,7 @@ qemuStateCleanup(void)
 if (!qemu_driver)
 return -1;
 
+virThreadPoolFree(qemu_driver->workerPool);
 virObjectUnref(qemu_driver->migrationErrors);
 virLockManagerPluginUnref(qemu_driver->lockManager);
 virSysinfoDefFree(qemu_driver->hostsysinfo);
@@ -1078,7 +1079,6 @@ qemuStateCleanup(void)
 ebtablesContextFree(qemu_driver->ebtables);
 VIR_FREE(qemu_driver->qemuImgBinary);
 virObjectUnref(qemu_driver->domains);
-virThreadPoolFree(qemu_driver->workerPool);
 
 if (qemu_driver->lockFD != -1)
 virPidFileRelease(qemu_driver->config->stateDir, "driver", 
qemu_driver->lockFD);
-- 
2.40.0