LGTM, Acked-by: Jarno Rajahalme <[email protected]>
> On Feb 23, 2017, at 1:31 PM, Andy Zhou <[email protected]> wrote: > > 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 <[email protected]> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-February/328969.html > CC: Huanle Han <[email protected]> > Signed-off-by: Andy Zhou <[email protected]> > --- > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
