ofproto-dpif: Avoid segfault deleting facets that execute LEARN actions.

"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"

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 <mmao@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 54392aa..44810dd 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011 Nicira Networks.
+ * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -210,11 +210,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;
 
     /* If nonnull, called just before executing a resubmit action.
      *
@@ -338,7 +344,8 @@ static void facet_update_time(struct ofproto_dpif *, 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 ofproto_dpif *, struct facet *);
+static void facet_account(struct ofproto_dpif *, struct facet *,
+                          bool may_flow_mod);
 
 static bool facet_is_controller_flow(struct facet *);
 
@@ -2916,7 +2923,7 @@ update_stats(struct ofproto_dpif *p)
             subfacet->dp_byte_count = stats->n_bytes;
 
             subfacet_update_time(p, subfacet, stats->used);
-            facet_account(p, facet);
+            facet_account(p, facet, true);
             facet_push_stats(facet);
         } else {
             if (!VLOG_DROP_WARN(&rl)) {
@@ -3206,7 +3213,8 @@ facet_remove(struct ofproto_dpif *ofproto, struct facet *facet)
 }
 
 static void
-facet_account(struct ofproto_dpif *ofproto, struct facet *facet)
+facet_account(struct ofproto_dpif *ofproto, struct facet *facet,
+              bool may_flow_mod)
 {
     uint64_t n_bytes;
     struct subfacet *subfacet;
@@ -3228,7 +3236,8 @@ facet_account(struct ofproto_dpif *ofproto, struct facet *facet)
 
         action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
                               facet->flow.vlan_tci, 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));
     }
@@ -3301,7 +3310,7 @@ facet_flush_stats(struct ofproto_dpif *ofproto, struct facet *facet)
     }
 
     facet_push_stats(facet);
-    facet_account(ofproto, facet);
+    facet_account(ofproto, facet, false);
 
     if (ofproto->netflow && !facet_is_controller_flow(facet)) {
         struct ofexpired expired;
@@ -4691,7 +4700,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;
@@ -4721,7 +4730,8 @@ action_xlate_ctx_init(struct action_xlate_ctx *ctx,
     ctx->base_flow.tun_id = 0;
     ctx->base_flow.vlan_tci = initial_tci;
     ctx->packet = packet;
-    ctx->may_learn = packet != NULL;
+    ctx->may_learn_macs = packet != NULL;
+    ctx->may_flow_mod = packet != NULL;
     ctx->resubmit_hook = NULL;
 }
 
@@ -5328,7 +5338,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);
     }