"ovs-ofctl del-flows <bridge>" can result in the following call path:
delete_flows_loose() in ofproto.c
-> collect_rules_loose() -- uses 'ofproto_node' inside 'struct rule'
-> rule_destruct() in ofproto-dpif.c
-> facet_revalidate()
-> facet_remove()
-> facet_flush_stats()
-> facet_account()
-> xlate_actions()
-> xlate_learn_action()
-> ofproto_flow_mod() back in ofproto.c
-> modify_flow_strict()
-> collect_rules_strict() -- also uses 'ofproto_node'
which goes "boom" when we fall back up the call chain because the nested
use of ofproto_node steps on the outer use of ofproto_node.
This commit fixes the problem by refusing to translate "learn" actions
within facet_flush_stats(), breaking the doubled use.
Another possible approach would be to switch to another way to keep track
of rules in the flow_mod implementations, so that there'd be no fighting
over 'ofproto_node'. But then "ovs-ofctl del-flows" might still leave some
flows around (ones created by "learn" actions as flows are accounted as
facets get deleted), which would be surprising behavior. And it seems in
general a bad idea to allow recursive flow_mods; the consequences have not
been carefully thought through.
Before this commit, one can reproduce the problem by running an "hping3"
between a couple of VMs plus the following commands on OVS in the middle.
Sometimes you have to run them a few times:
ovs-ofctl del-flows br0
ovs-ofctl add-flow br0 "table=0 actions=learn(table=1, \
idle_timeout=600, NXM_OF_VLAN_TCI[0..11], \
NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], \
output:NXM_OF_IN_PORT[], fin_idle_timeout=10), resubmit(,1)"
ovs-ofctl add-flow br0 "table=1 priority=0 actions=flood"
Bug #10184.
Reported-by: Michael Mao <[email protected]>
Signed-off-by: Ben Pfaff <[email protected]>
---
ofproto/ofproto-dpif.c | 34 +++++++++++++++++++++-------------
1 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 200846b..0ffc4e8 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -209,11 +209,17 @@ struct action_xlate_ctx {
* revalidating without a packet to refer to. */
const struct ofpbuf *packet;
- /* Should OFPP_NORMAL MAC learning and NXAST_LEARN actions execute? We
- * want to execute them if we are actually processing a packet, or if we
- * are accounting for packets that the datapath has processed, but not if
- * we are just revalidating. */
- bool may_learn;
+ /* Should OFPP_NORMAL update the MAC learning table? We want to update it
+ * if we are actually processing a packet, or if we are accounting for
+ * packets that the datapath has processed, but not if we are just
+ * revalidating. */
+ bool do_learn_macs;
+
+ /* Should "learn" actions update the flow table? We want to update it if
+ * we are actually processing a packet, or in most cases if we are
+ * accounting for packets that the datapath has processed, but not if we
+ * are just revalidating. */
+ bool do_learn_action;
/* The rule that we are currently translating, or NULL. */
struct rule_dpif *rule;
@@ -346,7 +352,7 @@ static void facet_flush_stats(struct facet *);
static void facet_update_time(struct facet *, long long int used);
static void facet_reset_counters(struct facet *);
static void facet_push_stats(struct facet *);
-static void facet_account(struct facet *);
+static void facet_account(struct facet *, bool may_add_flows);
static bool facet_is_controller_flow(struct facet *);
@@ -2989,7 +2995,7 @@ update_stats(struct ofproto_dpif *p)
facet->tcp_flags |= stats->tcp_flags;
subfacet_update_time(subfacet, stats->used);
- facet_account(facet);
+ facet_account(facet, true);
facet_push_stats(facet);
} else {
if (!VLOG_DROP_WARN(&rl)) {
@@ -3241,7 +3247,7 @@ facet_remove(struct facet *facet)
}
static void
-facet_account(struct facet *facet)
+facet_account(struct facet *facet, bool may_add_flows)
{
struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
uint64_t n_bytes;
@@ -3267,7 +3273,8 @@ facet_account(struct facet *facet)
action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
facet->flow.vlan_tci,
facet->rule, facet->tcp_flags, NULL);
- ctx.may_learn = true;
+ ctx.do_learn_macs = true;
+ ctx.do_learn_action = may_add_flows;
ofpbuf_delete(xlate_actions(&ctx, facet->rule->up.actions,
facet->rule->up.n_actions));
}
@@ -3341,7 +3348,7 @@ facet_flush_stats(struct facet *facet)
}
facet_push_stats(facet);
- facet_account(facet);
+ facet_account(facet, false);
if (ofproto->netflow && !facet_is_controller_flow(facet)) {
struct ofexpired expired;
@@ -4964,7 +4971,7 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
case OFPUTIL_NXAST_LEARN:
ctx->has_learn = true;
- if (ctx->may_learn) {
+ if (ctx->do_learn_action) {
xlate_learn_action(ctx, (const struct nx_action_learn *) ia);
}
break;
@@ -5017,7 +5024,8 @@ action_xlate_ctx_init(struct action_xlate_ctx *ctx,
ctx->base_flow.vlan_tci = initial_tci;
ctx->rule = rule;
ctx->packet = packet;
- ctx->may_learn = packet != NULL;
+ ctx->do_learn_macs = packet != NULL;
+ ctx->do_learn_action = packet != NULL;
ctx->tcp_flags = tcp_flags;
ctx->resubmit_hook = NULL;
}
@@ -5645,7 +5653,7 @@ xlate_normal(struct action_xlate_ctx *ctx)
}
/* Learn source MAC. */
- if (ctx->may_learn) {
+ if (ctx->do_learn_macs) {
update_learning_table(ctx->ofproto, &ctx->flow, vlan, in_bundle);
}
--
1.7.2.5
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
