[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] xen: Avoid deadlock in xenUnifiedDomainGetXMLDesc



On Mon, Jun 24, 2013 at 11:08:16AM +0200, Stefan Bader wrote:
> So this is one way I am able to get this issue resolved. It might not
> be ideal as maybe there is a slight chance of inconsistencies. But at
> least it does not lock up anymore.
> 
> -Stefan
> 
> ---
> 
> From f0c28a9f7af84a01bb2c3d71067adefb92e919d9 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan bader canonical com>
> Date: Mon, 24 Jun 2013 09:30:20 +0200
> Subject: [PATCH] xen: Avoid deadlock in xenUnifiedDomainGetXMLDesc
> 
> Commit 95e18efd added the use virDomainDefPtr to several Xen
> driver methods. That structure is obtained by calling
> xenGetDomainDefForDom() which will acquire the mutex to the
> priv pointer.
> Unfortunately (at least) xenUnifiedDomainGetXMLDesc already
> takes that mutex and now is locking up while calling into
> xenDomainUsedCpus().
> 
> xenDomainUsedCpus
>   ...
>   nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
>     return xenUnifiedDomainGetVcpusFlags(...)
>       ...
>       if (!(def = xenGetDomainDefForDom(dom)))
>         return xenGetDomainDefForUUID(dom->conn, dom->uuid);
>           ...
>           ret = xenHypervisorLookupDomainByUUID(conn, uuid);
>             ...
>             xenUnifiedLock(priv);
>             name = xenStoreDomainGetName(conn, id);
>             xenUnifiedUnlock(priv);
>   ...
>   if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
>     ...
>     if (!(def = xenGetDomainDefForDom(dom)))
>       [again like above]
> 
> The deadlock is observable when connecting to a Xen host using the
> old xm stack and trying to obtain domain XML data from any running
> guest (like "dumpxml 0").
> 
> This would be a minimal fix that tries to avoid the deadlock. I am not
> completely sure this is not allowing a small window for getting
> inconsistent data.

The root cause is that one public API is calling into another
public API. This is bad practice in general - as well as the
deadlock you see, it will result in multiple access control
checks which should not happen. I'm looking into how to refactor
the code to simplify the calling pattern here.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]