bond: Avoid deadlock while updating post recirculation rules.

If the PACKET_OUT from controller ends up with sending packet to
a bond interface, the main thread will take locks in the following
order:
  handle_openflow
  --> take ofproto_mutex
  handle_packet_out
  packet_xlate
  output_normal
  bond_update_post_recirc_rules
  --> take rwlock in bond.c

If at the same time revalidator thread is processing other packet
with the output to the same bond:
  xlate_actions
  output_normal
  bond_update_post_recirc_rules
  --> take rwlock in bond.c
  update_recirc_rules
  ofproto_dpif_add_internal_flow
  ofproto_flow_mod
  --> take ofproto_mutex

So, it is possible for these 2 threads to lock each other by
taking one lock and waiting for another thread to release the
second lock.

It is also possible for the main thread to lock itself up by trying
to acquire ofproto_mutex for the second time, if it will actually
proceed with update_recirc_rules() after taking the bond rwlock.

The problem appears to be that bond_update_post_recirc_rules()
is called during the flow translation even if side effects are
prohibited, which is the case for openflow PACKET_OUT handling.

Skipping actual flow updates during the flow translation if
side effects are disabled to avoid the deadlock.

Since flows are not installed now when actions translated for
very first packet, installing initial flows in bond_reconfigure().
This will cover the case of allocating a new recirc_id.

Also checking if we need to update flows in bond_run() to cover
link state changes.

Regression test is added to catch the double lock case.

Reported-at: https://github.com/openvswitch/ovs-issues/issues/259
Reported-by: Daniel Ding <zhihui.ding@easystack.cn>
Fixes: adcf00ba35a0 ("ofproto/bond: Implement bond megaflow using recirculation")
Acked-by: Mike Pattrick <mkp@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

3 files changed, 44 insertions, 3 deletions

diff --git a/ofproto/bond.c b/ofproto/bond.c
index f06cf20c9..47630a6b0 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -187,10 +187,14 @@ static struct bond_member *choose_output_member(const struct bond *,
                                                 uint16_t vlan)
     OVS_REQ_RDLOCK(rwlock);
 static void update_recirc_rules__(struct bond *);
+static bool bond_may_recirc(const struct bond *);
+static void bond_update_post_recirc_rules__(struct bond *, bool force)
+    OVS_REQ_WRLOCK(rwlock);
 static bool bond_is_falling_back_to_ab(const struct bond *);
 static void bond_add_lb_output_buckets(const struct bond *);
 static void bond_del_lb_output_buckets(const struct bond *);
 
+
 /* Attempts to parse 's' as the name of a bond balancing mode.  If successful,
  * stores the mode in '*balance' and returns true.  Otherwise returns false
  * without modifying '*balance'. */
@@ -515,6 +519,12 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s)
         bond_entry_reset(bond);
     }
 
+    if (bond->ofproto->backer->rt_support.odp.recirc
+        && bond_may_recirc(bond)) {
+        /* Update rules to reflect possible recirc_id changes. */
+        update_recirc_rules(bond);
+    }
+
     ovs_rwlock_unlock(&rwlock);
     return revalidate;
 }
@@ -728,6 +738,12 @@ bond_run(struct bond *bond, enum lacp_status lacp_status)
         bond_choose_active_member(bond);
     }
 
+    if (bond->ofproto->backer->rt_support.odp.recirc
+        && bond_may_recirc(bond)) {
+        /* Update rules to reflect possible link state changes. */
+        bond_update_post_recirc_rules__(bond, false);
+    }
+
     revalidate = bond->bond_revalidate;
     bond->bond_revalidate = false;
     ovs_rwlock_unlock(&rwlock);
@@ -1091,6 +1107,19 @@ bond_update_post_recirc_rules(struct bond *bond, uint32_t *recirc_id,
     }
 }
 
+void
+bond_get_recirc_id_and_hash_basis(struct bond *bond, uint32_t *recirc_id,
+                                  uint32_t *hash_basis)
+{
+    ovs_rwlock_rdlock(&rwlock);
+    if (bond_may_recirc(bond)) {
+        *recirc_id = bond->recirc_id;
+        *hash_basis = bond->basis;
+    } else {
+        *recirc_id = *hash_basis = 0;
+    }
+    ovs_rwlock_unlock(&rwlock);
+}
 
 /* Rebalancing. */
 
diff --git a/ofproto/bond.h b/ofproto/bond.h
index 2eb0c95a7..3aa5c640d 100644
--- a/ofproto/bond.h
+++ b/ofproto/bond.h
@@ -131,6 +131,9 @@ void bond_rebalance(struct bond *);
 void bond_update_post_recirc_rules(struct bond *, uint32_t *recirc_id,
                                    uint32_t *hash_basis);
 
+void bond_get_recirc_id_and_hash_basis(struct bond *, uint32_t *recirc_id,
+                                       uint32_t *hash_basis);
+
 bool bond_use_lb_output_action(const struct bond *bond);
 
 #endif /* bond.h */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 2b1ed9cfe..ab6f39bb2 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2477,9 +2477,18 @@ output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle,
             /* In case recirculation is not actually in use, 'xr.recirc_id'
              * will be set to '0', since a valid 'recirc_id' can
              * not be zero.  */
-            bond_update_post_recirc_rules(out_xbundle->bond,
-                                          &xr.recirc_id,
-                                          &xr.hash_basis);
+            if (ctx->xin->allow_side_effects) {
+                bond_update_post_recirc_rules(out_xbundle->bond,
+                                              &xr.recirc_id,
+                                              &xr.hash_basis);
+            } else {
+                /* If side effects are not allowed, only getting the bond
+                 * configuration.  Rule updates will be handled by the
+                 * main thread later. */
+                bond_get_recirc_id_and_hash_basis(out_xbundle->bond,
+                                                  &xr.recirc_id,
+                                                  &xr.hash_basis);
+            }
             if (xr.recirc_id) {
                 /* Use recirculation instead of output. */
                 use_recirc = true;