[ovs-dev] [PATCH 1/2] ofp-actions: Make ofpacts_check() report consistency for all protocols.

Simon Horman horms at verge.net.au
Mon Nov 18 06:48:29 UTC 2013


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 <blp at 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 <horms at 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 <horms at 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




More information about the dev mailing list