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 <hanxue...@gmail.com>
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-February/328969.html
CC: Huanle Han <hanxue...@gmail.com>
Signed-off-by: Andy Zhou <az...@ovn.org>
---
ofproto/bond.c | 27 +++++++++++++++------------
ofproto/bond.h | 3 ++-
ofproto/ofproto-dpif-xlate.c | 24 ++++++++++++++++--------
3 files changed, 33 insertions(+), 21 deletions(-)
diff --git a/ofproto/bond.c b/ofproto/bond.c
index 260023e4bb64..6e10c5143c0e 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. */
diff --git a/ofproto/bond.h b/ofproto/bond.h
index 9a5ea9e21040..6e1221d2381b 100644
--- a/ofproto/bond.h
+++ b/ofproto/bond.h
@@ -120,7 +120,8 @@ void bond_rebalance(struct bond *);
* Bond module pulls stats from those post recirculation rules. If rebalancing
* is needed, those rules are updated with new output actions.
*/
-void bond_update_post_recirc_rules(struct bond *, const bool force);
+void bond_update_post_recirc_rules(struct bond *, uint32_t *recirc_id,
+ uint32_t *hash_basis);
bool bond_may_recirc(const struct bond *, uint32_t *recirc_id,
uint32_t *hash_bias);
#endif /* bond.h */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 503a347fc98f..89fc3a44a0d1 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1987,15 +1987,23 @@ output_normal(struct xlate_ctx *ctx, const struct
xbundle *out_xbundle,
struct flow_wildcards *wc = ctx->wc;
struct ofport_dpif *ofport;
- if (ctx->xbridge->support.odp.recirc) {
- use_recirc = bond_may_recirc(
- out_xbundle->bond, &xr.recirc_id, &xr.hash_basis);
-
- if (use_recirc) {
- /* Only TCP mode uses recirculation. */
+ if (ctx->xbridge->support.odp.recirc
+ && bond_may_recirc(out_xbundle->bond, NULL, NULL)) {
+ /* To avoid unnecessary locking, bond_may_recirc() is first
+ * called outside of the 'rwlock'. After acquiring the lock,
+ * bond_update_post_recirc_rules() will check again to make
+ * sure bond configuration has not been changed.
+ *
+ * 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 (xr.recirc_id) {
+ /* Use recirculation instead of output. */
+ use_recirc = true;
xr.hash_alg = OVS_HASH_ALG_L4;
- bond_update_post_recirc_rules(out_xbundle->bond, false);
-
/* Recirculation does not require unmasking hash fields. */
wc = NULL;
}
--
1.9.1
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
