From: Ben Pfaff Date: Wed, 21 Mar 2012 16:01:02 +0000 (-0700) Subject: ofproto-dpif: Avoid segfault deleting facets that execute LEARN actions. X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=805e4d07ecd8ce86c6f36bdb83d2e7fee56891c7;p=openvswitch ofproto-dpif: Avoid segfault deleting facets that execute LEARN actions. "ovs-ofctl del-flows " 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" This commit has a side effect that leftover unaccounted packets no longer update the timeouts in MAC learning actions in some cases, when the facets that cause updates are deleted. At most one second of updates should be lost. Bug #10184. Reported-by: Michael Mao Signed-off-by: Ben Pfaff --- diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 200846b7..2afd94a9 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 may_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 may_flow_mod; /* 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_flow_mod); 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_flow_mod) { 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.may_learn_macs = true; + ctx.may_flow_mod = may_flow_mod; 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->may_flow_mod) { 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->may_learn_macs = packet != NULL; + ctx->may_flow_mod = 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->may_learn_macs) { update_learning_table(ctx->ofproto, &ctx->flow, vlan, in_bundle); }