[ovs-dev] [layering 2/3] ofproto: Fix layer violation in in-band control.

Ben Pfaff blp at nicira.com
Thu Sep 29 22:40:22 UTC 2011


ofproto/in-band.[ch] is supposed to be a fairly high-level interface.  It
has no business poking around ODP actions, and yet in_band_rule_check() did
just that.  This commit fixes the problem by making the caller do the work
related to actions.
---
 ofproto/connmgr.c      |    6 ++----
 ofproto/connmgr.h      |    5 +----
 ofproto/in-band.c      |   35 ++++++++++++-----------------------
 ofproto/in-band.h      |    3 +--
 ofproto/ofproto-dpif.c |   32 +++++++++++++++++++++++++-------
 5 files changed, 41 insertions(+), 40 deletions(-)

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 2d0b8c5..b9736cb 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1391,11 +1391,9 @@ connmgr_msg_in_hook(struct connmgr *mgr, const struct flow *flow,
 }
 
 bool
-connmgr_may_set_up_flow(struct connmgr *mgr, const struct flow *flow,
-                        const struct nlattr *odp_actions,
-                        size_t actions_len)
+connmgr_may_set_up_flow(struct connmgr *mgr, const struct flow *flow)
 {
-    return !mgr->in_band || in_band_rule_check(flow, odp_actions, actions_len);
+    return !mgr->in_band || in_band_rule_check(flow);
 }
 
 /* Fail-open and in-band implementation. */
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 0224352..b6129fb 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -23,7 +23,6 @@
 #include "openflow/nicira-ext.h"
 #include "openvswitch/types.h"
 
-struct nlattr;
 struct ofconn;
 struct ofopgroup;
 struct ofputil_flow_removed;
@@ -131,9 +130,7 @@ void connmgr_set_in_band_queue(struct connmgr *, int queue_id);
 /* In-band implementation. */
 bool connmgr_msg_in_hook(struct connmgr *, const struct flow *,
                          const struct ofpbuf *packet);
-bool connmgr_may_set_up_flow(struct connmgr *, const struct flow *,
-                             const struct nlattr *odp_actions,
-                             size_t actions_len);
+bool connmgr_may_set_up_flow(struct connmgr *, const struct flow *);
 
 /* Fail-open and in-band implementation. */
 void connmgr_flushed(struct connmgr *);
diff --git a/ofproto/in-band.c b/ofproto/in-band.c
index 13093e0..ec60e2e 100644
--- a/ofproto/in-band.c
+++ b/ofproto/in-band.c
@@ -251,31 +251,20 @@ in_band_msg_in_hook(struct in_band *in_band, const struct flow *flow,
     return false;
 }
 
-/* Returns true if the rule that would match 'flow' with 'actions' is
- * allowed to be set up in the datapath. */
+/* Returns true if the rule that would match 'flow' with 'actions' is allowed
+ * to be set up in the datapath.  If it returns false, the rule may be set up
+ * if and only if it sends a copy of the packet to OFPP_LOCAL.
+ *
+ * (The goal here is to ensure flows can't prevent DHCP replies from getting to
+ * OFPP_LOCAL.  We don't have in-kernel DHCP chaddr matching, hence this
+ * kluge.) */
 bool
-in_band_rule_check(const struct flow *flow,
-                   const struct nlattr *actions, size_t actions_len)
+in_band_rule_check(const struct flow *flow)
 {
-    /* Don't allow flows that would prevent DHCP replies from being seen
-     * by the local port. */
-    if (flow->dl_type == htons(ETH_TYPE_IP)
-            && flow->nw_proto == IPPROTO_UDP
-            && flow->tp_src == htons(DHCP_SERVER_PORT)
-            && flow->tp_dst == htons(DHCP_CLIENT_PORT)) {
-        const struct nlattr *a;
-        unsigned int left;
-
-        NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) {
-            if (nl_attr_type(a) == OVS_ACTION_ATTR_OUTPUT
-                && nl_attr_get_u32(a) == OVSP_LOCAL) {
-                return true;
-            }
-        }
-        return false;
-    }
-
-    return true;
+    return (flow->dl_type != htons(ETH_TYPE_IP)
+            || flow->nw_proto != IPPROTO_UDP
+            || flow->tp_src != htons(DHCP_SERVER_PORT)
+            || flow->tp_dst != htons(DHCP_CLIENT_PORT));
 }
 
 static void
diff --git a/ofproto/in-band.h b/ofproto/in-band.h
index f7f2ec6..ea3f634 100644
--- a/ofproto/in-band.h
+++ b/ofproto/in-band.h
@@ -40,7 +40,6 @@ void in_band_wait(struct in_band *);
 
 bool in_band_msg_in_hook(struct in_band *, const struct flow *,
                          const struct ofpbuf *packet);
-bool in_band_rule_check(const struct flow *,
-                        const struct nlattr *odp_actions, size_t actions_len);
+bool in_band_rule_check(const struct flow *);
 
 #endif /* in-band.h */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index f9738e7..cb22e73 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3634,6 +3634,30 @@ action_xlate_ctx_init(struct action_xlate_ctx *ctx,
     ctx->resubmit_hook = NULL;
 }
 
+/* Checks with in-band control to see if we're allowed to set up this flow. */
+static bool
+may_set_up_flow(struct action_xlate_ctx *ctx)
+{
+    const struct nlattr *a;
+    unsigned int left;
+
+    if (connmgr_may_set_up_flow(ctx->ofproto->up.connmgr, &ctx->flow)) {
+        /* Definitely OK. */
+        return true;
+    }
+
+    /* Maybe still OK, if this flow outputs to OFPP_LOCAL. */
+    NL_ATTR_FOR_EACH_UNSAFE (a, left, ctx->odp_actions->data,
+                             ctx->odp_actions->size) {
+        if (nl_attr_type(a) == OVS_ACTION_ATTR_OUTPUT
+            && nl_attr_get_u32(a) == OVSP_LOCAL) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 static struct ofpbuf *
 xlate_actions(struct action_xlate_ctx *ctx,
               const union ofp_action *in, size_t n_in)
@@ -3659,14 +3683,8 @@ xlate_actions(struct action_xlate_ctx *ctx,
         add_sflow_action(ctx);
         do_xlate_actions(in, n_in, ctx);
         fix_sflow_action(ctx);
-    }
 
-    /* Check with in-band control to see if we're allowed to set up this
-     * flow. */
-    if (!connmgr_may_set_up_flow(ctx->ofproto->up.connmgr, &ctx->flow,
-                                 ctx->odp_actions->data,
-                                 ctx->odp_actions->size)) {
-        ctx->may_set_up_flow = false;
+        ctx->may_set_up_flow = ctx->may_set_up_flow && may_set_up_flow(ctx);
     }
 
     return ctx->odp_actions;
-- 
1.7.4.4




More information about the dev mailing list