On 04/09/2016 12:14 PM, Maxim Nestratov wrote:
> Below is backtraces of two deadlocked threads:
>
> thread #1:
> virDomainConfVMNWFilterTeardown
> virNWFilterTeardownFilter
> lock updateMutex <------------
> _virNWFilterTeardownFilter
> try to lock interface <----------
>
> thread #2:
> learnIPAddressThread
> lock interface <-------
> virNWFilterInstantiateFilterLate
> try to lock updateMutex <----------
>
> The problem is fixed by unlocking interface before calling
> virNWFilterInstantiateFilterLate to avoid updateMutex and interface ordering
> deadlocks. Otherwise we are going to instantiate the filter while holding
> interface lock, which will try to lock updateMutex, and if some other thread
> instantiating a filter in parallel is holding updateMutex and is trying to
> lock interface, both will deadlock.
> Also it is safe to unlock interface before virNWFilterInstantiateFilterLate
> because learnIPAddressThread stopped capturing packets and applied necessary
> rules on the interface, while instantiating a new filter doesn't require a
> locked interface.
>
> Signed-off-by: Maxim Nestratov <mnestratov@xxxxxxxxxxxxx>
> ---
> src/nwfilter/nwfilter_learnipaddr.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
> index 1adbadb..cfd92d9 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.c
> +++ b/src/nwfilter/nwfilter_learnipaddr.c
> @@ -611,6 +611,16 @@ learnIPAddressThread(void *arg)
> sa.data.inet4.sin_addr.s_addr = vmaddr;
> char *inetaddr;
>
> + /* It is necessary to unlock interface here to avoid updateMutex and
> + * interface ordering deadlocks. Otherwise we are going to
> + * instantiate the filter, which will try to lock updateMutex, and
> + * some other thread instantiating a filter in parallel is holding
> + * updateMutex and is trying to lock interface, both will deadlock.
> + * Also it is safe to unlock interface here because we stopped
> + * capturing and applied necessary rules on the interface, while
> + * instantiating a new filter doesn't require a locked interface.*/
> + virNWFilterUnlockIface(req->ifname);
> +
> if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) {
> if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) {
> VIR_ERROR(_("Failed to add IP address %s to IP address "
> @@ -636,11 +646,11 @@ learnIPAddressThread(void *arg)
> req->ifname, req->ifindex);
>
> techdriver->applyDropAllRules(req->ifname);
> + virNWFilterUnlockIface(req->ifname);
> }
>
> VIR_DEBUG("pcap thread terminating for interface %s", req->ifname);
>
> - virNWFilterUnlockIface(req->ifname);
>
> err_no_lock:
> virNWFilterDeregisterLearnReq(req->ifindex);
>
This looks sensible to me... the virNWFilterInstantiateFilterLate call tree
certainly expects the iface to be unlocked at a certain point. This patch just
moves the unlock a bit earlier.
ACK, but maybe wait another day to see if anyone else wants to jump in, I'm
not really familiar with this code (then again I doubt anyone watching the
list is deeply familiar with it either)
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list