On 03/07/2014 09:15 AM, Daniel P. Berrange wrote:
For
https://bugzilla.redhat.com/show_bug.cgi?id=1066801
The nwfilter conf update mutex previously serialized updates to the internal data structures for firewall rules, and updates to the firewall itself. The latter was recently turned into a read/write lock, and filter instantiation allowed to proceed in parallel. It was believed that this was ok, since each filter is created on a seperate iptables/ebtables chain.
s/seperate/separate/
It turns out that there is a sutle lock ordering problem
s/sutle/subtle/
on virNWFilterObjPtr instances. __virNWFilterInstantiateFilter will hold a lock on the virNWFilterObjPtr it is instantiating. This in turn invokes virNWFilterInstantiate which then invokes virNWFilterDetermineMissingVarsRec which then invokes virNWFilterObjFindByName. This iterates over every single virNWFilterObjPtr in the list, locking them and checking their name. So if 2 or more threads try to instantiate a filter in parallel, they'll all hold 1 lock at the top level in the __virNWFilterInstantiateFilter method which will cause the other thread to deadlock in virNWFilterObjFindByName.
The fix is to add an exclusive mutex to serialize the execution of __virNWFilterInstantiateFilter.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/nwfilter/nwfilter_driver.c | 6 ++++-- src/nwfilter/nwfilter_gentech_driver.c | 34 ++++++++++++++++++++++++++++++++-- src/nwfilter/nwfilter_gentech_driver.h | 2 +- 3 files changed, 37 insertions(+), 5 deletions(-)
ACK with spelling fixes.
+/* Serializes instantiation of filters. This is neccessary
s/neccessary/necessary/
+ * to avoid lock ordering deadlocks. eg __virNWFilterInstantiateFilter + * will hold a lock on a virNWFilterObjPtr. This in turn invokes + * virNWFilterInstantiate which invokes virNWFilterDetermineMissingVarsRec + * which invokes virNWFilterObjFindByName. This iterates over every single + * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a + * filter in parallel, they'll both hold 1 lock at the top level in + * __virNWFilterInstantiateFilter which will cause the other thread + * to dead lock in virNWFilterObjFindByName.
s/dead lock/deadlock/
+ * + * XXX better long term solution is to make virNWFilterObjList use a + * hash table as is done for virDomainObjList. You can then get + * lockless lookup of objects by name. + */ +static virMutex updateMutex;
-void virNWFilterTechDriversInit(bool privileged) { +int virNWFilterTechDriversInit(bool privileged) {
While touching this, put { on its own line. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org