ofp-table: Parse table features messages more carefully.

Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>

include/openflow/openflow-1.3.h

@@ -215,13 +215,24 @@ struct ofp13_table_stats {
 };
 OFP_ASSERT(sizeof(struct ofp13_table_stats) == 24);
 
+enum ofp15_table_features_command {
+    OFPTFC15_REPLACE = 0,       /* Replace full pipeline. */
+    OFPTFC15_MODIFY = 1,        /* Modify flow tables capabilities. */
+    OFPTFC15_ENABLE = 2,        /* Enable flow tables in the pipeline. */
+    OFPTFC15_DISABLE = 3,       /* Disable flow tables in pipeline. */
+};
+
 /* Body for ofp_multipart_request of type OFPMP_TABLE_FEATURES./
  * Body of reply to OFPMP_TABLE_FEATURES request. */
 struct ofp13_table_features {
     ovs_be16 length;          /* Length is padded to 64 bits. */
     uint8_t table_id;         /* Identifier of table. Lower numbered tables
                                  are consulted first. */
-    uint8_t pad[5];           /* Align to 64-bits. */
+
+    /* Added in OF1.5.  Earlier versions acted like OFPTFC15_REPLACE. */
+    uint8_t command;          /* One of OFPTFC15_*. */
+
+    uint8_t pad[4];           /* Align to 64-bits. */
     char name[OFP_MAX_TABLE_NAME_LEN];
     ovs_be64 metadata_match;  /* Bits of metadata table can match. */
     ovs_be64 metadata_write;  /* Bits of metadata table can write. */
@@ -260,6 +271,17 @@ enum ofp13_table_feature_prop_type {
     OFPTFPT13_APPLY_SETFIELD_MISS  = 15, /* Apply Set-Field for table-miss. */
     OFPTFPT13_EXPERIMENTER         = 0xFFFE, /* Experimenter property. */
     OFPTFPT13_EXPERIMENTER_MISS    = 0xFFFF, /* Experimenter for table-miss. */
+
+    /* OpenFlow says that each of these properties must occur exactly once. */
+#define OFPTFPT13_REQUIRED ((1u << OFPTFPT13_INSTRUCTIONS) |    \
+                            (1u << OFPTFPT13_NEXT_TABLES) |     \
+                            (1u << OFPTFPT13_WRITE_ACTIONS) |   \
+                            (1u << OFPTFPT13_APPLY_ACTIONS) |   \
+                            (1u << OFPTFPT13_MATCH) |           \
+                            (1u << OFPTFPT13_WILDCARDS) |       \
+                            (1u << OFPTFPT13_WRITE_SETFIELD) |  \
+                            (1u << OFPTFPT13_APPLY_SETFIELD))
+
 };
 
 /* Body of reply to OFPMP13_PORT request. If a counter is unsupported, set

include/openvswitch/ofp-table.h

@@ -187,13 +187,23 @@ void ofputil_table_desc_format(struct ds *,
  * include support for a subset of ofp_table_features through OFPST_TABLE (aka
  * OFPMP_TABLE). */
 struct ofputil_table_features {
-    uint8_t table_id;         /* Identifier of table. Lower numbered tables
+    /* Only for OFPT_TABLE_FEATURES requests and only the first table_features
-                                 are consulted first. */
+     * in such a request.  */
+    enum ofp15_table_features_command command;
+
+    /* The following are always present in table features requests and
+     * replies. */
+    uint8_t table_id;
     char name[OFP_MAX_TABLE_NAME_LEN];
     ovs_be64 metadata_match;  /* Bits of metadata table can match. */
     ovs_be64 metadata_write;  /* Bits of metadata table can write. */
     uint32_t max_entries;     /* Max number of entries supported. */
 
+    /* True if the message included any properties.  This is important for
+     * OFPT_TABLE_FEATURES requests, which change table properties only if any
+     * are included. */
+    bool any_properties;
+
     /* Flags.
      *
      * 'miss_config' is relevant for OpenFlow 1.1 and 1.2 only, because those

lib/ofp-table.c

@@ -74,6 +74,33 @@ ofputil_table_vacancy_to_string(enum ofputil_table_vacancy vacancy)
     default: return "***error***";
     }
 }
+
+static bool
+ofp15_table_features_command_is_valid(enum ofp15_table_features_command cmd)
+{
+    switch (cmd) {
+    case OFPTFC15_REPLACE:
+    case OFPTFC15_MODIFY:
+    case OFPTFC15_ENABLE:
+    case OFPTFC15_DISABLE:
+        return true;
+
+    default:
+        return false;
+    }
+}
+
+static const char *
+ofp15_table_features_command_to_string(enum ofp15_table_features_command cmd)
+{
+    switch (cmd) {
+    case OFPTFC15_REPLACE: return "replace";
+    case OFPTFC15_MODIFY: return "modify";
+    case OFPTFC15_ENABLE: return "enable";
+    case OFPTFC15_DISABLE: return "disable";
+    default: return "***bad command***";
+    }
+}
 
 /* ofputil_table_map.  */
 
@@ -361,6 +388,15 @@ ofputil_decode_table_features(struct ofpbuf *msg,
         return OFPERR_OFPBPC_BAD_LEN;
     }
 
+    if (oh->version >= OFP15_VERSION) {
+        if (!ofp15_table_features_command_is_valid(otf->command)) {
+            return OFPERR_OFPTFFC_BAD_COMMAND;
+        }
+        tf->command = otf->command;
+    } else {
+        tf->command = OFPTFC15_REPLACE;
+    }
+
     tf->table_id = otf->table_id;
     if (tf->table_id == OFPTT_ALL) {
         return OFPERR_OFPTFFC_BAD_TABLE;
@@ -382,7 +418,9 @@ ofputil_decode_table_features(struct ofpbuf *msg,
 
     struct ofpbuf properties = ofpbuf_const_initializer(ofpbuf_pull(msg, len),
                                                         len);
+    tf->any_properties = properties.size > 0;
     ofpbuf_pull(&properties, sizeof *otf);
+    uint32_t seen = 0;
     while (properties.size > 0) {
         struct ofpbuf payload;
         enum ofperr error;
@@ -393,6 +431,14 @@ ofputil_decode_table_features(struct ofpbuf *msg,
             return error;
         }
 
+        if (type < 32) {
+            uint32_t bit = 1u << type;
+            if (seen & bit) {
+                return OFPERR_OFPTFFC_BAD_FEATURES;
+            }
+            seen |= bit;
+        }
+
         switch ((enum ofp13_table_feature_prop_type) type) {
         case OFPTFPT13_INSTRUCTIONS:
             error = parse_instruction_ids(&payload, loose,
@@ -464,6 +510,36 @@ ofputil_decode_table_features(struct ofpbuf *msg,
         }
     }
 
+    /* OpenFlow 1.3 and 1.4 always require all of the required properties.
+     * OpenFlow 1.5 requires all of them if any property is present. */
+    if ((seen & OFPTFPT13_REQUIRED) != OFPTFPT13_REQUIRED
+        && (tf->any_properties || oh->version < OFP15_VERSION)) {
+        VLOG_WARN_RL(&rl, "table features message missing required property");
+        return OFPERR_OFPTFFC_BAD_FEATURES;
+    }
+
+    /* Copy nonmiss to miss when appropriate. */
+    if (tf->any_properties) {
+        if (!(seen & (1u << OFPTFPT13_INSTRUCTIONS_MISS))) {
+            tf->miss.instructions = tf->nonmiss.instructions;
+        }
+        if (!(seen & (1u << OFPTFPT13_NEXT_TABLES_MISS))) {
+            memcpy(tf->miss.next, tf->nonmiss.next, sizeof tf->miss.next);
+        }
+        if (!(seen & (1u << OFPTFPT13_WRITE_ACTIONS_MISS))) {
+            tf->miss.write.ofpacts = tf->nonmiss.write.ofpacts;
+        }
+        if (!(seen & (1u << OFPTFPT13_APPLY_ACTIONS_MISS))) {
+            tf->miss.apply.ofpacts = tf->nonmiss.apply.ofpacts;
+        }
+        if (!(seen & (1u << OFPTFPT13_WRITE_SETFIELD_MISS))) {
+            tf->miss.write.set_fields = tf->nonmiss.write.set_fields;
+        }
+        if (!(seen & (1u << OFPTFPT13_APPLY_SETFIELD_MISS))) {
+            tf->miss.apply.set_fields = tf->nonmiss.apply.set_fields;
+        }
+    }
+
     /* Fix inconsistencies:
      *
      *     - Turn on 'match' bits that are set in 'mask', because maskable
@@ -577,6 +653,7 @@ ofputil_append_table_features_reply(const struct ofputil_table_features *tf,
 
     otf = ofpbuf_put_zeros(reply, sizeof *otf);
     otf->table_id = tf->table_id;
+    otf->command = version >= OFP15_VERSION ? tf->command : 0;
     ovs_strlcpy_arrays(otf->name, tf->name);
     otf->metadata_match = tf->metadata_match;
     otf->metadata_write = tf->metadata_write;
@@ -1434,6 +1511,12 @@ ofputil_table_features_format(
     const struct ofputil_table_stats *prev_stats,
     int *first_ditto, int *last_ditto)
 {
+    if (!prev_features && features->command != OFPTFC15_REPLACE) {
+        ds_put_format(s, "\n  command: %s",
+                      ofp15_table_features_command_to_string(
+                          features->command));
+    }
+
     int table = features->table_id;
     int prev_table = prev_features ? prev_features->table_id : 0;