[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