[ovs-dev] [PATCH 2/4] nx-match: Do not check pre-requisites for load actions

Simon Horman horms at verge.net.au
Thu Oct 18 09:02:19 UTC 2012


There are (or at least will be) cases where this check can produce false
positives.  For example, a flow which matches a non-MPLS packet and then
applies an MPLS push action followed by an action to load the MPLS label.

I believe this is the same problem that was recently discussed in
relation to set-field. In that case my understanding is that consensus
was to omit the check and potentially revisit the problem later.

Signed-off-by: Simon Horman <horms at verge.net.au>

---

v2.2
* No change

v2.1
* Initial post
---
 lib/autopath.c    |    6 +++---
 lib/autopath.h    |    3 +--
 lib/bundle.c      |    7 +++----
 lib/bundle.h      |    3 +--
 lib/learn.c       |   10 +++++-----
 lib/learn.h       |    2 +-
 lib/meta-flow.c   |   14 +++++---------
 lib/meta-flow.h   |    4 ++--
 lib/multipath.c   |    7 +++----
 lib/multipath.h   |    3 +--
 lib/nx-match.c    |   16 ++++++++--------
 lib/nx-match.h    |    6 ++----
 lib/ofp-actions.c |   23 +++++++++++------------
 lib/ofp-actions.h |    2 +-
 ofproto/ofproto.c |    4 ++--
 15 files changed, 49 insertions(+), 61 deletions(-)

diff --git a/lib/autopath.c b/lib/autopath.c
index b204e84..1af5cd9 100644
--- a/lib/autopath.c
+++ b/lib/autopath.c
@@ -80,16 +80,16 @@ autopath_from_openflow(const struct nx_action_autopath *nap,
         return OFPERR_OFPBAC_BAD_ARGUMENT;
     }
 
-    return autopath_check(autopath, NULL);
+    return autopath_check(autopath);
 }
 
 enum ofperr
-autopath_check(const struct ofpact_autopath *autopath, const struct flow *flow)
+autopath_check(const struct ofpact_autopath *autopath)
 {
     VLOG_WARN_ONCE("The autopath action is deprecated and may be removed in"
                    " February 2013.  Please email dev at openvswitch.org with"
                    " concerns.");
-    return mf_check_dst(&autopath->dst, flow);
+    return mf_check_dst(&autopath->dst);
 }
 
 void
diff --git a/lib/autopath.h b/lib/autopath.h
index 337e7d1..ef46456 100644
--- a/lib/autopath.h
+++ b/lib/autopath.h
@@ -33,8 +33,7 @@ void autopath_parse(struct ofpact_autopath *, const char *);
 
 enum ofperr autopath_from_openflow(const struct nx_action_autopath *,
                                    struct ofpact_autopath *);
-enum ofperr autopath_check(const struct ofpact_autopath *,
-                           const struct flow *);
+enum ofperr autopath_check(const struct ofpact_autopath *);
 void autopath_to_nxast(const struct ofpact_autopath *,
                        struct ofpbuf *openflow);
 
diff --git a/lib/bundle.c b/lib/bundle.c
index b68ebab..6ced9ad 100644
--- a/lib/bundle.c
+++ b/lib/bundle.c
@@ -173,20 +173,19 @@ bundle_from_openflow(const struct nx_action_bundle *nab,
     ofpact_update_len(ofpacts, &bundle->ofpact);
 
     if (!error) {
-        error = bundle_check(bundle, OFPP_MAX, NULL);
+        error = bundle_check(bundle, OFPP_MAX);
     }
     return error;
 }
 
 enum ofperr
-bundle_check(const struct ofpact_bundle *bundle, int max_ports,
-             const struct flow *flow)
+bundle_check(const struct ofpact_bundle *bundle, int max_ports)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
     size_t i;
 
     if (bundle->dst.field) {
-        enum ofperr error = mf_check_dst(&bundle->dst, flow);
+        enum ofperr error = mf_check_dst(&bundle->dst);
         if (error) {
             return error;
         }
diff --git a/lib/bundle.h b/lib/bundle.h
index 5b6bb67..a41f3f6 100644
--- a/lib/bundle.h
+++ b/lib/bundle.h
@@ -39,8 +39,7 @@ uint16_t bundle_execute(const struct ofpact_bundle *, const struct flow *,
                         void *aux);
 enum ofperr bundle_from_openflow(const struct nx_action_bundle *,
                                  struct ofpbuf *ofpact);
-enum ofperr bundle_check(const struct ofpact_bundle *, int max_ports,
-                         const struct flow *);
+enum ofperr bundle_check(const struct ofpact_bundle *, int max_ports);
 void bundle_to_nxast(const struct ofpact_bundle *, struct ofpbuf *of10);
 void bundle_parse(const char *, struct ofpbuf *ofpacts);
 void bundle_parse_load(const char *, struct ofpbuf *ofpacts);
diff --git a/lib/learn.c b/lib/learn.c
index b9bbc97..f9d5527 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -168,7 +168,7 @@ learn_from_openflow(const struct nx_action_learn *nal, struct ofpbuf *ofpacts)
 /* Checks that 'learn' is a valid action on 'flow'.  Returns 0 if it is valid,
  * otherwise an OFPERR_*. */
 enum ofperr
-learn_check(const struct ofpact_learn *learn, const struct flow *flow)
+learn_check(const struct ofpact_learn *learn)
 {
     const struct ofpact_learn_spec *spec;
     struct match match;
@@ -179,7 +179,7 @@ learn_check(const struct ofpact_learn *learn, const struct flow *flow)
 
         /* Check the source. */
         if (spec->src_type == NX_LEARN_SRC_FIELD) {
-            error = mf_check_src(&spec->src, flow);
+            error = mf_check_src(&spec->src);
             if (error) {
                 return error;
             }
@@ -188,7 +188,7 @@ learn_check(const struct ofpact_learn *learn, const struct flow *flow)
         /* Check the destination. */
         switch (spec->dst_type) {
         case NX_LEARN_DST_MATCH:
-            error = mf_check_src(&spec->dst, &match.flow);
+            error = mf_check_src(&spec->dst);
             if (error) {
                 return error;
             }
@@ -197,7 +197,7 @@ learn_check(const struct ofpact_learn *learn, const struct flow *flow)
             break;
 
         case NX_LEARN_DST_LOAD:
-            error = mf_check_dst(&spec->dst, &match.flow);
+            error = mf_check_dst(&spec->dst);
             if (error) {
                 return error;
             }
@@ -583,7 +583,7 @@ learn_parse(char *arg, const struct flow *flow, struct ofpbuf *ofpacts)
 
     /* In theory the above should have caught any errors, but... */
     if (flow) {
-        error = learn_check(learn, flow);
+        error = learn_check(learn);
         if (error) {
             ovs_fatal(0, "%s: %s", orig, ofperr_to_string(error));
         }
diff --git a/lib/learn.h b/lib/learn.h
index adf597e..3fa2d53 100644
--- a/lib/learn.h
+++ b/lib/learn.h
@@ -33,7 +33,7 @@ struct nx_action_learn;
 
 enum ofperr learn_from_openflow(const struct nx_action_learn *,
                                 struct ofpbuf *ofpacts);
-enum ofperr learn_check(const struct ofpact_learn *, const struct flow *);
+enum ofperr learn_check(const struct ofpact_learn *);
 void learn_to_nxast(const struct ofpact_learn *, struct ofpbuf *openflow);
 
 void learn_execute(const struct ofpact_learn *, const struct flow *,
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 591eb34..8931523 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -1670,8 +1670,7 @@ mf_set(const struct mf_field *mf,
 }
 
 static enum ofperr
-mf_check__(const struct mf_subfield *sf, const struct flow *flow,
-           const char *type)
+mf_check__(const struct mf_subfield *sf, const char *type)
 {
     if (!sf->field) {
         VLOG_WARN_RL(&rl, "unknown %s field", type);
@@ -1684,9 +1683,6 @@ mf_check__(const struct mf_subfield *sf, const struct flow *flow,
         VLOG_WARN_RL(&rl, "bit offset %d and width %d exceeds %d-bit width "
                      "of %s field %s", sf->ofs, sf->n_bits,
                      sf->field->n_bits, type, sf->field->name);
-    } else if (flow && !mf_are_prereqs_ok(sf->field, flow)) {
-        VLOG_WARN_RL(&rl, "%s field %s lacks correct prerequisites",
-                     type, sf->field->name);
     } else {
         return 0;
     }
@@ -1698,18 +1694,18 @@ mf_check__(const struct mf_subfield *sf, const struct flow *flow,
  * 0 if so, otherwise an OpenFlow error code (e.g. as returned by
  * ofp_mkerr()).  */
 enum ofperr
-mf_check_src(const struct mf_subfield *sf, const struct flow *flow)
+mf_check_src(const struct mf_subfield *sf)
 {
-    return mf_check__(sf, flow, "source");
+    return mf_check__(sf, "source");
 }
 
 /* Checks whether 'sf' is valid for writing a subfield into 'flow'.  Returns 0
  * if so, otherwise an OpenFlow error code (e.g. as returned by
  * ofp_mkerr()). */
 enum ofperr
-mf_check_dst(const struct mf_subfield *sf, const struct flow *flow)
+mf_check_dst(const struct mf_subfield *sf)
 {
-    int error = mf_check__(sf, flow, "destination");
+    int error = mf_check__(sf, "destination");
     if (!error && !sf->field->writable) {
         VLOG_WARN_RL(&rl, "destination field %s is not writable",
                      sf->field->name);
diff --git a/lib/meta-flow.h b/lib/meta-flow.h
index 60bfeca..3fe9014 100644
--- a/lib/meta-flow.h
+++ b/lib/meta-flow.h
@@ -332,8 +332,8 @@ void mf_format_subfield(const struct mf_subfield *, struct ds *);
 char *mf_parse_subfield__(struct mf_subfield *sf, const char **s);
 const char *mf_parse_subfield(struct mf_subfield *, const char *);
 
-enum ofperr mf_check_src(const struct mf_subfield *, const struct flow *);
-enum ofperr mf_check_dst(const struct mf_subfield *, const struct flow *);
+enum ofperr mf_check_src(const struct mf_subfield *);
+enum ofperr mf_check_dst(const struct mf_subfield *);
 
 /* Parsing and formatting. */
 char *mf_parse(const struct mf_field *, const char *,
diff --git a/lib/multipath.c b/lib/multipath.c
index f6a1a0a..200d2d3 100644
--- a/lib/multipath.c
+++ b/lib/multipath.c
@@ -68,16 +68,15 @@ multipath_from_openflow(const struct nx_action_multipath *nam,
         return OFPERR_OFPBAC_BAD_ARGUMENT;
     }
 
-    return multipath_check(mp, NULL);
+    return multipath_check(mp);
 }
 
 /* Checks that 'mp' is valid on flow.  Returns 0 if it is valid, otherwise an
  * OFPERR_*. */
 enum ofperr
-multipath_check(const struct ofpact_multipath *mp,
-                const struct flow *flow)
+multipath_check(const struct ofpact_multipath *mp)
 {
-    return mf_check_dst(&mp->dst, flow);
+    return mf_check_dst(&mp->dst);
 }
 
 /* Converts 'mp' into an OpenFlow NXAST_MULTIPATH action, which it appends to
diff --git a/lib/multipath.h b/lib/multipath.h
index 1b5160d..1f02e9e 100644
--- a/lib/multipath.h
+++ b/lib/multipath.h
@@ -33,8 +33,7 @@ struct ofpbuf;
 
 enum ofperr multipath_from_openflow(const struct nx_action_multipath *,
                                     struct ofpact_multipath *);
-enum ofperr multipath_check(const struct ofpact_multipath *,
-                            const struct flow *);
+enum ofperr multipath_check(const struct ofpact_multipath *);
 void multipath_to_nxast(const struct ofpact_multipath *,
                         struct ofpbuf *openflow);
 
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 9918994..1e59387 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1068,7 +1068,7 @@ nxm_reg_move_from_openflow(const struct nx_action_reg_move *narm,
     move->dst.ofs = ntohs(narm->dst_ofs);
     move->dst.n_bits = ntohs(narm->n_bits);
 
-    return nxm_reg_move_check(move, NULL);
+    return nxm_reg_move_check(move);
 }
 
 enum ofperr
@@ -1090,7 +1090,7 @@ nxm_reg_load_from_openflow(const struct nx_action_reg_load *narl,
         return OFPERR_OFPBAC_BAD_ARGUMENT;
     }
 
-    return nxm_reg_load_check(load, NULL);
+    return nxm_reg_load_check(load);
 }
 
 enum ofperr
@@ -1122,26 +1122,26 @@ nxm_reg_load_from_openflow12_set_field(
     load = ofpact_put_REG_LOAD(ofpacts);
     ofpact_set_field_init(load, mf, oasf + 1);
 
-    return nxm_reg_load_check(load, NULL);
+    return nxm_reg_load_check(load);
 }
 
 enum ofperr
-nxm_reg_move_check(const struct ofpact_reg_move *move, const struct flow *flow)
+nxm_reg_move_check(const struct ofpact_reg_move *move)
 {
     enum ofperr error;
 
-    error = mf_check_src(&move->src, flow);
+    error = mf_check_src(&move->src);
     if (error) {
         return error;
     }
 
-    return mf_check_dst(&move->dst, NULL);
+    return mf_check_dst(&move->dst);
 }
 
 enum ofperr
-nxm_reg_load_check(const struct ofpact_reg_load *load, const struct flow *flow)
+nxm_reg_load_check(const struct ofpact_reg_load *load)
 {
-    return mf_check_dst(&load->dst, flow);
+    return mf_check_dst(&load->dst);
 }
 
 void
diff --git a/lib/nx-match.h b/lib/nx-match.h
index 6a57297..9ece6e8 100644
--- a/lib/nx-match.h
+++ b/lib/nx-match.h
@@ -68,10 +68,8 @@ enum ofperr nxm_reg_load_from_openflow(const struct nx_action_reg_load *,
 enum ofperr nxm_reg_load_from_openflow12_set_field(
     const struct ofp12_action_set_field * oasf, struct ofpbuf *ofpacts);
 
-enum ofperr nxm_reg_move_check(const struct ofpact_reg_move *,
-                               const struct flow *);
-enum ofperr nxm_reg_load_check(const struct ofpact_reg_load *,
-                               const struct flow *);
+enum ofperr nxm_reg_move_check(const struct ofpact_reg_move *);
+enum ofperr nxm_reg_load_check(const struct ofpact_reg_load *);
 
 void nxm_reg_move_to_nxast(const struct ofpact_reg_move *,
                            struct ofpbuf *openflow);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 4a63acb..1fb0304 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -109,7 +109,7 @@ output_reg_from_openflow(const struct nx_action_output_reg *naor,
     output_reg->src.n_bits = nxm_decode_n_bits(naor->ofs_nbits);
     output_reg->max_len = ntohs(naor->max_len);
 
-    return mf_check_src(&output_reg->src, NULL);
+    return mf_check_src(&output_reg->src);
 }
 
 static void
@@ -989,7 +989,7 @@ exit:
 }
 
 static enum ofperr
-ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
+ofpact_check__(const struct ofpact *a, int max_ports)
 {
     const struct ofpact_enqueue *enqueue;
 
@@ -1010,10 +1010,10 @@ ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
         return 0;
 
     case OFPACT_OUTPUT_REG:
-        return mf_check_src(&ofpact_get_OUTPUT_REG(a)->src, flow);
+        return mf_check_src(&ofpact_get_OUTPUT_REG(a)->src);
 
     case OFPACT_BUNDLE:
-        return bundle_check(ofpact_get_BUNDLE(a), max_ports, flow);
+        return bundle_check(ofpact_get_BUNDLE(a), max_ports);
 
     case OFPACT_SET_VLAN_VID:
     case OFPACT_SET_VLAN_PCP:
@@ -1028,10 +1028,10 @@ ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
         return 0;
 
     case OFPACT_REG_MOVE:
-        return nxm_reg_move_check(ofpact_get_REG_MOVE(a), flow);
+        return nxm_reg_move_check(ofpact_get_REG_MOVE(a));
 
     case OFPACT_REG_LOAD:
-        return nxm_reg_load_check(ofpact_get_REG_LOAD(a), flow);
+        return nxm_reg_load_check(ofpact_get_REG_LOAD(a));
 
     case OFPACT_DEC_TTL:
     case OFPACT_SET_TUNNEL:
@@ -1042,13 +1042,13 @@ ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
         return 0;
 
     case OFPACT_LEARN:
-        return learn_check(ofpact_get_LEARN(a), flow);
+        return learn_check(ofpact_get_LEARN(a));
 
     case OFPACT_MULTIPATH:
-        return multipath_check(ofpact_get_MULTIPATH(a), flow);
+        return multipath_check(ofpact_get_MULTIPATH(a));
 
     case OFPACT_AUTOPATH:
-        return autopath_check(ofpact_get_AUTOPATH(a), flow);
+        return autopath_check(ofpact_get_AUTOPATH(a));
 
     case OFPACT_NOTE:
     case OFPACT_EXIT:
@@ -1067,13 +1067,12 @@ ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
  * appropriate for a packet with the prerequisites satisfied by 'flow' in a
  * switch with no more than 'max_ports' ports. */
 enum ofperr
-ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len,
-              const struct flow *flow, int max_ports)
+ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len, int max_ports)
 {
     const struct ofpact *a;
 
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
-        enum ofperr error = ofpact_check__(a, flow, max_ports);
+        enum ofperr error = ofpact_check__(a, max_ports);
         if (error) {
             return error;
         }
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index c62020d..12e5891 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -440,7 +440,7 @@ enum ofperr ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
                                                  unsigned int instructions_len,
                                                  struct ofpbuf *ofpacts);
 enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
-                          const struct flow *, int max_ports);
+                          int max_ports);
 
 /* Converting ofpacts to OpenFlow. */
 void ofpacts_put_openflow10(const struct ofpact[], size_t ofpacts_len,
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 2fb2fc8..035febe 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2148,7 +2148,7 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
 
     /* Verify actions against packet, then send packet if successful. */
     flow_extract(payload, 0, NULL, po.in_port, &flow);
-    error = ofpacts_check(po.ofpacts, po.ofpacts_len, &flow, p->max_ports);
+    error = ofpacts_check(po.ofpacts, po.ofpacts_len, p->max_ports);
     if (!error) {
         error = p->ofproto_class->packet_out(p, payload, &flow,
                                              po.ofpacts, po.ofpacts_len);
@@ -3308,7 +3308,7 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
     }
     if (!error) {
         error = ofpacts_check(fm.ofpacts, fm.ofpacts_len,
-                              &fm.match.flow, ofproto->max_ports);
+                              ofproto->max_ports);
     }
     if (!error) {
         error = handle_flow_mod__(ofconn_get_ofproto(ofconn), ofconn, &fm, oh);
-- 
1.7.10.4




More information about the dev mailing list