ofproto/bond: Fix a race condition in updating post recirculation rules

When updating post recirc rules, rule management requires calls to
hmap APIs, which requires proper locking to ensure mutual exclsion in
accessing the hmap internal data structure. The locking currently is
missing from the output_normal() xlate path, thus causing
a race condition.

The race condition leads to segfault crash of ovs-vswitchd, with the
following stack trace:

The crash was found by adding and deleting bond interfaces repeatedly
with on-going traffic hitting the bond interfaces.  The same test was
ran over multiple days with this patch to ensure the same crash was
not seen.

The patch added the necessary lock annotation that would have caught
the bug.

Tested-by: Salvatore Cambria <salvatore.cambria@citrix.com>
Reported-by: Salvatore Cambria <salvatore.cambria@citrix.com>
Signed-off-by: Andy Zhou <azhou@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>

diff --git a/AUTHORS b/AUTHORS
index 4970973..5a0998f 100644 (file)
--- a/AUTHORS
+++ b/AUTHORS
@@ -266,6 +266,7 @@ Spiro Kourtessis        spiro@vmware.com
 Sridhar Samudrala       samudrala.sridhar@gmail.com
 Srini Seetharaman       seethara@stanford.edu
 Sabyasachi Sengupta     Sabyasachi.Sengupta@alcatel-lucent.com
+Salvatore Cambria       salvatore.cambria@citrix.com
 Stephen Hemminger       shemminger@vyatta.com
 Stephen Finucane        stephen.finucane@intel.com
 Suganya Ramachandran    suganyar@vmware.com

diff --git a/ofproto/bond.c b/ofproto/bond.c
index 44e9df1..aa34a83 100644 (file)
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -325,6 +325,7 @@ add_pr_rule(struct bond *bond, const struct match *match,
 
 static void
 update_recirc_rules(struct bond *bond)
+    OVS_REQ_WRLOCK(rwlock)
 {
     struct match match;
     struct bond_pr_rule_op *pr_op, *next_op;
@@ -946,8 +947,9 @@ bond_may_recirc(const struct bond *bond, uint32_t *recirc_id,
     }
 }
 
-void
-bond_update_post_recirc_rules(struct bond* bond, const bool force)
+static void
+bond_update_post_recirc_rules__(struct bond* bond, const bool force)
+    OVS_REQ_WRLOCK(rwlock)
 {
    struct bond_entry *e;
    bool update_rules = force;  /* Always update rules if caller forces it. */
@@ -968,6 +970,14 @@ bond_update_post_recirc_rules(struct bond* bond, const bool force)
         update_recirc_rules(bond);
    }
 }
+
+void
+bond_update_post_recirc_rules(struct bond* bond, const bool force)
+{
+    ovs_rwlock_wrlock(&rwlock);
+    bond_update_post_recirc_rules__(bond, force);
+    ovs_rwlock_unlock(&rwlock);
+}
 \f
 /* Rebalancing. */
 
@@ -1226,7 +1236,7 @@ bond_rebalance(struct bond *bond)
     }
 
     if (use_recirc && rebalanced) {
-        bond_update_post_recirc_rules(bond,true);
+        bond_update_post_recirc_rules__(bond,true);
     }
 
 done: