ofproto/bond: Fix bond reconfiguration race condition.

During the upcall thread bond output translation, bond_may_recirc()
is currently called outside the lock. In case the main thread executes
bond_reconfigure() at the same time, the upcall thread may find bond
state to be inconsistent when calling bond_update_post_recirc_rules().

This patch fixes the race condition by acquiring the write lock
before calling bond_may_recirc(). The APIs are refactored slightly.

The race condition can result in the following stack trace. Copied
from 'Reported-at':

    Thread 23 handler69:
    Invalid write of size 8
        update_recirc_rules (bond.c:385)
        bond_update_post_recirc_rules__ (bond.c:952)
        bond_update_post_recirc_rules (bond.c:960)
        output_normal (ofproto-dpif-xlate.c:2102)
        xlate_normal (ofproto-dpif-xlate.c:2858)
        xlate_output_action (ofproto-dpif-xlate.c:4407)
        do_xlate_actions (ofproto-dpif-xlate.c:5335)
        xlate_actions (ofproto-dpif-xlate.c:6198)
        upcall_xlate (ofproto-dpif-upcall.c:1129)
        process_upcall (ofproto-dpif-upcall.c:1271)
        recv_upcalls (ofproto-dpif-upcall.c:822)
        udpif_upcall_handler (ofproto-dpif-upcall.c:740)
    Address 0x18630490 is 1,904 bytes inside a block of size 12,288 free'd
        free (vg_replace_malloc.c:529)
        bond_entry_reset (bond.c:1635)
        bond_reconfigure (bond.c:457)
        bundle_set (ofproto-dpif.c:2896)
        ofproto_bundle_register (ofproto.c:1343)
        port_configure (bridge.c:1159)
        bridge_reconfigure (bridge.c:785)
        bridge_run (bridge.c:3099)
        main (ovs-vswitchd.c:111)
    Block was alloc'd at
        malloc (vg_replace_malloc.c:298)
        xmalloc (util.c:110)
        bond_entry_reset (bond.c:1629)
        bond_reconfigure (bond.c:457)
        bond_create (bond.c:245)
        bundle_set (ofproto-dpif.c:2900)
        ofproto_bundle_register (ofproto.c:1343)
        port_configure (bridge.c:1159)
        bridge_reconfigure (bridge.c:785)
        bridge_run (bridge.c:3099)
        main (ovs-vswitchd.c:111)

Reported-by: Huanle Han <hanxueluo@gmail.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-February/328969.html
CC: Huanle Han <hanxueluo@gmail.com>
Signed-off-by: Andy Zhou <azhou@ovn.org>
Acked-by: Jarno Rajahalme <jarno@ovn.org>
Acked-by: Huanle Han <hanxueluo@gmail.com>

1 files changed, 15 insertions, 12 deletions

diff --git a/ofproto/bond.c b/ofproto/bond.c
index 260023e4b..6e10c5143 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -916,17 +916,16 @@ bool
 bond_may_recirc(const struct bond *bond, uint32_t *recirc_id,
                 uint32_t *hash_bias)
 {
-    if (bond->balance == BM_TCP && bond->recirc_id) {
-        if (recirc_id) {
-            *recirc_id = bond->recirc_id;
-        }
-        if (hash_bias) {
-            *hash_bias = bond->basis;
-        }
-        return true;
-    } else {
-        return false;
+    bool may_recirc = bond->balance == BM_TCP && bond->recirc_id;
+
+    if (recirc_id) {
+        *recirc_id = may_recirc ? bond->recirc_id : 0;
     }
+    if (hash_bias) {
+        *hash_bias = may_recirc ? bond->basis : 0;
+    }
+
+    return may_recirc;
 }
 
 static void
@@ -954,12 +953,16 @@ bond_update_post_recirc_rules__(struct bond* bond, const bool force)
 }
 
 void
-bond_update_post_recirc_rules(struct bond* bond, const bool force)
+bond_update_post_recirc_rules(struct bond *bond, uint32_t *recirc_id,
+                              uint32_t *hash_basis)
 {
     ovs_rwlock_wrlock(&rwlock);
-    bond_update_post_recirc_rules__(bond, force);
+    if (bond_may_recirc(bond, recirc_id, hash_basis)) {
+        bond_update_post_recirc_rules__(bond, false);
+    }
     ovs_rwlock_unlock(&rwlock);
 }
+
 
 /* Rebalancing. */