[ovs-dev] [PATCH 5/5] [RFC] ofp-actions: Check pre-requisists of set-field actions

Simon Horman horms at verge.net.au
Wed Sep 12 08:44:32 UTC 2012


By passing a flow to the action parser the pre-requisites
of set-feild actions will be checked. If the flow is NULL,
for instance in test code such as ofctl_parse_ofp11_instructions(),
then the check is skiped, as it always was before this change.

Unfortunately I don't think that this check is correct as
it does not take into account other actions actions that may
be applied before the load action, e.g. vlan push/pop, which may
affect the pre-requisites.

I believe that in its current form there is scope for both false positives
(not so bad, perhaps) and false negatives (pretty bad). I would welcome
some input on if these concerns are valid and if so how they may be
overcome.

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

---

v5
* Initial post
---
 lib/nx-match.c        |    5 +++--
 lib/nx-match.h        |    3 ++-
 lib/ofp-actions.c     |   31 ++++++++++++++++++-------------
 lib/ofp-actions.h     |    3 ++-
 lib/ofp-util.c        |    6 ++++--
 utilities/ovs-ofctl.c |    2 +-
 6 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/lib/nx-match.c b/lib/nx-match.c
index b1eb9e2..c6fee74 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1107,7 +1107,8 @@ nxm_reg_load_from_openflow(const struct nx_action_reg_load *narl,
 
 enum ofperr
 nxm_reg_load_from_openflow12_set_field(
-    const struct ofp12_action_set_field * oasf, struct ofpbuf *ofpacts)
+    const struct ofp12_action_set_field *oasf, struct ofpbuf *ofpacts,
+    struct flow *flow)
 {
     uint16_t oasf_len = ntohs(oasf->len);
     ovs_be32 *p = (ovs_be32*)oasf->field;
@@ -1143,7 +1144,7 @@ nxm_reg_load_from_openflow12_set_field(
                  &load->subvalue, sizeof load->subvalue, load->dst.ofs,
                  mf->n_bits);
 
-    return nxm_reg_load_check(load, NULL);
+    return nxm_reg_load_check(load, flow);
 }
 
 enum ofperr
diff --git a/lib/nx-match.h b/lib/nx-match.h
index 6a57297..721bdeb 100644
--- a/lib/nx-match.h
+++ b/lib/nx-match.h
@@ -66,7 +66,8 @@ enum ofperr nxm_reg_move_from_openflow(const struct nx_action_reg_move *,
 enum ofperr nxm_reg_load_from_openflow(const struct nx_action_reg_load *,
                                        struct ofpbuf *ofpacts);
 enum ofperr nxm_reg_load_from_openflow12_set_field(
-    const struct ofp12_action_set_field * oasf, struct ofpbuf *ofpacts);
+    const struct ofp12_action_set_field * oasf, struct ofpbuf *ofpacts,
+    struct flow *flow);
 
 enum ofperr nxm_reg_move_check(const struct ofpact_reg_move *,
                                const struct flow *);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index df94974..6f748ba 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -385,7 +385,8 @@ ofpact_from_nxast(const union ofp_action *a, enum ofputil_action_code code,
 }
 
 static enum ofperr
-ofpact_from_openflow10(const union ofp_action *a, struct ofpbuf *out)
+ofpact_from_openflow10(const union ofp_action *a, struct ofpbuf *out,
+                       struct flow *flow OVS_UNUSED)
 {
     enum ofputil_action_code code;
     enum ofperr error;
@@ -510,15 +511,16 @@ log_bad_action(const union ofp_action *actions, size_t n_actions, size_t ofs,
 
 static enum ofperr
 ofpacts_from_openflow(const union ofp_action *in, size_t n_in,
-                      struct ofpbuf *out,
+                      struct ofpbuf *out, struct flow *flow,
                       enum ofperr (*ofpact_from_openflow)(
-                          const union ofp_action *a, struct ofpbuf *out))
+                          const union ofp_action *a, struct ofpbuf *out,
+                          struct flow *flow))
 {
     const union ofp_action *a;
     size_t left;
 
     ACTION_FOR_EACH (a, left, in, n_in) {
-        enum ofperr error = ofpact_from_openflow(a, out);
+        enum ofperr error = ofpact_from_openflow(a, out, flow);
         if (error) {
             log_bad_action(in, n_in, a - in, error);
             return error;
@@ -538,7 +540,7 @@ static enum ofperr
 ofpacts_from_openflow10(const union ofp_action *in, size_t n_in,
                         struct ofpbuf *out)
 {
-    return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow10);
+    return ofpacts_from_openflow(in, n_in, out, NULL, ofpact_from_openflow10);
 }
 
 static enum ofperr
@@ -648,7 +650,8 @@ output_from_openflow11(const struct ofp11_action_output *oao,
 }
 
 static enum ofperr
-ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out)
+ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out,
+                       struct flow *flow OVS_UNUSED)
 {
     enum ofputil_action_code code;
     enum ofperr error;
@@ -728,7 +731,7 @@ static enum ofperr
 ofpacts_from_openflow11(const union ofp_action *in, size_t n_in,
                         struct ofpbuf *out)
 {
-    return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow11);
+    return ofpacts_from_openflow(in, n_in, out, NULL, ofpact_from_openflow11);
 }
 
 static enum ofperr
@@ -768,7 +771,8 @@ decode_openflow12_action(const union ofp_action *a,
 }
 
 static enum ofperr
-ofpact_from_openflow12(const union ofp_action *a, struct ofpbuf *out)
+ofpact_from_openflow12(const union ofp_action *a, struct ofpbuf *out,
+                       struct flow *flow)
 {
     /* XXX */
     enum ofputil_action_code code;
@@ -792,7 +796,7 @@ ofpact_from_openflow12(const union ofp_action *a, struct ofpbuf *out)
 
     case OFPUTIL_OFPAT12_SET_FIELD:
         return nxm_reg_load_from_openflow12_set_field(
-            (const struct ofp12_action_set_field *)a, out);
+            (const struct ofp12_action_set_field *)a, out, flow);
 
 #define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) case OFPUTIL_##ENUM:
 #include "ofp-util.def"
@@ -804,9 +808,9 @@ ofpact_from_openflow12(const union ofp_action *a, struct ofpbuf *out)
 
 static enum ofperr
 ofpacts_from_openflow12(const union ofp_action *in, size_t n_in,
-                        struct ofpbuf *out)
+                        struct ofpbuf *out, struct flow *flow)
 {
-    return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow12);
+    return ofpacts_from_openflow(in, n_in, out, flow, ofpact_from_openflow12);
 }
 
 /* OpenFlow 1.1 instructions. */
@@ -987,7 +991,8 @@ enum ofperr
 ofpacts_pull_openflow11_instructions(enum ofp_version ofp_version,
                                      struct ofpbuf *openflow,
                                      unsigned int instructions_len,
-                                     struct ofpbuf *ofpacts)
+                                     struct ofpbuf *ofpacts,
+                                     struct flow *flow)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
     const struct ofp11_instruction *instructions;
@@ -1027,7 +1032,7 @@ ofpacts_pull_openflow11_instructions(enum ofp_version ofp_version,
         get_actions_from_instruction(insts[OVSINST_OFPIT11_APPLY_ACTIONS],
                                      &actions, &n_actions);
         if (ofp_version == OFP12_VERSION) {
-            error = ofpacts_from_openflow12(actions, n_actions, ofpacts);
+            error = ofpacts_from_openflow12(actions, n_actions, ofpacts, flow);
         } else if (ofp_version == OFP11_VERSION){
             error = ofpacts_from_openflow11(actions, n_actions, ofpacts);
         } else {
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 540c91d..4d6ec42 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -402,7 +402,8 @@ enum ofperr ofpacts_pull_openflow11_actions(struct ofpbuf *openflow,
 enum ofperr ofpacts_pull_openflow11_instructions(enum ofp_version ofp_version,
                                                  struct ofpbuf *openflow,
                                                  unsigned int instructions_len,
-                                                 struct ofpbuf *ofpacts);
+                                                 struct ofpbuf *ofpacts,
+                                                 struct flow *flow);
 enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
                           const struct flow *, int max_ports);
 
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index ac3a401..273eacb 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1145,7 +1145,8 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
         }
 
         error = ofpacts_pull_openflow11_instructions(oh->version,
-                                                     &b, b.size, ofpacts);
+                                                     &b, b.size, ofpacts,
+                                                     &fm->match.flow);
         if (error) {
             return error;
         }
@@ -1636,7 +1637,8 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
 
         if (ofpacts_pull_openflow11_instructions(oh->version, msg,
                                                  length - sizeof *ofs -
-                                                 padded_match_len, ofpacts)) {
+                                                 padded_match_len, ofpacts,
+                                                 &fs->match.flow)) {
             VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad instructions");
             return EINVAL;
         }
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 125523d..c7beb1a 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2553,7 +2553,7 @@ ofctl_parse_ofp11_instructions(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         size = of11_in.size;
         error = ofpacts_pull_openflow11_instructions(OFP11_VERSION,
                                                      &of11_in, of11_in.size,
-                                                     &ofpacts);
+                                                     &ofpacts, NULL);
         if (error) {
             printf("bad OF1.1 instructions: %s\n\n", ofperr_get_name(error));
             ofpbuf_uninit(&ofpacts);
-- 
1.7.10.4




More information about the dev mailing list