[libvirt] [PATCH] nwfilter: re-order lock grabbed by IP addr. learn thread

The IP address learning thread was causing a deadlock when it instantiated a filter while a filter update/change was ongoing. The reason for this was the ordering of locks due to the following calls virNWFilterUnlockFilterUpdates() virNWFilterPoolObjFindByName() The below patch now puts the order of the locks in the above shown order when instantiating the filter from the IP address learning thread. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/nwfilter/nwfilter_gentech_driver.c | 45 ++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 17 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c @@ -552,6 +552,8 @@ virNWFilterRuleInstancesToArray(int nEnt * all its subfilters in a depth-first traversal of the tree of referenced * filters. The name of the interface to which the rules belong must be * provided. Apply the values of variables as needed. + * + * Call this function while holding the NWFilter filter update lock */ static int virNWFilterInstantiate(virConnectPtr conn, @@ -575,8 +577,6 @@ virNWFilterInstantiate(virConnectPtr con void **ptrs = NULL; int instantiate = 1; - virNWFilterLockFilterUpdates(); - virNWFilterHashTablePtr missing_vars = virNWFilterHashTableCreate(0); if (!missing_vars) { virReportOOMError(); @@ -668,8 +668,6 @@ virNWFilterInstantiate(virConnectPtr con err_exit: - virNWFilterUnlockFilterUpdates(); - for (j = 0; j < nEntries; j++) virNWFilterRuleInstFree(insts[j]); @@ -681,6 +679,7 @@ err_exit: } + * Call this function while holding the NWFilter filter update lock static int __virNWFilterInstantiateFilter(virConnectPtr conn, bool teardownOld, @@ -823,23 +822,30 @@ _virNWFilterInstantiateFilter(virConnect ? net->data.direct.linkdev : NULL; int ifindex; + int rc; if (ifaceGetIndex(true, net->ifname, &ifindex)) return 1; - return __virNWFilterInstantiateFilter(conn, - teardownOld, - net->ifname, - ifindex, - linkdev, - net->type, - net->mac, - net->filter, - net->filterparams, - useNewFilter, - conn->nwfilterPrivateData, - false, - foundNewFilter); + virNWFilterLockFilterUpdates(); + + rc = __virNWFilterInstantiateFilter(conn, + teardownOld, + net->ifname, + ifindex, + linkdev, + net->type, + net->mac, + net->filter, + net->filterparams, + useNewFilter, + conn->nwfilterPrivateData, + false, + foundNewFilter); + + virNWFilterUnlockFilterUpdates(); + + return rc; } @@ -857,6 +863,8 @@ virNWFilterInstantiateFilterLate(virConn int rc; bool foundNewFilter = false; + virNWFilterLockFilterUpdates(); + rc = __virNWFilterInstantiateFilter(conn, 1, ifname, @@ -878,6 +886,9 @@ virNWFilterInstantiateFilterLate(virConn _virNWFilterTeardownFilter(ifname); } } + + virNWFilterUnlockFilterUpdates(); + return rc; }

On 11/18/2010 05:16 AM, Stefan Berger wrote:
The IP address learning thread was causing a deadlock when it instantiated a filter while a filter update/change was ongoing. The reason for this was the ordering of locks due to the following calls
virNWFilterUnlockFilterUpdates() virNWFilterPoolObjFindByName()
+ * Call this function while holding the NWFilter filter update lock static int __virNWFilterInstantiateFilter(virConnectPtr conn,
I'm assuming that's a bogus line in your patch,
bool teardownOld, @@ -823,23 +822,30 @@ _virNWFilterInstantiateFilter(virConnect ? net->data.direct.linkdev : NULL; int ifindex; + int rc;
if (ifaceGetIndex(true, net->ifname, &ifindex)) return 1;
...
+ virNWFilterLockFilterUpdates(); + + rc = __virNWFilterInstantiateFilter(conn,
...especially given the fact that you grab the lock here, so __virNWFilterInstantiateFilter should NOT have the filter update lock in the caller. ACK, once you fix that compilation error due to the stray line. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 11/19/2010 02:49 PM, Eric Blake wrote:
The IP address learning thread was causing a deadlock when it instantiated a filter while a filter update/change was ongoing. The reason for this was the ordering of locks due to the following calls
virNWFilterUnlockFilterUpdates() virNWFilterPoolObjFindByName()
+ * Call this function while holding the NWFilter filter update lock static int __virNWFilterInstantiateFilter(virConnectPtr conn, I'm assuming that's a bogus line in your patch, Yes, incomplete conversion from // to /* style comment. Fixed. bool teardownOld, @@ -823,23 +822,30 @@ _virNWFilterInstantiateFilter(virConnect ? net->data.direct.linkdev : NULL; int ifindex; + int rc;
if (ifaceGetIndex(true, net->ifname,&ifindex)) return 1; ... + virNWFilterLockFilterUpdates(); + + rc = __virNWFilterInstantiateFilter(conn, ...especially given the fact that you grab the lock here, so __virNWFilterInstantiateFilter should NOT have the filter update lock in
On 11/18/2010 05:16 AM, Stefan Berger wrote: the caller.
ACK, once you fix that compilation error due to the stray line.
Pushed. Stefan
participants (2)
-
Eric Blake
-
Stefan Berger