[libvirt] [PATCH] nwfilter: fix lock order deadlock
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@virtuozzo.com> --- 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); -- 2.4.3
09.04.2016 19:14, Maxim Nestratov пишет:
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@virtuozzo.com> --- src/nwfilter/nwfilter_learnipaddr.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
No one interested?
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@virtuozzo.com> --- 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
On 04/19/2016 05:45 PM, Cole Robinson wrote:
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@virtuozzo.com> --- 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)
Stefan Berger is, but I don't think he watches the list closely these days. I just found the original message (somehow it had been tossed into my spam folder) and Replied-All to it with Stefan Cc'ed.
On 04/20/2016 12:06 PM, Laine Stump wrote:
On 04/19/2016 05:45 PM, Cole Robinson wrote:
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@virtuozzo.com> --- 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)
Stefan Berger is, but I don't think he watches the list closely these days. I just found the original message (somehow it had been tossed into my spam folder) and Replied-All to it with Stefan Cc'ed.
ACK. I agree to the change. It seems to be possible to release the lock earlier. Stefan
20.04.2016 22:04, Stefan Berger пишет:
On 04/20/2016 12:06 PM, Laine Stump wrote:
On 04/19/2016 05:45 PM, Cole Robinson wrote:
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@virtuozzo.com> --- 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)
Stefan Berger is, but I don't think he watches the list closely these days. I just found the original message (somehow it had been tossed into my spam folder) and Replied-All to it with Stefan Cc'ed.
ACK. I agree to the change. It seems to be possible to release the lock earlier.
Stefan
Shall I push it then? Maxim
On 05/24/2016 03:22 AM, Maxim Nestratov wrote:
20.04.2016 22:04, Stefan Berger пишет:
On 04/20/2016 12:06 PM, Laine Stump wrote:
On 04/19/2016 05:45 PM, Cole Robinson wrote:
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@virtuozzo.com> --- 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)
Stefan Berger is, but I don't think he watches the list closely these days. I just found the original message (somehow it had been tossed into my spam folder) and Replied-All to it with Stefan Cc'ed.
ACK. I agree to the change. It seems to be possible to release the lock earlier.
Stefan
Shall I push it then?
Yes. Stefan
24.05.2016 13:56, Stefan Berger пишет:
On 05/24/2016 03:22 AM, Maxim Nestratov wrote:
20.04.2016 22:04, Stefan Berger пишет:
On 04/20/2016 12:06 PM, Laine Stump wrote:
On 04/19/2016 05:45 PM, Cole Robinson wrote:
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@virtuozzo.com> --- 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)
Stefan Berger is, but I don't think he watches the list closely these days. I just found the original message (somehow it had been tossed into my spam folder) and Replied-All to it with Stefan Cc'ed.
ACK. I agree to the change. It seems to be possible to release the lock earlier.
Stefan
Shall I push it then?
Yes.
Stefan
Pushed now. Thanks. Maxim
On Sat, Apr 09, 2016 at 07:14:30PM +0300, 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@virtuozzo.com> ---
The nwfilter code is complex. This seems to be fixing a small corner case because virDomainConfVMNWFilterTeardown should terminate that learnIPAddressThread, but it doesn't wait until that thread is terminated. I'm not sure, whether it's safe to unlock the iface. Do you have some reproducer for this deadlock? Pavel
20.04.2016 14:08, Pavel Hrdina пишет:
On Sat, Apr 09, 2016 at 07:14:30PM +0300, 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@virtuozzo.com> --- The nwfilter code is complex. This seems to be fixing a small corner case because virDomainConfVMNWFilterTeardown should terminate that learnIPAddressThread, but it doesn't wait until that thread is terminated. I'm not sure, whether it's safe to unlock the iface. Do you have some reproducer for this deadlock?
Pavel
No, I don't think it's only about a small corner case with virDomainConfVMNWFilterTeardown and learnIPAddressThread. It's more common case with learnIPAddressThread and *any* virNWFilterInstantiateFilter call. We would have had a corner case fix if we just waited for learnIPAddressThread completion in virDomainConfVMNWFilterTeardown. I don't have a reproducer in a distributable form. The issue was caught by our testing system which perfomed a kind of stress start/stop test. A VM had a network interface with <filterref filter='no-mac-spoofing-no-ip-spoofing-no-promisc'/> section without IP address specified. For me it's a kind of exotic configuration, nevertheless deadlock should not happen. I don't insint on my approach but it seems pretty safe to call virNWFilterInstantiateFilterLate with unlocked interface just because the function itself locks it when necessary. Maxim
Stefan - since you wrote the nwfilter driver, perhaps you could take a look at this? (assuming that it hasn't all paged out of your brain by now :-) 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@virtuozzo.com> --- 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);
participants (5)
-
Cole Robinson -
Laine Stump -
Maxim Nestratov -
Pavel Hrdina -
Stefan Berger