From: Ben Pfaff <blp@nicira.com>
Date: Wed, 8 Apr 2009 20:06:02 +0000 (-0700)
Subject: vswitch: Fix handling of ARPs received on bonded interfaces.
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ad51aaf63d5dc7beaf946e2818425265e5ca5825;p=openvswitch

vswitch: Fix handling of ARPs received on bonded interfaces.

The vswitch must handle ARPs directed to broadcast that arrived on bonded
interfaces differently based on whether they are ARP requests or replies.
This cannot be done in a flow-based manner using OpenFlow, because
OpenFlow does not distinguish between ARP requests and replies.  Thus,
every such packet must be handled separately by the bonding code, and a
flow must not be set up.

Before secchan was integrated into vswitch, this was handled correctly.
This commit restores that correct behavior, by making it possible for
a normal-action callback to signal that the actions must not be used to
set up a flow.
---

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index 6a5ef79b..c8a5e8b3 100644
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -93,8 +93,9 @@ static void hton_ofp_phy_port(struct ofp_phy_port *);
 
 static int xlate_actions(const union ofp_action *in, size_t n_in,
                          const flow_t *flow, struct ofproto *ofproto,
-                         bool revalidating,
-                         struct odp_actions *out, tag_type *tags);
+                         const struct ofpbuf *packet,
+                         struct odp_actions *out, tag_type *tags,
+                         bool *may_setup_flow);
 
 #define UNKNOWN_SUPER ((struct rule *)-1)
 struct rule {
@@ -135,6 +136,8 @@ struct rule {
      * A super-rule with wildcard fields never has ODP actions (since the
      * datapath only supports exact-match flows). */
     bool installed;             /* Installed in datapath? */
+    bool may_install;           /* True ordinarily; false if actions must
+                                 * be reassessed for every packet. */
     int n_odp_actions;
     union odp_action *odp_actions;
 };
@@ -165,7 +168,7 @@ static void rule_free(struct rule *);
 static void rule_destroy(struct rule *);
 static struct rule *rule_from_cls_rule(const struct cls_rule *);
 static bool rule_make_actions(struct ofproto *, struct rule *,
-                              bool revalidating);
+                              const struct ofpbuf *packet);
 static void rule_install(struct ofproto *, struct rule *,
                          struct odp_flow_stats *);
 static void rule_uninstall(struct ofproto *, struct rule *);
@@ -944,8 +947,8 @@ ofproto_send_packet(struct ofproto *p, const flow_t *flow,
     struct odp_actions odp_actions;
     int error;
 
-    error = xlate_actions(actions, n_actions, flow, p, false, &odp_actions,
-                          NULL);
+    error = xlate_actions(actions, n_actions, flow, p, packet, &odp_actions,
+                          NULL, NULL);
     if (error) {
         return error;
     }
@@ -975,7 +978,7 @@ ofproto_add_flow(struct ofproto *p,
     }
 
     if (!wildcards) {
-        rule_make_actions(p, rule, false);
+        rule_make_actions(p, rule, packet);
         if (packet
             && !dpif_execute(&p->dpif, flow->in_port,
                              rule->odp_actions, rule->n_odp_actions, packet)) {
@@ -1408,7 +1411,8 @@ rule_has_out_port(const struct rule *rule, uint16_t out_port)
 
 /* Returns true if the actions changed, false otherwise. */
 static bool
-rule_make_actions(struct ofproto *p, struct rule *rule, bool revalidating)
+rule_make_actions(struct ofproto *p, struct rule *rule,
+                  const struct ofpbuf *packet)
 {
     const struct rule *super;
     struct odp_actions a;
@@ -1420,7 +1424,7 @@ rule_make_actions(struct ofproto *p, struct rule *rule, bool revalidating)
     super = rule->super ? rule->super : rule;
     rule->tags = 0;
     xlate_actions(super->actions, super->n_actions, &rule->cr.flow, p,
-                  revalidating, &a, &rule->tags);
+                  packet, &a, &rule->tags, &rule->may_install);
 
     actions_len = a.n_actions * sizeof *a.actions;
     if (rule->n_odp_actions != a.n_actions
@@ -1438,21 +1442,38 @@ static void
 rule_install(struct ofproto *p, struct rule *rule,
              struct odp_flow_stats *stats)
 {
-    struct odp_flow odp_flow;
-
     assert(!rule->cr.wc.wildcards);
 
-    odp_flow.key = rule->cr.flow;
-    odp_flow.actions = rule->odp_actions;
-    odp_flow.n_actions = rule->n_odp_actions;
-    if (!dpif_flow_add(&p->dpif, &odp_flow)) {
-        rule->installed = true;
-        if (stats) {
-            *stats = odp_flow.stats;
+    if (rule->may_install) {
+        struct odp_flow odp_flow;
+
+        odp_flow.key = rule->cr.flow;
+        odp_flow.actions = rule->odp_actions;
+        odp_flow.n_actions = rule->n_odp_actions;
+        if (!dpif_flow_add(&p->dpif, &odp_flow)) {
+            rule->installed = true;
+            if (stats) {
+                *stats = odp_flow.stats;
+            }
         }
     }
 }
 
+static void
+rule_update(struct ofproto *p, struct rule *rule)
+{
+    if (rule->may_install) {
+        if (rule->installed) {
+            dpif_flow_set_actions(&p->dpif, &rule->cr.flow,
+                                  rule->odp_actions, rule->n_odp_actions);
+        } else {
+            rule_install(p, rule, NULL);
+        }
+    } else {
+        rule_uninstall(p, rule);
+    }
+}
+
 static void
 rule_uninstall(struct ofproto *p, struct rule *rule)
 {
@@ -1667,11 +1688,15 @@ struct action_xlate_ctx {
     const flow_t *flow;         /* Flow to which these actions correspond. */
     int recurse;                /* Recursion level, via xlate_table_action. */
     struct ofproto *ofproto;
-    bool revalidating;
+    const struct ofpbuf *packet; /* The packet corresponding to 'flow', or a
+                                  * null pointer if we are revalidating
+                                  * without a packet to refer to. */
 
     /* Output. */
     struct odp_actions *out;    /* Datapath actions. */
     tag_type *tags;             /* Tags associated with OFPP_NORMAL actions. */
+    bool may_setup_flow;        /* True ordinarily; false if the actions must
+                                 * be reassessed for every packet. */
 };
 
 static void do_xlate_actions(const union ofp_action *in, size_t n_in,
@@ -1732,9 +1757,11 @@ xlate_output_action(struct action_xlate_ctx *ctx,
         xlate_table_action(ctx, ctx->flow->in_port);
         break;
     case OFPP_NORMAL:
-        ctx->ofproto->ofhooks->normal_cb(ctx->flow, ctx->revalidating,
-                                         ctx->out, ctx->tags,
-                                         ctx->ofproto->aux);
+        if (!ctx->ofproto->ofhooks->normal_cb(ctx->flow, ctx->packet,
+                                              ctx->out, ctx->tags,
+                                              ctx->ofproto->aux)) {
+            ctx->may_setup_flow = false;
+        }
         break;
     case OFPP_FLOOD:
         add_output_group_action(ctx->out, DP_GROUP_FLOOD);
@@ -1850,8 +1877,9 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
 
 static int
 xlate_actions(const union ofp_action *in, size_t n_in,
-              const flow_t *flow, struct ofproto *ofproto, bool revalidating,
-              struct odp_actions *out, tag_type *tags)
+              const flow_t *flow, struct ofproto *ofproto,
+              const struct ofpbuf *packet,
+              struct odp_actions *out, tag_type *tags, bool *may_setup_flow)
 {
     tag_type no_tags = 0;
     struct action_xlate_ctx ctx;
@@ -1859,10 +1887,14 @@ xlate_actions(const union ofp_action *in, size_t n_in,
     ctx.flow = flow;
     ctx.recurse = 0;
     ctx.ofproto = ofproto;
-    ctx.revalidating = revalidating;
+    ctx.packet = packet;
     ctx.out = out;
     ctx.tags = tags ? tags : &no_tags;
+    ctx.may_setup_flow = true;
     do_xlate_actions(in, n_in, &ctx);
+    if (may_setup_flow) {
+        *may_setup_flow = ctx.may_setup_flow;
+    }
     if (odp_actions_overflow(out)) {
         odp_actions_init(out);
         return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_TOO_MANY);
@@ -1901,7 +1933,7 @@ handle_packet_out(struct ofproto *p, struct ofconn *ofconn,
 
     flow_extract(&payload, ofp_port_to_odp_port(ntohs(opo->in_port)), &flow);
     error = xlate_actions((const union ofp_action *) opo->actions, n_actions,
-                          &flow, p, false, &actions, NULL);
+                          &flow, p, &payload, &actions, NULL, NULL);
     if (error) {
         return error;
     }
@@ -2358,7 +2390,8 @@ update_stats(struct rule *rule, const struct odp_flow_stats *stats)
 
 static int
 send_buffered(struct ofproto *p, struct ofconn *ofconn, uint32_t buffer_id,
-              const struct rule *rule, int *byte_count)
+              const struct rule *rule,
+              struct ofpbuf **packetp, int *byte_count)
 {
     struct odp_actions actions;
     struct ofpbuf *packet;
@@ -2376,10 +2409,11 @@ send_buffered(struct ofproto *p, struct ofconn *ofconn, uint32_t buffer_id,
     if (error) {
         return error;
     }
+    *packetp = packet;
 
     flow_extract(packet, in_port, &flow);
-    error = xlate_actions(rule->actions, rule->n_actions, &flow, p, false,
-                          &actions, NULL);
+    error = xlate_actions(rule->actions, rule->n_actions, &flow, p, packet,
+                          &actions, NULL, NULL);
     if (error) {
         return error;
     }
@@ -2389,7 +2423,6 @@ send_buffered(struct ofproto *p, struct ofconn *ofconn, uint32_t buffer_id,
     if (!error) {
         *byte_count = packet->size;
     }
-    ofpbuf_delete(packet);
 
     return 0;
 }
@@ -2399,6 +2432,7 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
          struct ofp_flow_mod *ofm, size_t n_actions)
 {
     struct rule *rule, *displaced_rule;
+    struct ofpbuf *packet = NULL;
     int error = 0;
 
     rule = rule_create(NULL, (const union ofp_action *) ofm->actions,
@@ -2409,7 +2443,7 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
     if (ofm->buffer_id != htonl(UINT32_MAX)) {
         int byte_count = 0;
         error = send_buffered(p, ofconn, ntohl(ofm->buffer_id),
-                              rule, &byte_count);
+                              rule, &packet, &byte_count);
         rule->byte_count += byte_count;
         rule->packet_count += byte_count > 0;
     }
@@ -2437,7 +2471,7 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
     } else {
         struct odp_flow_stats stats;
 
-        rule_make_actions(p, rule, false);
+        rule_make_actions(p, rule, packet);
         rule_install(p, rule, &stats);
         if (displaced_rule) {
             if (displaced_rule->super &&
@@ -2447,6 +2481,7 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
             rule_destroy(displaced_rule);
         }
     }
+    ofpbuf_delete(packet);
     return error;
 }
 
@@ -2473,9 +2508,10 @@ modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm,
                                 n_actions * sizeof *rule->actions);
         rule->n_actions = n_actions;
         if (!rule->cr.wc.wildcards) {
-            if (rule_make_actions(p, rule, false)) {
-                dpif_flow_set_actions(&p->dpif, &rule->cr.flow,
-                                      rule->odp_actions, rule->n_odp_actions);
+            if (rule_make_actions(p, rule, NULL)) {
+                rule_update(p, rule);
+            } else if (rule->installed && !rule->may_install) {
+                rule_uninstall(p, rule);
             }
         }
     }
@@ -2819,12 +2855,20 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
         }
 
         /* Install flow entry into datapath. */
-        rule_make_actions(p, subrule, false);
+        rule_make_actions(p, subrule, packet);
         rule_install(p, subrule, NULL);
     } else {
-        /* A flow got dropped due to a hash collision, or the packet was
-         * buffered before we processed another packet from the same flow. */
         subrule = rule;
+        if (!rule->may_install) {
+            /* The rule is not installable, that is, we need to process every
+             * packet, so do that here. */
+            rule_make_actions(p, subrule, packet);
+            rule_install(p, subrule, NULL);
+        } else {
+            /* A flow got dropped due to a hash collision, or the packet was
+             * buffered before we processed another packet from the same
+             * flow. */
+        }
     }
 
     /* Execute subrule on packet. */
@@ -2872,9 +2916,10 @@ revalidate_rule(struct ofproto *p, struct rule *rule)
         }
     }
 
-    if (rule_make_actions(p, rule, true)) {
-        dpif_flow_set_actions(&p->dpif, flow, rule->odp_actions,
-                              rule->n_odp_actions);
+    if (rule_make_actions(p, rule, NULL)) {
+        rule_update(p, rule);
+    } else if (rule->installed && !rule->may_install) {
+        rule_uninstall(p, rule);
     }
     return true;
 }
@@ -3110,8 +3155,8 @@ pick_fallback_dpid(void)
     return eth_addr_to_uint64(ea);
 }
 
-static void
-default_normal_ofhook_cb(const flow_t *flow, bool revalidating,
+static bool
+default_normal_ofhook_cb(const flow_t *flow, const struct ofpbuf *packet,
                          struct odp_actions *actions, tag_type *tags,
                          void *ofproto_)
 {
@@ -3120,11 +3165,11 @@ default_normal_ofhook_cb(const flow_t *flow, bool revalidating,
 
     /* Drop frames for reserved multicast addresses. */
     if (eth_addr_is_reserved(flow->dl_dst)) {
-        return;
+        return true;
     }
 
     /* Learn source MAC (but don't try to learn from revalidation). */
-    if (!revalidating) {
+    if (packet != NULL) {
         tag_type rev_tag = mac_learning_learn(ofproto->ml, flow->dl_src,
                                               0, flow->in_port);
         if (rev_tag) {
@@ -3146,6 +3191,8 @@ default_normal_ofhook_cb(const flow_t *flow, bool revalidating,
     } else {
         /* Drop. */
     }
+
+    return true;
 }
 
 static const struct ofhooks default_ofhooks = {
diff --git a/secchan/ofproto.h b/secchan/ofproto.h
index a90f23f8..b0a54639 100644
--- a/secchan/ofproto.h
+++ b/secchan/ofproto.h
@@ -112,7 +112,7 @@ void ofproto_flush_flows(struct ofproto *);
 struct ofhooks {
     void (*port_changed_cb)(enum ofp_port_reason, const struct ofp_phy_port *,
                             void *aux);
-    void (*normal_cb)(const flow_t *, bool revalidating,
+    bool (*normal_cb)(const flow_t *, const struct ofpbuf *packet,
                       struct odp_actions *, tag_type *, void *aux);
 };
 void ofproto_revalidate(struct ofproto *, tag_type);
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 5b976abc..c88d8cf6 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1559,9 +1559,13 @@ compose_actions(struct bridge *br, const flow_t *flow, uint16_t vlan,
     }
 }
 
-static void
-process_flow(struct bridge *br, const flow_t *flow, bool revalidating,
-             struct odp_actions *actions, tag_type *tags)
+/* If the composed actions may be applied to any packet in the given 'flow',
+ * returns true.  Otherwise, the actions should only be applied to 'packet', or
+ * not at all, if 'packet' was NULL. */
+static bool
+process_flow(struct bridge *br, const flow_t *flow,
+             const struct ofpbuf *packet, struct odp_actions *actions,
+             tag_type *tags)
 {
     struct iface *in_iface;
     struct port *in_port;
@@ -1572,7 +1576,7 @@ process_flow(struct bridge *br, const flow_t *flow, bool revalidating,
     in_iface = iface_from_dp_ifidx(br, flow->in_port);
     if (!in_iface) {
         /* No interface?  Something fishy... */
-        if (!revalidating) {
+        if (packet != NULL) {
             /* Odd.  A few possible reasons here:
              *
              * - We deleted an interface but there are still a few packets
@@ -1591,7 +1595,7 @@ process_flow(struct bridge *br, const flow_t *flow, bool revalidating,
         }
 
         /* Return without adding any actions, to drop packets on this flow. */
-        return;
+        return true;
     }
     in_port = in_iface->port;
 
@@ -1609,7 +1613,7 @@ process_flow(struct bridge *br, const flow_t *flow, bool revalidating,
     if (in_port->vlan >= 0) {
         if (vlan) {
             /* XXX support double tagging? */
-            if (!revalidating) {
+            if (packet != NULL) {
                 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
                 VLOG_WARN_RL(&rl, "bridge %s: dropping VLAN %"PRIu16" tagged "
                              "packet received on port %s configured with "
@@ -1667,7 +1671,7 @@ process_flow(struct bridge *br, const flow_t *flow, bool revalidating,
         int out_port_idx;
         bool may_learn;
 
-        if (revalidating) {
+        if (!packet) {
             /* Don't try to learn from revalidation. */
             may_learn = false;
         } else if (in_port->n_ifaces > 1) {
@@ -1716,9 +1720,10 @@ process_flow(struct bridge *br, const flow_t *flow, bool revalidating,
         out_port = NULL;
     }
 
+done:
+    compose_actions(br, flow, vlan, in_port, out_port, tags, actions);
+
     /*
-     * Add a new flow.
-     *
      * We send out only a single packet, instead of setting up a flow, if:
      *
      *   - Flows are disabled entirely; or
@@ -1728,8 +1733,10 @@ process_flow(struct bridge *br, const flow_t *flow, bool revalidating,
      *     handled differently, but OpenFlow unfortunately can't distinguish
      *     them.
      */
-done:
-    compose_actions(br, flow, vlan, in_port, out_port, tags, actions);
+    return (br->flow_idle_time >= 0
+            && (in_port->n_ifaces < 2
+                || flow->dl_type != htons(ETH_TYPE_ARP)
+                || !eth_addr_is_broadcast(flow->dl_dst)));
 }
 
 /* Careful: 'opp' is in host byte order and opp->port_no is an OFP port
@@ -1769,8 +1776,8 @@ bridge_port_changed_ofhook_cb(enum ofp_port_reason reason,
     }
 }
 
-static void
-bridge_normal_ofhook_cb(const flow_t *flow, bool revalidating,
+static bool
+bridge_normal_ofhook_cb(const flow_t *flow, const struct ofpbuf *packet,
                         struct odp_actions *actions, tag_type *tags, void *br_)
 {
     struct bridge *br = br_;
@@ -1783,7 +1790,7 @@ bridge_normal_ofhook_cb(const flow_t *flow, bool revalidating,
     }
 #endif
 
-    process_flow(br, flow, revalidating, actions, tags);
+    return process_flow(br, flow, packet, actions, tags);
 }
 
 static struct ofhooks bridge_ofhooks = {