Fix table checking for goto table instruction.

Usually the table id in flow mods is 255, which means that goto table
instruction cannot be checked before the table is picked (for flow add),
or the rules to be modified are found (flow mod).

Move goto table checking from decode (ofp-util) to actions checking
(ofp-actions), and postpone the action checking until the table in
which the actions are added is known.

This fixes OFPBRC_BAD_TABLE_ID errors for flow adds that specify the table
id as 255, and have a goto table instruction.

Signed-off-by: Jarno Rajahalme <jarno.rajahalme@nsn.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index e1b44d8..899928a 100644 (file)
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1103,7 +1103,6 @@ ofpacts_pull_openflow11_actions(struct ofpbuf *openflow,
 enum ofperr
 ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
                                      unsigned int instructions_len,
-                                     uint8_t table_id,
                                      struct ofpbuf *ofpacts)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
@@ -1181,10 +1180,6 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
 
         oigt = instruction_get_OFPIT11_GOTO_TABLE(
             insts[OVSINST_OFPIT11_GOTO_TABLE]);
-        if (table_id >= oigt->table_id) {
-            error = OFPERR_OFPBRC_BAD_TABLE_ID;
-            goto exit;
-        }
         ogt = ofpact_put_GOTO_TABLE(ofpacts);
         ogt->table_id = oigt->table_id;
     }
@@ -1204,7 +1199,8 @@ exit:
 \f
 /* May modify flow->dl_type, caller must restore it. */
 static enum ofperr
-ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports)
+ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
+               uint8_t table_id)
 {
     const struct ofpact_enqueue *enqueue;
 
@@ -1289,7 +1285,6 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports)
 
     case OFPACT_CLEAR_ACTIONS:
     case OFPACT_WRITE_METADATA:
-    case OFPACT_GOTO_TABLE:
         return 0;
 
     case OFPACT_METER: {
@@ -1299,6 +1294,13 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports)
         }
         return 0;
     }
+
+    case OFPACT_GOTO_TABLE:
+        if (ofpact_get_GOTO_TABLE(a)->table_id <= table_id) {
+            return OFPERR_OFPBRC_BAD_TABLE_ID;
+        }
+        return 0;
+
     default:
         NOT_REACHED();
     }
@@ -1311,14 +1313,14 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports)
  * May temporarily modify 'flow', but restores the changes before returning. */
 enum ofperr
 ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len,
-              struct flow *flow, ofp_port_t max_ports)
+              struct flow *flow, ofp_port_t max_ports, uint8_t table_id)
 {
     const struct ofpact *a;
     ovs_be16 dl_type = flow->dl_type;
     enum ofperr error = 0;
 
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
-        error = ofpact_check__(a, flow, max_ports);
+        error = ofpact_check__(a, flow, max_ports, table_id);
         if (error) {
             break;
         }

diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index ddd94d4..b97afd0 100644 (file)
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -499,10 +499,10 @@ enum ofperr ofpacts_pull_openflow11_actions(struct ofpbuf *openflow,
                                             struct ofpbuf *ofpacts);
 enum ofperr ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
                                                  unsigned int instructions_len,
-                                                 uint8_t table_id,
                                                  struct ofpbuf *ofpacts);
 enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
-                          struct flow *, ofp_port_t max_ports);
+                          struct flow *, ofp_port_t max_ports,
+                          uint8_t table_id);
 enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len);
 
 /* Converting ofpacts to OpenFlow. */

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 2c1fb5d..b1e369c 100644 (file)
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1016,7 +1016,7 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
         fm->ofpacts = ofpbuf_steal_data(&ofpacts);
 
         err = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
-                            OFPP_MAX);
+                            OFPP_MAX, 0);
         if (err) {
             exit(EXIT_FAILURE);
         }

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 48b8928..aa4009d 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1507,8 +1507,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
             return error;
         }
 
-        error = ofpacts_pull_openflow11_instructions(&b, b.size, ofm->table_id,
-                                                     ofpacts);
+        error = ofpacts_pull_openflow11_instructions(&b, b.size, ofpacts);
         if (error) {
             return error;
         }
@@ -2384,8 +2383,7 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         }
 
         if (ofpacts_pull_openflow11_instructions(msg, length - sizeof *ofs -
-                                                 padded_match_len,
-                                                 ofs->table_id, ofpacts)) {
+                                                 padded_match_len, ofpacts)) {
             VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad instructions");
             return EINVAL;
         }

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 7fc5e03..41d106a 100644 (file)
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1397,7 +1397,7 @@ int ofproto_class_unregister(const struct ofproto_class *);
 enum { OFPROTO_POSTPONE = 1 << 16 };
 BUILD_ASSERT_DECL(OFPROTO_POSTPONE < OFPERR_OFS);
 
-int ofproto_flow_mod(struct ofproto *, const struct ofputil_flow_mod *);
+int ofproto_flow_mod(struct ofproto *, struct ofputil_flow_mod *);
 void ofproto_add_flow(struct ofproto *, const struct match *,
                       unsigned int priority,
                       const struct ofpact *ofpacts, size_t ofpacts_len);

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 1fdfb5f..d857902 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -197,13 +197,13 @@ static bool rule_is_modifiable(const struct rule *);
 
 /* OpenFlow. */
 static enum ofperr add_flow(struct ofproto *, struct ofconn *,
-                            const struct ofputil_flow_mod *,
+                            struct ofputil_flow_mod *,
                             const struct ofp_header *);
 static void delete_flow__(struct rule *, struct ofopgroup *,
                           enum ofp_flow_removed_reason);
 static bool handle_openflow(struct ofconn *, const struct ofpbuf *);
 static enum ofperr handle_flow_mod__(struct ofproto *, struct ofconn *,
-                                     const struct ofputil_flow_mod *,
+                                     struct ofputil_flow_mod *,
                                      const struct ofp_header *);
 static void calc_duration(long long int start, long long int now,
                           uint32_t *sec, uint32_t *nsec);
@@ -1629,7 +1629,7 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match,
  *
  * This is a helper function for in-band control and fail-open. */
 int
-ofproto_flow_mod(struct ofproto *ofproto, const struct ofputil_flow_mod *fm)
+ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm)
 {
     return handle_flow_mod__(ofproto, NULL, fm, NULL);
 }
@@ -2404,19 +2404,20 @@ find_meter(const struct ofpact ofpacts[], size_t ofpacts_len)
     return 0;
 }
 
-/* Checks that the 'ofpacts_len' bytes of actions in 'ofpacts' are
- * appropriate for a packet with the prerequisites satisfied by 'flow'.
+/* Checks that the 'ofpacts_len' bytes of actions in 'ofpacts' are appropriate
+ * for a packet with the prerequisites satisfied by 'flow' in table 'table_id'.
  * 'flow' may be temporarily modified, but is restored at return.
  */
 static enum ofperr
 ofproto_check_ofpacts(struct ofproto *ofproto,
                       const struct ofpact ofpacts[], size_t ofpacts_len,
-                      struct flow *flow)
+                      struct flow *flow, uint8_t table_id)
 {
     enum ofperr error;
     uint32_t mid;
 
-    error = ofpacts_check(ofpacts, ofpacts_len, flow, ofproto->max_ports);
+    error = ofpacts_check(ofpacts, ofpacts_len, flow, ofproto->max_ports,
+                          table_id);
     if (error) {
         return error;
     }
@@ -2474,7 +2475,7 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
     /* Verify actions against packet, then send packet if successful. */
     in_port_.ofp_port = po.in_port;
     flow_extract(payload, 0, 0, NULL, &in_port_, &flow);
-    error = ofproto_check_ofpacts(p, po.ofpacts, po.ofpacts_len, &flow);
+    error = ofproto_check_ofpacts(p, po.ofpacts, po.ofpacts_len, &flow, 0);
     if (!error) {
         error = p->ofproto_class->packet_out(p, payload, &flow,
                                              po.ofpacts, po.ofpacts_len);
@@ -3284,7 +3285,7 @@ is_flow_deletion_pending(const struct ofproto *ofproto,
  * if any. */
 static enum ofperr
 add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
-         const struct ofputil_flow_mod *fm, const struct ofp_header *request)
+         struct ofputil_flow_mod *fm, const struct ofp_header *request)
 {
     struct oftable *table;
     struct ofopgroup *group;
@@ -3323,6 +3324,13 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
         return OFPERR_OFPBRC_EPERM;
     }
 
+    /* Verify actions. */
+    error = ofproto_check_ofpacts(ofproto, fm->ofpacts, fm->ofpacts_len,
+                                  &fm->match.flow, table_id);
+    if (error) {
+        return error;
+    }
+
     /* Allocate new rule and initialize classifier rule. */
     rule = ofproto->ofproto_class->rule_alloc();
     if (!rule) {
@@ -3433,8 +3441,7 @@ exit:
  * Returns 0 on success, otherwise an OpenFlow error code. */
 static enum ofperr
 modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
-               const struct ofputil_flow_mod *fm,
-               const struct ofp_header *request,
+               struct ofputil_flow_mod *fm, const struct ofp_header *request,
                struct list *rules)
 {
     struct ofopgroup *group;
@@ -3456,6 +3463,13 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
             continue;
         }
 
+        /* Verify actions. */
+        error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
+                              ofproto->max_ports, rule->table_id);
+        if (error) {
+            return error;
+        }
+
         actions_changed = !ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
                                          rule->ofpacts, rule->ofpacts_len);
 
@@ -3483,8 +3497,7 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
 
 static enum ofperr
 modify_flows_add(struct ofproto *ofproto, struct ofconn *ofconn,
-                 const struct ofputil_flow_mod *fm,
-                 const struct ofp_header *request)
+                 struct ofputil_flow_mod *fm, const struct ofp_header *request)
 {
     if (fm->cookie_mask != htonll(0) || fm->new_cookie == htonll(UINT64_MAX)) {
         return 0;
@@ -3499,7 +3512,7 @@ modify_flows_add(struct ofproto *ofproto, struct ofconn *ofconn,
  * if any. */
 static enum ofperr
 modify_flows_loose(struct ofproto *ofproto, struct ofconn *ofconn,
-                   const struct ofputil_flow_mod *fm,
+                   struct ofputil_flow_mod *fm,
                    const struct ofp_header *request)
 {
     struct list rules;
@@ -3524,7 +3537,7 @@ modify_flows_loose(struct ofproto *ofproto, struct ofconn *ofconn,
  * if any. */
 static enum ofperr
 modify_flow_strict(struct ofproto *ofproto, struct ofconn *ofconn,
-                   const struct ofputil_flow_mod *fm,
+                   struct ofputil_flow_mod *fm,
                    const struct ofp_header *request)
 {
     struct list rules;
@@ -3699,11 +3712,6 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
     ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
     error = ofputil_decode_flow_mod(&fm, oh, ofconn_get_protocol(ofconn),
                                     &ofpacts);
-    if (!error) {
-        error = ofproto_check_ofpacts(ofproto, fm.ofpacts, fm.ofpacts_len,
-                                      &fm.match.flow);
-    }
-
     if (!error) {
         error = handle_flow_mod__(ofproto, ofconn, &fm, oh);
     }
@@ -3745,8 +3753,7 @@ exit:
 
 static enum ofperr
 handle_flow_mod__(struct ofproto *ofproto, struct ofconn *ofconn,
-                  const struct ofputil_flow_mod *fm,
-                  const struct ofp_header *oh)
+                  struct ofputil_flow_mod *fm, const struct ofp_header *oh)
 {
     if (ofproto->n_pending >= 50) {
         ovs_assert(!list_is_empty(&ofproto->pending));

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index b2470e6..2d6872b 100644 (file)
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2637,9 +2637,15 @@ ofctl_parse_ofp11_instructions(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         /* Convert to ofpacts. */
         ofpbuf_init(&ofpacts, 0);
         size = of11_in.size;
-        error = ofpacts_pull_openflow11_instructions(
-            &of11_in, of11_in.size, table_id ? atoi(table_id) : 0,
-            &ofpacts);
+        error = ofpacts_pull_openflow11_instructions(&of11_in, of11_in.size,
+                                                     &ofpacts);
+        if (!error) {
+            /* Verify actions. */
+            struct flow flow;
+            memset(&flow, 0, sizeof flow);
+            error = ofpacts_check(ofpacts.data, ofpacts.size, &flow, OFPP_MAX,
+                                  table_id ? atoi(table_id) : 0);
+        }
         if (error) {
             printf("bad OF1.1 instructions: %s\n\n", ofperr_get_name(error));
             ofpbuf_uninit(&ofpacts);