ofproto: Centralize action checking, doing it at decode time.

Jarno pointed out that modify_flows__() didn't really need to check every
instance of the flow separately.  After some further investigation I
decided that this was even more of an improvement.

CC: Jarno Rajahalme <jrajahalme@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>

lib/ofp-actions.c

@@ -1823,8 +1823,9 @@ ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports)
  * Modifies some actions, filling in fields that could not be properly set
  * without context. */
 static enum ofperr
-ofpact_check__(struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
+ofpact_check__(struct ofpact *a, struct flow *flow,
-               uint8_t table_id, bool enforce_consistency)
+               bool enforce_consistency, ofp_port_t max_ports,
+               uint8_t table_id, uint8_t n_tables)
 {
     const struct ofpact_enqueue *enqueue;
     const struct mf_field *mf;
@@ -2020,7 +2021,7 @@ ofpact_check__(struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
     case OFPACT_WRITE_ACTIONS: {
         struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
         return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
-                             flow, max_ports, table_id, false);
+                             flow, false, max_ports, table_id, n_tables);
     }
 
     case OFPACT_WRITE_METADATA:
@@ -2034,11 +2035,14 @@ ofpact_check__(struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
         return 0;
     }
 
-    case OFPACT_GOTO_TABLE:
+    case OFPACT_GOTO_TABLE: {
-        if (ofpact_get_GOTO_TABLE(a)->table_id <= table_id) {
+        uint8_t goto_table = ofpact_get_GOTO_TABLE(a)->table_id;
+        if ((table_id != 255 && goto_table <= table_id)
+            || (n_tables != 255 && goto_table >= n_tables)) {
             return OFPERR_OFPBRC_BAD_TABLE_ID;
         }
         return 0;
+    }
 
     case OFPACT_GROUP:
         return 0;
@@ -2063,8 +2067,9 @@ ofpact_check__(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(struct ofpact ofpacts[], size_t ofpacts_len,
-              struct flow *flow, ofp_port_t max_ports, uint8_t table_id,
+              struct flow *flow, bool enforce_consistency,
-              bool enforce_consistency)
+              ofp_port_t max_ports,
+              uint8_t table_id, uint8_t n_tables)
 {
     struct ofpact *a;
     ovs_be16 dl_type = flow->dl_type;
@@ -2072,8 +2077,8 @@ ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len,
     enum ofperr error = 0;
 
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
-        error = ofpact_check__(a, flow, max_ports, table_id,
+        error = ofpact_check__(a, flow, enforce_consistency,
-                               enforce_consistency);
+                               max_ports, table_id, n_tables);
         if (error) {
             break;
         }

lib/ofp-actions.h

@@ -587,8 +587,9 @@ enum ofperr ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
                                                enum ofp_version version,
                                                struct ofpbuf *ofpacts);
 enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
-                          struct flow *, ofp_port_t max_ports,
+                          struct flow *, bool enforce_consistency,
-                          uint8_t table_id, bool enforce_consistency);
+                          ofp_port_t max_ports,
+                          uint8_t table_id, uint8_t n_tables);
 enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len);
 enum ofperr ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports);
 

lib/ofp-parse.c

@@ -1368,7 +1368,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
             enum ofperr err;
 
             err = ofpacts_check(ofpacts.data, ofpacts.size, &fm->match.flow,
-                                OFPP_MAX, 0, true);
+                                true, OFPP_MAX, fm->table_id, 255);
             if (err) {
                 if (!enforce_consistency &&
                     err == OFPERR_OFPBAC_MATCH_INCONSISTENT) {
@@ -1377,7 +1377,8 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
                     /* Try again, allowing for inconsistency.
                      * XXX: As a side effect, logging may be duplicated. */
                     err = ofpacts_check(ofpacts.data, ofpacts.size,
-                                        &fm->match.flow, OFPP_MAX, 0, false);
+                                        &fm->match.flow, false,
+                                        OFPP_MAX, fm->table_id, 255);
                 }
                 if (err) {
                     error = xasprintf("actions are invalid with specified match "

lib/ofp-print.c

@@ -757,7 +757,8 @@ ofp_print_flow_mod(struct ds *s, const struct ofp_header *oh, int verbosity)
     protocol = ofputil_protocol_set_tid(protocol, true);
 
     ofpbuf_init(&ofpacts, 64);
-    error = ofputil_decode_flow_mod(&fm, oh, protocol, &ofpacts);
+    error = ofputil_decode_flow_mod(&fm, oh, protocol, &ofpacts,
+                                    OFPP_MAX, 255);
     if (error) {
         ofpbuf_uninit(&ofpacts);
         ofp_print_error(s, error);

lib/ofp-util.c

@@ -1484,7 +1484,8 @@ enum ofperr
 ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
                         const struct ofp_header *oh,
                         enum ofputil_protocol protocol,
-                        struct ofpbuf *ofpacts)
+                        struct ofpbuf *ofpacts,
+                        ofp_port_t max_port, uint8_t max_table)
 {
     ovs_be16 raw_flags;
     enum ofperr error;
@@ -1663,7 +1664,9 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
                 : OFPERR_OFPFMFC_TABLE_FULL);
     }
 
-    return 0;
+    return ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
+                         oh->version > OFP10_VERSION, max_port,
+                         fm->table_id, max_table);
 }
 
 static enum ofperr

lib/ofp-util.h

@@ -294,7 +294,9 @@ struct ofputil_flow_mod {
 enum ofperr ofputil_decode_flow_mod(struct ofputil_flow_mod *,
                                     const struct ofp_header *,
                                     enum ofputil_protocol,
-                                    struct ofpbuf *ofpacts);
+                                    struct ofpbuf *ofpacts,
+                                    ofp_port_t max_port,
+                                    uint8_t max_table);
 struct ofpbuf *ofputil_encode_flow_mod(const struct ofputil_flow_mod *,
                                        enum ofputil_protocol);
 

ofproto/ofproto.c

@@ -271,9 +271,12 @@ static enum ofperr modify_flows__(struct ofproto *, struct ofconn *,
 static void delete_flow__(struct rule *rule, struct ofopgroup *,
                           enum ofp_flow_removed_reason)
     OVS_REQUIRES(ofproto_mutex);
+static bool ofproto_group_exists__(const struct ofproto *ofproto,
+                                   uint32_t group_id)
+    OVS_REQ_RDLOCK(ofproto->groups_rwlock);
 static bool ofproto_group_exists(const struct ofproto *ofproto,
                                  uint32_t group_id)
-    OVS_REQ_RDLOCK(ofproto->groups_rwlock);
+    OVS_EXCLUDED(ofproto->groups_rwlock);
 static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *);
 static bool handle_openflow(struct ofconn *, const struct ofpbuf *);
 static enum ofperr handle_flow_mod__(struct ofproto *, struct ofconn *,
@@ -2829,46 +2832,33 @@ reject_slave_controller(struct ofconn *ofconn)
     }
 }
 
-/* Checks that the 'ofpacts_len' bytes of actions in 'ofpacts' are appropriate
+/* Checks that the 'ofpacts_len' bytes of action in 'ofpacts' are appropriate
- * for a packet with the prerequisites satisfied by 'flow' in table 'table_id'.
+ * for 'ofproto':
- * 'flow' may be temporarily modified, but is restored at return.
+ *
- */
+ *    - If they use a meter, then 'ofproto' has that meter configured.
+ *
+ *    - If they use any groups, then 'ofproto' has that group configured.
+ *
+ * Returns 0 if successful, otherwise an OpenFlow error. */
 static enum ofperr
 ofproto_check_ofpacts(struct ofproto *ofproto,
-                      struct ofpact ofpacts[], size_t ofpacts_len,
+                      const struct ofpact ofpacts[], size_t ofpacts_len)
-                      struct flow *flow, uint8_t table_id,
-                      const struct ofp_header *oh)
 {
-    enum ofperr error;
     const struct ofpact *a;
     uint32_t mid;
 
-    error = ofpacts_check(ofpacts, ofpacts_len, flow,
-                          u16_to_ofp(ofproto->max_ports), table_id,
-                          oh && oh->version > OFP10_VERSION);
-    if (error) {
-        return error;
-    }
-
-    OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
-        if (a->type == OFPACT_GROUP) {
-            bool exists;
-
-            ovs_rwlock_rdlock(&ofproto->groups_rwlock);
-            exists = ofproto_group_exists(ofproto,
-                                          ofpact_get_GROUP(a)->group_id);
-            ovs_rwlock_unlock(&ofproto->groups_rwlock);
-
-            if (!exists) {
-                return OFPERR_OFPBAC_BAD_OUT_GROUP;
-            }
-        }
-    }
-
     mid = ofpacts_get_meter(ofpacts, ofpacts_len);
     if (mid && get_provider_meter_id(ofproto, mid) == UINT32_MAX) {
         return OFPERR_OFPMMFC_INVALID_METER;
     }
+
+    OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
+        if (a->type == OFPACT_GROUP
+            && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) {
+            return OFPERR_OFPBAC_BAD_OUT_GROUP;
+        }
+    }
+
     return 0;
 }
 
@@ -2918,7 +2908,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, 0, oh);
+    error = ofproto_check_ofpacts(p, po.ofpacts, po.ofpacts_len);
     if (!error) {
         error = p->ofproto_class->packet_out(p, payload, &flow,
                                              po.ofpacts, po.ofpacts_len);
@@ -3966,14 +3956,6 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
         }
     }
 
-    /* Verify actions. */
-    error = ofproto_check_ofpacts(ofproto, fm->ofpacts, fm->ofpacts_len,
-                                  &fm->match.flow, table_id, request);
-    if (error) {
-        cls_rule_destroy(&cr);
-        return error;
-    }
-
     /* Serialize against pending deletion. */
     if (is_flow_deletion_pending(ofproto, &cr, table_id)) {
         cls_rule_destroy(&cr);
@@ -4072,19 +4054,6 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
     enum ofperr error;
     size_t i;
 
-    /* Verify actions before we start to modify any rules, to avoid partial
-     * flow table modifications. */
-    for (i = 0; i < rules->n; i++) {
-        struct rule *rule = rules->rules[i];
-
-        error = ofproto_check_ofpacts(ofproto, fm->ofpacts, fm->ofpacts_len,
-                                      &fm->match.flow, rule->table_id,
-                                      request);
-        if (error) {
-            return error;
-        }
-    }
-
     type = fm->command == OFPFC_ADD ? OFOPERATION_REPLACE : OFOPERATION_MODIFY;
     group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id);
     error = OFPERR_OFPBRC_EPERM;
@@ -4417,7 +4386,12 @@ 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);
+                                    &ofpacts,
+                                    u16_to_ofp(ofproto->max_ports),
+                                    ofproto->n_tables);
+    if (!error) {
+        error = ofproto_check_ofpacts(ofproto, fm.ofpacts, fm.ofpacts_len);
+    }
     if (!error) {
         error = handle_flow_mod__(ofproto, ofconn, &fm, oh);
     }
@@ -5321,7 +5295,7 @@ ofproto_group_write_lookup(const struct ofproto *ofproto, uint32_t group_id,
 }
 
 static bool
-ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id)
+ofproto_group_exists__(const struct ofproto *ofproto, uint32_t group_id)
     OVS_REQ_RDLOCK(ofproto->groups_rwlock)
 {
     struct ofgroup *grp;
@@ -5335,6 +5309,19 @@ ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id)
     return false;
 }
 
+static bool
+ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id)
+    OVS_EXCLUDED(ofproto->groups_rwlock)
+{
+    bool exists;
+
+    ovs_rwlock_rdlock(&ofproto->groups_rwlock);
+    exists = ofproto_group_exists__(ofproto, group_id);
+    ovs_rwlock_unlock(&ofproto->groups_rwlock);
+
+    return exists;
+}
+
 static uint32_t
 group_get_ref_count(struct ofgroup *group)
     OVS_EXCLUDED(ofproto_mutex)
@@ -5569,7 +5556,7 @@ add_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
         goto unlock_out;
     }
 
-    if (ofproto_group_exists(ofproto, gm->group_id)) {
+    if (ofproto_group_exists__(ofproto, gm->group_id)) {
         error = OFPERR_OFPGMFC_GROUP_EXISTS;
         goto unlock_out;
     }

utilities/ovs-ofctl.c

@@ -3077,8 +3077,9 @@ ofctl_parse_ofp11_instructions(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
             /* Verify actions, enforce consistency. */
             struct flow flow;
             memset(&flow, 0, sizeof flow);
-            error = ofpacts_check(ofpacts.data, ofpacts.size, &flow, OFPP_MAX,
+            error = ofpacts_check(ofpacts.data, ofpacts.size, &flow,
-                                  table_id ? atoi(table_id) : 0, true);
+                                  true, OFPP_MAX,
+                                  table_id ? atoi(table_id) : 0, 255);
         }
         if (error) {
             printf("bad OF1.1 instructions: %s\n\n", ofperr_get_name(error));