On Fri, Nov 15, 2013 at 02:33:14PM -0800, Ben Pfaff wrote:
> Until now ofpacts_check() has been told either to enforce consistency or
> not, but that means that the caller has to know exactly what protocol is
> going to be in use (because some protocols require consistency to be
> enforced and others don't). This commit changes ofpacts_check() to just
> rule out protocols that require enforcement when it detects
> inconsistencies.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
Hi Ben,
this approach seems reasonable to me.
The problem I see with verification of VLAN actions that follow push_mpls
when OpenFlow1.3 is not that different protocols require consistency to
enforced while others don't. Rather the problem I as is that what is
consistent is different for pre-OpenFlow1.3 and OpenFlow1.3+ tag ordering.
I believe that logic to handle that difference can be built on top of your
patch and what follows is a prototype of such logic. I'm not sure that this
is the direction you want us to take so I would appreciate feedback on this
approach.
From: Simon Horman <ho...@verge.net.au>
[PATCH] [RFC] Consistency checking of OF1.3 VLAN actions after mpls_push
*** Do not apply. ***
The aim of this patch is to support verification of VLAN
actions after an mpls_push action for OpenFlow1.3. This supplements
existing support for verifying these actions for pre-OpenFlow1.3.
In OpenFlow1.1 and 1.2 MPLS tags are pushed after any VLAN tags that
immediately follow the ethernet header. This is pre-OpenFlow1.3 tag
ordering. Open vSwitch also uses this ordering when supporting MPLS
actions via Nicira extensions to OpenFlow1.0.
When using pre-OpenFlow1.3 tag ordering an MPLS push action does not
affect the VLANs of a packet. If VLAN tags are present immediately after
the ethernet header then they remain present there.
In of OpenFlow1.3+ MPLS LSEs are pushed before any VLAN tags that
immediately follow the ethernet header. This is OpenFlow1.3+ tag
ordering.
When using OpenFlow1.3+ tag ordering an MPLS push action affects the
VLANs of a packet as any VLAN tags previously present after the ethernet
header are moved to be immediately after the newly pushed MPLS LSE. Thus
for the purpose of action consistency checking a packet may be changed
from a VLAN packet to a non-VLAN packet.
In this way the effective value of the VLAN TCI of a packet may differ
after an MPLS push depending on the OpenFlow version in use.
Signed-off-by: Simon Horman <ho...@verge.net.au>
---
This patch is an RFC intended to further discussion. It builds and depends
upon "ofp-actions: Make ofpacts_check() report consistency for all
protocols".
It also requires a revised version of "odp: Allow VLAN actions after MPLS
actions". I have held off on publishing that patch or a revised version of
the MPLS patchset it is a part of as I believe this proposal can be
discussed without those details.
---
lib/ofp-actions.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 116 insertions(+), 14 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index e07ea1d..47244f5 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1879,15 +1879,102 @@ inconsistent_match(enum ofputil_protocol
*usable_protocols)
*usable_protocols &= OFPUTIL_P_OF10_ANY;
}
+/* Checks the consistency of a VLAN action by checking the state of the
+ * VLAN_CFI bit of the VLAN TCI against 'desired_cfi' and updating
+ * '*usable_protocols' accordingly.
+ *
+ * 'pre_of13_vlan_tci' is the VLAN TCI to check against for pre-OpenFlow1.3
+ * protocols. 'of13_vlan_tci' is the VLAN TCI to check against for
+ * OpenFlow1.3+ protocols.
+ *
+ * If the desired_cfi bit is in the desired state for any usable protocol
+ * then true is returned. Else false is returned.
+ *
+ * The check for consistent VLAN actions may depend on the OpenFlow version
+ * used and whether the VLAN action is preceded by an MPLS push action or
+ * not.
+ *
+ * In OpenFlow1.1 and 1.2 MPLS tags are pushed after any VLAN tags that
+ * immediately follow the ethernet header. This is pre-OpenFlow1.3 tag
+ * ordering. Open vSwitch also uses this ordering when supporting MPLS
+ * actions via Nicira extensions to OpenFlow1.0.
+ *
+ * When using pre-OpenFlow1.3 tag ordering an MPLS push action does not
+ * affect the VLANs of a packet. If VLAN tags are present immediately after
+ * the ethernet header then they remain present there.
+ *
+ * In of OpenFlow1.3+ MPLS LSEs are pushed before any VLAN tags that
+ * immediately follow the ethernet header. This is OpenFlow1.3+ tag
+ * ordering.
+ *
+ * When using OpenFlow1.3+ tag ordering an MPLS push action affects the
+ * VLANs of a packet as any VLAN tags previously present after the ethernet
+ * header are moved to be immediately after the newly pushed MPLS LSE. Thus
+ * for the purpose of action consistency checking a packet may be changed
+ * from a VLAN packet to a non-VLAN packet.
+ *
+ * In this way the effective value of the VLAN TCI of a packet may differ
+ * after an MPLS push depending on the OpenFlow version in use. This
+ * corresponds to the 'pre_of13_vlan_tci' and 'of13_vlan_tci' parameters of
+ * this function. */
+static bool
+check_vlan_action(enum ofputil_protocol *usable_protocols,
+ ovs_be32 pre_of13_vlan_tci, ovs_be32 of13_vlan_tci,
+ bool desired_cfi)
+{
+ bool ok = false;
+ ovs_be16 tci;
+
+ if (desired_cfi) {
+ tci = htons(VLAN_CFI);
+ } else {
+ tci = htons(0);
+ }
+
+ if (*usable_protocols & OFPUTIL_P_OF13_UP) {
+ if ((of13_vlan_tci & tci) != tci) {
+ /* CFI miss-match for OpenFlow1.3+: clear those protocols */
+ *usable_protocols &= ~OFPUTIL_P_OF13_UP;
+ } else {
+ /* CFI bit is valid for a usable protocol:
+ * eventually return true */
+ ok = true;
+ }
+ }
+
+ if (*usable_protocols & ~OFPUTIL_P_OF13_UP) {
+ if ((pre_of13_vlan_tci & tci) != tci) {
+ enum ofputil_protocol of13_up;
+
+ /* CFI miss-match for pre-OpenFlow1.3: clear those protocols
+ * except OFPUTIL_P_OF10_ANY as it allows inconsistency. */
+ of13_up = *usable_protocols & OFPUTIL_P_OF13_UP;
+ inconsistent_match(usable_protocols);
+ *usable_protocols &= of13_up;
+ } else {
+ /* CFI bit is valid for a usable protocol:
+ * eventually return true */
+ ok = true;
+ }
+ }
+
+ return ok;
+}
+
/* May modify flow->dl_type, flow->nw_proto and flow->vlan_tci,
* caller must restore them.
*
+ * May also modify of13_vlan_tci which the caller should discard. On the
+ * first call to ofpact_check__() of13_vlan_tci should be set to
+ * flow->vlan_tci
+ *
* Modifies some actions, filling in fields that could not be properly set
* without context. */
static enum ofperr
ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
struct flow *flow, ofp_port_t max_ports,
- uint8_t table_id, uint8_t n_tables)
+ uint8_t table_id, uint8_t n_tables,
+ ovs_be32 *of13_vlan_tci)
{
const struct ofpact_enqueue *enqueue;
const struct mf_field *mf;
@@ -1920,12 +2007,13 @@ ofpact_check__(enum ofputil_protocol *usable_protocols,
struct ofpact *a,
* OpenFlow 1.1+ if need be. */
ofpact_get_SET_VLAN_VID(a)->flow_has_vlan =
(flow->vlan_tci & htons(VLAN_CFI)) == htons(VLAN_CFI);
- if (!(flow->vlan_tci & htons(VLAN_CFI)) &&
- !ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
- inconsistent_match(usable_protocols);
+ if (!ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) {
+ check_vlan_action(usable_protocols, flow->vlan_tci,
+ *of13_vlan_tci, true);
}
/* Temporary mark that we have a vlan tag. */
flow->vlan_tci |= htons(VLAN_CFI);
+ *of13_vlan_tci |= htons(VLAN_CFI);
return 0;
case OFPACT_SET_VLAN_PCP:
@@ -1933,29 +2021,31 @@ ofpact_check__(enum ofputil_protocol *usable_protocols,
struct ofpact *a,
* OpenFlow 1.1+ if need be. */
ofpact_get_SET_VLAN_PCP(a)->flow_has_vlan =
(flow->vlan_tci & htons(VLAN_CFI)) == htons(VLAN_CFI);
- if (!(flow->vlan_tci & htons(VLAN_CFI)) &&
- !ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
- inconsistent_match(usable_protocols);
+ if (!ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) {
+ check_vlan_action(usable_protocols, flow->vlan_tci,
+ *of13_vlan_tci, true);
}
/* Temporary mark that we have a vlan tag. */
flow->vlan_tci |= htons(VLAN_CFI);
+ *of13_vlan_tci |= htons(VLAN_CFI);
return 0;
case OFPACT_STRIP_VLAN:
- if (!(flow->vlan_tci & htons(VLAN_CFI))) {
- inconsistent_match(usable_protocols);
- }
+ check_vlan_action(usable_protocols, flow->vlan_tci, *of13_vlan_tci,
+ true);
/* Temporary mark that we have no vlan tag. */
- flow->vlan_tci = htons(0);
+ *of13_vlan_tci = flow->vlan_tci = htons(0);
return 0;
case OFPACT_PUSH_VLAN:
- if (flow->vlan_tci & htons(VLAN_CFI)) {
+ if (!check_vlan_action(usable_protocols, flow->vlan_tci,
+ *of13_vlan_tci, false)) {
/* Multiple VLAN headers not supported. */
return OFPERR_OFPBAC_BAD_TAG;
}
/* Temporary mark that we have a vlan tag. */
flow->vlan_tci |= htons(VLAN_CFI);
+ *of13_vlan_tci |= htons(VLAN_CFI);
return 0;
case OFPACT_SET_ETH_SRC:
@@ -2012,7 +2102,9 @@ ofpact_check__(enum ofputil_protocol *usable_protocols,
struct ofpact *a,
mf = ofpact_get_SET_FIELD(a)->field;
/* Require OXM_OF_VLAN_VID to have an existing VLAN header. */
if (!mf_are_prereqs_ok(mf, flow) ||
- (mf->id == MFF_VLAN_VID && !(flow->vlan_tci & htons(VLAN_CFI)))) {
+ (mf->id == MFF_VLAN_VID &&
+ !check_vlan_action(usable_protocols, flow->vlan_tci,
+ *of13_vlan_tci, true))) {
VLOG_WARN_RL(&rl, "set_field %s lacks correct prerequisities",
mf->name);
return OFPERR_OFPBAC_MATCH_INCONSISTENT;
@@ -2025,6 +2117,7 @@ ofpact_check__(enum ofputil_protocol *usable_protocols,
struct ofpact *a,
/* The set field may add or remove the vlan tag,
* Mark the status temporarily. */
flow->vlan_tci = ofpact_get_SET_FIELD(a)->value.be16;
+ *of13_vlan_tci = ofpact_get_SET_FIELD(a)->value.be16;
}
return 0;
@@ -2071,6 +2164,13 @@ ofpact_check__(enum ofputil_protocol *usable_protocols,
struct ofpact *a,
* Thus nothing can be assumed about the network protocol.
* Temporarily mark that we have no nw_proto. */
flow->nw_proto = 0;
+
+ /* In the case of OpenFlow1.3+ the MPLS LSE is pushed before
+ * any VLAN tags which were previously present immediately
+ * after the VLAN header.
+ * Temporarily mark that there is no VLAN tag in the case
+ * of OpenFlow1.3+ */
+ *of13_vlan_tci = htons(0);
return 0;
case OFPACT_POP_MPLS:
@@ -2144,12 +2244,14 @@ ofpacts_check(struct ofpact ofpacts[], size_t
ofpacts_len,
struct ofpact *a;
ovs_be16 dl_type = flow->dl_type;
ovs_be16 vlan_tci = flow->vlan_tci;
+ ovs_be32 of13_vlan_tci = flow->vlan_tci;
uint8_t nw_proto = flow->nw_proto;
enum ofperr error = 0;
OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
error = ofpact_check__(usable_protocols, a, flow,
- max_ports, table_id, n_tables);
+ max_ports, table_id, n_tables,
+ &of13_vlan_tci);
if (error) {
break;
}
--
1.8.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
