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@xxxxxxxxxx> > --- > 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list