[libvirt] [PATCH] Add a mutex to serialize updates to firewall

[Eric Blake]

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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140307/9210721f/attachment-0001.sig>