Re: [PATCH] qemuProcessStop: Don't unlock domain until most of the cleanup is done

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.