In current recirculation implementation, the flow misses (with
'recirc_id' set) are always looked up on the receiving bridge's
internal flow table. However, the bond port may actually reside
on another bridge which gets connected to the receiving bridge
via patch port. Since the recirculation rules are pushed to the
other bridge's internal table, the flow lookup on the receiving
bridge will match nothing but the drop rule, causing unexpected
packet drops.
This commit fixes the above bug via changing the lookup logic
such that the internal flow table is checked only when flow
miss handling reaches the bridge (ofproto) which owns the bond
port.
VMware-BZ: 1362178
Reported-by: Ansis Atteka <aatt...@nicira.com>
Signed-off-by: Alex Wang <al...@nicira.com>
---
ofproto/bond.c | 25 +++++++++++++++++++++++++
ofproto/bond.h | 1 +
ofproto/ofproto-dpif-xlate.c | 14 ++++++++++++--
ofproto/ofproto-dpif.c | 19 ++++++++++---------
4 files changed, 48 insertions(+), 11 deletions(-)
diff --git a/ofproto/bond.c b/ofproto/bond.c
index 44e9df1..93b28f8 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -946,6 +946,31 @@ bond_may_recirc(const struct bond *bond, uint32_t
*recirc_id,
}
}
+/* Given the unique 'recirc_id', finds the corresponding 'bond' and
+ * returns the 'bond->ofproto'. */
+struct ofproto_dpif *
+bond_get_ofproto_by_recirc_id(uint32_t recirc_id)
+{
+ struct bond *bond;
+
+ if (!recirc_id) {
+ goto err;
+ }
+
+ ovs_rwlock_rdlock(&rwlock);
+ HMAP_FOR_EACH (bond, hmap_node, all_bonds) {
+ if (bond->recirc_id == recirc_id) {
+ ovs_rwlock_unlock(&rwlock);
+
+ return bond->ofproto;
+ }
+ }
+ ovs_rwlock_unlock(&rwlock);
+
+err:
+ return NULL;
+}
+
void
bond_update_post_recirc_rules(struct bond* bond, const bool force)
{
diff --git a/ofproto/bond.h b/ofproto/bond.h
index d7cb715..b03eadf 100644
--- a/ofproto/bond.h
+++ b/ofproto/bond.h
@@ -124,4 +124,5 @@ void bond_rebalance(struct bond *);
void bond_update_post_recirc_rules(struct bond *, const bool force);
bool bond_may_recirc(const struct bond *, uint32_t *recirc_id,
uint32_t *hash_bias);
+struct ofproto_dpif *bond_get_ofproto_by_recirc_id(uint32_t recirc_id);
#endif /* bond.h */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 4f77ac5..de9fa70 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1846,8 +1846,16 @@ compose_output_action__(struct xlate_ctx *ctx,
ofp_port_t ofp_port,
const struct xport *peer = xport->peer;
struct flow old_flow = ctx->xin->flow;
enum slow_path_reason special;
+ uint8_t table_id = 0;
ctx->xbridge = peer->xbridge;
+ /* If 'recirc_id' is set and 'ofproto' is the same as the bond's
+ * 'ofproto', starts the look up from internal table. */
+ if (flow->recirc_id
+ && (peer->xbridge->ofproto
+ == bond_get_ofproto_by_recirc_id(flow->recirc_id))) {
+ table_id = TBL_INTERNAL;
+ }
flow->in_port.ofp_port = peer->ofp_port;
flow->metadata = htonll(0);
memset(&flow->tunnel, 0, sizeof flow->tunnel);
@@ -1859,14 +1867,16 @@ compose_output_action__(struct xlate_ctx *ctx,
ofp_port_t ofp_port,
ctx->xout->slow |= special;
} else if (may_receive(peer, ctx)) {
if (xport_stp_forward_state(peer)) {
- xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true);
+ xlate_table_action(ctx, flow->in_port.ofp_port, table_id,
+ true, true);
} else {
/* Forwarding is disabled by STP. Let OFPP_NORMAL and the
* learning action look at the packet, then drop it. */
struct flow old_base_flow = ctx->base_flow;
size_t old_size = ofpbuf_size(&ctx->xout->odp_actions);
mirror_mask_t old_mirrors = ctx->xout->mirrors;
- xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true);
+ xlate_table_action(ctx, flow->in_port.ofp_port, table_id,
+ true, true);
ctx->xout->mirrors = old_mirrors;
ctx->base_flow = old_base_flow;
ofpbuf_set_size(&ctx->xout->odp_actions, old_size);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 46595f8..bdc0f1c 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3192,7 +3192,7 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, struct
flow *flow,
{
enum rule_dpif_lookup_verdict verdict;
enum ofputil_port_config config = 0;
- uint8_t table_id;
+ uint8_t table_id = 0;
if (ofproto_dpif_get_enable_recirc(ofproto)) {
/* Always exactly match recirc_id since datapath supports
@@ -3201,17 +3201,18 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, struct
flow *flow,
wc->masks.recirc_id = UINT32_MAX;
}
- /* Start looking up from internal table for post recirculation flows
- * or packets. We can also simply send all, including normal flows
- * or packets to the internal table. They will not match any post
- * recirculation rules except the 'catch all' rule that resubmit
- * them to table 0.
+ /* If 'ofproto' is the same as 'bond's ofproto, starts looking up
+ * from internal table for post recirculation flows or packets.
+ * We can also simply send all, including normal flows or packets
+ * to the internal table. They will not match any post recirculation
+ * rules except the 'catch all' rule that resubmit them to table 0.
*
* As an optimization, we send normal flows and packets to table 0
* directly, saving one table lookup. */
- table_id = flow->recirc_id ? TBL_INTERNAL : 0;
- } else {
- table_id = 0;
+ if (flow->recirc_id
+ && ofproto == bond_get_ofproto_by_recirc_id(flow->recirc_id)) {
+ table_id = TBL_INTERNAL;
+ }
}
verdict = rule_dpif_lookup_from_table(ofproto, flow, wc, true,
--
1.7.9.5
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
