On Tue, Jul 23, 2024 at 13:51:09 +0100, Daniel P. Berrangé wrote: > On Tue, Jul 23, 2024 at 02:41:13PM +0200, Peter Krempa wrote: > > On Fri, Jul 19, 2024 at 14:46:49 +0200, Michal Privoznik wrote: > > > Imagine two threads. Thread A is executing qemuProcessStop() and > > > thread B is executing qemuDomainCreateXML(). To make things more > > > interesting, the domain XML passed to qemuDomainCreateXML matches > > > name + UUID of that the virDomainObj that qemuProcessStop() is > > > currently working with. Here's what happens. > > > > > > 1) Thread A locks @vm, enters qemuProcessStop(). > > > > > > 2) Thread B parses given XML, calls virDomainObjListAdd() -> > > > virDomainObjListAdd() -> virDomainObjListAddLocked() -> > > > virDomainObjListFindByUUIDLocked(). Since there's a match on > > > UUID, an virDomainObj object is returned and the thread > > > proceeds to calling virObjectLock(). NB, it's the same object > > > as in thread A. > > > > > > 3) Thread A sets vm->def->id = -1; this means that from this > > > point on, virDomainObjIsActive() will return false. > > > > > > 4) Thread A calls qemuDomainObjStopWorker() which unlocks the > > > @vm. > > > > > > 5) Thread B acquires the @vm lock and since > > > virDomainObjIsActive() returns false, it proceeds to calling > > > virDomainObjAssignDef() where vm->def is replaced. > > > > > > 6) Thread B then calls qemuProcessBeginJob() which unlocks the > > > @vm temporarily. > > > > > > 7) Thread A, still in qemuDomainObjStopWorker() acquires @vm lock > > > and proceeds with cleanup. > > > > > > 8) Thread A finds different definition than the one needing > > > cleanup. > > > > > > In my testing I've seen stale pointers, e.g. > > > vm->def->nets[0]->priv was NULL, which lead to a SIGSEGV as > > > there's 'QEMU_DOMAIN_NETWORK_PRIVATE(net)->created' line when > > > cleaning up nets. Your mileage may vary. > > > > > > Even if we did not crash, the plain fact that vm->def is changed > > > in the middle of qemuProcessStop() means we might be cleaning up > > > something else than intended. > > > > This paragraph is the important bit. The root cause of the problem here > > is that 'virDomainObjListAdd' inside 'qemuDomainCreateXML' can modify > > 'vm->def' whithout holding any _MODIFY-type JOB on the domain object > > which we normally require for any modification of 'vm->def' related data. > > > > This wasn't a problem until now as we've relinquished the lock on @vm > > only in situations when the @vm object was considered live: > > > > 1) Before the per-VM thread cleanup was added to qemuProcessStop it > > never unlocked > > 2) After the per-VM thread cleanup was added, this unlock was done > > prior to setting vm->def->id to '-1' > > 3) All other cases are done only when the VM is live. > > > > > As a fix, I'm moving all lines that obviously touch vm->def > > > before the domain object is unlocked. That left > > > virHookCall(VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END) nearly > > > next to virHookCall(VIR_HOOK_QEMU_OP_RELEASE, > > > VIR_HOOK_SUBOP_END) which I figured is not something we want. So > > > I've shuffled things a bit more. > > > > This feels like a fix for symptoms of 'virDomainObjListAdd' not > > honouring the _MODIFY-type job expectation, and we're shuffling code > > around so that it doesn't care about the broken expectation. > > > > Since I don't currently have a better idea of how to fix this I'm okay > > with this patch given the following conditions: > > > > > Fixes: 3865410e7f67ca4ec66e9a905e75f452762a97f0 > > > > Explain that the above commit inverted the order of setting the VM as > > inactive and unlocking thus allowing the above sequence of events to > > happen, and, > > Why not just revert that change ? It claimed to be making things > safer, but did the opposite. Even with this fixup below I'm > pretty uncomfortable with setting 'id = -1' and unlocking the > @vm, before we've done all our cleanup. You'd have to also revert d29e0f3d4a5362d7b33261df1e55896396707de3 which is the commit actually moving the unlock of the VM object inside qemuProcessStop after setting id = -1 which actually does fix a crash on migration. We can possibly move the qemuDomainObjStopWorker(vm) call as the last thing in qemuProcessStop, so that everything is cleaned up before unlocking. Either way, unlocking inside qemuProcessStop is fragile as we're giving a chance for races which would not be possible before.
