[libvirt] [PATCH] Release VM lock before acquiring virDomainObjListPtr lock

[Peter Krempa]

On 04/09/13 11:08, Daniel P. Berrange wrote:
> On Mon, Apr 08, 2013 at 09:15:28PM -0600, Eric Blake wrote:
>> On 02/11/2013 09:46 AM, Daniel P. Berrange wrote:
>>> From: "Daniel P. Berrange" <berrange at redhat.com>
>>>
>>> When removing a VM from the virDomainObjListPtr, we must not
>>> be holding the VM lock while acquiring the list lock. Re-order
>>> code to ensure that we can release the VM lock early.
>>> ---
>>>   src/conf/domain_conf.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 5e16ddf..d92e54a 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -2115,11 +2115,10 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
>>>   {
>>>       char uuidstr[VIR_UUID_STRING_BUFLEN];
>>>
>>> -    virObjectLock(doms);
>>>       virUUIDFormat(dom->def->uuid, uuidstr);
>>> -
>>>       virObjectUnlock(dom);
>>>
>>> +    virObjectLock(doms);
>>
>> This patch seems to be implicated in Peter's latest proof of a
>> use-after-free data race:
>> https://www.redhat.com/archives/libvir-list/2013-April/msg00674.html
>
> The caller of this method should own a reference on virDomainObjPtr.
> This method will result in that reference being released, as the
> dom is removed from the list.
>
> If any other thread experiances a use-after-free, then that thread
> must have been missing a reference.

Actually, by default no reference updating is being done when the domain 
object is taken from the list. The only place where references are 
updated on domain objects is monitor code, right before the locks are 
dropped.

I'm working on patches that adds reference tracking when the domain 
object pointer is acquired from the list to avoid this kind of issue.

>
>> I'm trying to understand what the behavior was before this patch went in.
>
> Well this was just fixing a deadlock introduced in a previous patch.
> You need to look further back than just this patch. Originally the
> global QEMU driver lock would be held preventing any kind of concurrent
> execution.
>
> Regards,
> Daniel
>

Peter