ofp-actions: Fix variable length meta-flow OXMs.

Previously, if a flow action that involves a tunnel metadata meta-flow
field is dumped from vswitchd, the replied field length in the OXM header
is filled with the maximum possible field length, instead of the length
configured in the tunnel TLV mapping table. To solve this issue, this patch
introduces the following changes.

In order to maintain the correct length of variable length mf_fields (i.e.
tun_metadata), this patch creates a per-switch based map (struct vl_mff_map)
that hosts the variable length mf_fields. This map is updated when a
controller adds/deletes tlv-mapping entries to/from a switch. Although the
per-swtch based vl_mff_map only hosts tun_metadata for now, it is able to
support new variable length mf_fields in the future.

With this commit, when a switch decodes a flow action with mf_field, the switch
firstly looks up the global mf_fields map to identify the mf_field type. For
the variable length mf_fields, the switch uses the vl_mff_map to get the
configured mf_field entries. By lookig up vl_mff_map, the switch can check
if the added flow action access beyond the configured size of a variable
length mf_field, and the switch reports an ofperr if the controller adds a flow
with unmapped variable length mf_field. Later on, when a controller request
flows from the switch, with the per-switch based mf_fields, the switch will
encode the OXM header with correct length for variable length mf_fields.

To use the vl_mff_map for decoding flow actions, extract-ofp-actions is
updated to pass the vl_mff_map to the required action decoding functions.
Also, a new error code is introduced to identify a flow with an invalid
variable length mf_field. Moreover, a testcase is added to prevent future
regressions.

Committer notes:
 - Factor out common code
 - Style fixups
 - Rename OFPERR_NXFMFC_INVALID_VL_MFF -> OFPERR_NXFMFC_INVALID_TLV_FIELD

VMWare-BZ: #1768370
Reported-by: Harold Lim <[email protected]>
Suggested-by: Joe Stringer <[email protected]>
Suggested-by: Jarno Rajahalme <[email protected]>
Signed-off-by: Yi-Hung Wei <[email protected]>
Signed-off-by: Joe Stringer <[email protected]>

build-aux/extract-ofp-actions

@@ -123,7 +123,13 @@ def extract_ofp_actions(fn, definitions):
             fatal("unexpected syntax between actions")
 
         dsts = m.group(1)
-        argtype = m.group(2).strip().replace('.', '', 1)
+        argtypes = m.group(2).strip().replace('.', '', 1)
+
+        if 'VLMFF' in argtypes:
+            arg_vl_mff_map = True
+        else:
+            arg_vl_mff_map = False
+        argtype = argtypes.replace('VLMFF', '', 1).rstrip()
 
         get_line()
         m = re.match(r'\s+(([A-Z]+)_RAW([0-9]*)_([A-Z0-9_]+)),?', line)
@@ -210,17 +216,18 @@ def extract_ofp_actions(fn, definitions):
                     else:
                         max_length = min_length
 
-                    info = {"enum": enum,                 # 0
-                            "deprecation": deprecation,   # 1
-                            "file_name": file_name,       # 2
-                            "line_number": line_number,   # 3
-                            "min_length": min_length,     # 4
-                            "max_length": max_length,     # 5
-                            "arg_ofs": arg_ofs,           # 6
-                            "arg_len": arg_len,           # 7
-                            "base_argtype": base_argtype, # 8
-                            "version": version,           # 9
-                            "type": type_}                # 10
+                    info = {"enum": enum,                     # 0
+                            "deprecation": deprecation,       # 1
+                            "file_name": file_name,           # 2
+                            "line_number": line_number,       # 3
+                            "min_length": min_length,         # 4
+                            "max_length": max_length,         # 5
+                            "arg_ofs": arg_ofs,               # 6
+                            "arg_len": arg_len,               # 7
+                            "base_argtype": base_argtype,     # 8
+                            "arg_vl_mff_map": arg_vl_mff_map, # 9
+                            "version": version,               # 10
+                            "type": type_}                    # 11
                     domain[vendor][type_][version] = info
 
                     enums.setdefault(enum, [])
@@ -314,14 +321,16 @@ def extract_ofp_actions(fn, definitions):
         print """\
 static enum ofperr
 ofpact_decode(const struct ofp_action_header *a, enum ofp_raw_action_type raw,
-              enum ofp_version version, uint64_t arg, struct ofpbuf *out)
+              enum ofp_version version, uint64_t arg,
+              const struct vl_mff_map *vl_mff_map, struct ofpbuf *out)
 {
     switch (raw) {\
 """
         for versions in enums.values():
             enum = versions[0]["enum"]
             print "    case %s:" % enum
             base_argtype = versions[0]["base_argtype"]
+            arg_vl_mff_map = versions[0]["arg_vl_mff_map"]
             if base_argtype == 'void':
                 print "        return decode_%s(out);" % enum
             else:
@@ -333,7 +342,10 @@ ofpact_decode(const struct ofp_action_header *a, enum ofp_raw_action_type raw,
                         arg = "%s(arg)" % hton
                     else:
                         arg = "arg"
-                print "        return decode_%s(%s, version, out);" % (enum, arg)
+                if arg_vl_mff_map:
+                    print "        return decode_%s(%s, version, vl_mff_map, out);" % (enum, arg)
+                else:
+                    print "        return decode_%s(%s, version, out);" % (enum, arg)
             print
         print """\
     default:
@@ -346,19 +358,23 @@ ofpact_decode(const struct ofp_action_header *a, enum ofp_raw_action_type raw,
             enum = versions[0]["enum"]
             prototype = "static enum ofperr decode_%s(" % enum
             base_argtype = versions[0]["base_argtype"]
+            arg_vl_mff_map = versions[0]["arg_vl_mff_map"]
             if base_argtype != 'void':
                 if base_argtype.startswith('struct'):
                     prototype += "const %s *, enum ofp_version, " % base_argtype
                 else:
                     prototype += "%s, enum ofp_version, " % base_argtype
+                if arg_vl_mff_map:
+                    prototype += 'const struct vl_mff_map *, '
             prototype += "struct ofpbuf *);"
             print prototype
 
         print """
 static enum ofperr ofpact_decode(const struct ofp_action_header *,
                                  enum ofp_raw_action_type raw,
                                  enum ofp_version version,
-                                 uint64_t arg, struct ofpbuf *out);
+                                 uint64_t arg, const struct vl_mff_map *vl_mff_map,
+                                 struct ofpbuf *out);
 """
 
 if __name__ == '__main__':

build-aux/extract-ofp-fields

@@ -334,7 +334,7 @@ def make_meta_flow(meta_flow_h):
             rw = 'true'
         else:
             rw = 'false'
-        output += ["    %s, %s, %s, %s,"
+        output += ["    %s, %s, %s, %s, false,"
                    % (f['mask'], f['string'], PREREQS[f['prereqs']], rw)]
 
         oxm = f['OXM']
@@ -386,7 +386,7 @@ def make_meta_flow(meta_flow_h):
         else:
             output += ["    -1, /* not usable for prefix lookup */"]
 
-        output += ["},"]
+        output += ["    {OVSRCU_INITIALIZER(NULL)},},"]
     for oline in output:
         print(oline)
 

include/openvswitch/meta-flow.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+ * Copyright (c) 2011-2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -23,13 +23,16 @@
 #include <sys/types.h>
 #include <netinet/in.h>
 #include <netinet/ip6.h>
+#include "cmap.h"
 #include "openvswitch/flow.h"
 #include "openvswitch/ofp-errors.h"
 #include "openvswitch/packets.h"
+#include "openvswitch/thread.h"
 #include "openvswitch/util.h"
 
 struct ds;
 struct match;
+struct ofputil_tlv_table_mod;
 
 /* Open vSwitch fields
  * ===================
@@ -1751,6 +1754,7 @@ struct mf_field {
     enum mf_string string;
     enum mf_prereqs prereqs;
     bool writable;              /* May be written by actions? */
+    bool mapped;                /* Variable length mf_field is mapped. */
 
     /* Usable protocols.
      *
@@ -1770,6 +1774,9 @@ struct mf_field {
 
     int flow_be32ofs;  /* Field's be32 offset in "struct flow", if prefix tree
                         * lookup is supported for the field, or -1. */
+
+    /* For variable length mf_fields only. In ofproto->vl_mff_map->cmap. */
+    struct cmap_node cmap_node;
 };
 
 /* The representation of a field's value. */
@@ -1846,6 +1853,14 @@ union mf_subvalue {
 };
 BUILD_ASSERT_DECL(sizeof(union mf_value) == sizeof (union mf_subvalue));
 
+/* Variable length mf_fields mapping map. This is a single writer,
+ * multiple-reader hash table that a writer must hold the following mutex
+ * to access this map. */
+struct vl_mff_map {
+    struct cmap cmap;       /* Contains 'struct mf_field' */
+    struct ovs_mutex mutex;
+};
+
 bool mf_subvalue_intersect(const union mf_subvalue *a_value,
                            const union mf_subvalue *a_mask,
                            const union mf_subvalue *b_value,
@@ -1973,4 +1988,13 @@ void mf_format_subvalue(const union mf_subvalue *subvalue, struct ds *s);
 void field_array_set(enum mf_field_id id, const union mf_value *,
                      struct field_array *);
 
+/* Variable length fields. */
+void mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
+    OVS_REQUIRES(vl_mff_map->mutex);
+enum ofperr mf_vl_mff_map_mod_from_tun_metadata(
+    struct vl_mff_map *vl_mff_map, const struct ofputil_tlv_table_mod *)
+    OVS_REQUIRES(vl_mff_map->mutex);
+const struct mf_field * mf_get_vl_mff(const struct mf_field *,
+                                      const struct vl_mff_map *);
+bool mf_vl_mff_invalid(const struct mf_field *, const struct vl_mff_map *);
 #endif /* meta-flow.h */

include/openvswitch/ofp-actions.h

@@ -943,11 +943,14 @@ struct ofpact_unroll_xlate {
 enum ofperr ofpacts_pull_openflow_actions(struct ofpbuf *openflow,
                                           unsigned int actions_len,
                                           enum ofp_version version,
+                                          const struct vl_mff_map *,
                                           struct ofpbuf *ofpacts);
-enum ofperr ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
-                                               unsigned int instructions_len,
-                                               enum ofp_version version,
-                                               struct ofpbuf *ofpacts);
+enum ofperr
+ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
+                                   unsigned int instructions_len,
+                                   enum ofp_version version,
+                                   const struct vl_mff_map *vl_mff_map,
+                                   struct ofpbuf *ofpacts);
 enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
                           struct flow *, ofp_port_t max_ports,
                           uint8_t table_id, uint8_t n_tables,

include/openvswitch/ofp-errors.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+ * Copyright (c) 2008-2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -395,6 +395,10 @@ enum ofperr {
      * nxt_flow_mod_table_id extension is enabled. */
     OFPERR_NXFMFC_BAD_TABLE_ID,
 
+    /* NX1.0-1.1(1,536), NX1.2+(37).  Attempted to add a flow with an invalid
+     * variable length meta-flow field. */
+    OFPERR_NXFMFC_INVALID_TLV_FIELD,
+
 /* ## ---------------------- ## */
 /* ## OFPET_GROUP_MOD_FAILED ## */
 /* ## ---------------------- ## */

include/openvswitch/ofp-util.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+ * Copyright (c) 2008-2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -331,6 +331,7 @@ enum ofperr ofputil_decode_flow_mod(struct ofputil_flow_mod *,
                                     const struct ofp_header *,
                                     enum ofputil_protocol,
                                     const struct tun_table *,
+                                    const struct vl_mff_map *,
                                     struct ofpbuf *ofpacts,
                                     ofp_port_t max_port,
                                     uint8_t max_table);

lib/meta-flow.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+ * Copyright (c) 2011-2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@
 #include "openvswitch/dynamic-string.h"
 #include "nx-match.h"
 #include "openvswitch/ofp-util.h"
+#include "ovs-rcu.h"
 #include "ovs-thread.h"
 #include "packets.h"
 #include "random.h"
@@ -2639,3 +2640,109 @@ field_array_set(enum mf_field_id id, const union mf_value *value,
 
     memcpy(fa->values + offset, value, value_size);
 }
+
+static inline uint32_t
+mf_field_hash(uint32_t key)
+{
+    return hash_int(key, 0);
+}
+
+void
+mf_vl_mff_map_clear(struct vl_mff_map *vl_mff_map)
+    OVS_REQUIRES(vl_mff_map->mutex)
+{
+    struct mf_field *mf;
+
+    CMAP_FOR_EACH (mf, cmap_node, &vl_mff_map->cmap) {
+        cmap_remove(&vl_mff_map->cmap, &mf->cmap_node, mf_field_hash(mf->id));
+        ovsrcu_postpone(free, mf);
+    }
+}
+
+static struct mf_field *
+mf_get_vl_mff__(uint32_t id, const struct vl_mff_map *vl_mff_map)
+{
+    struct mf_field *field;
+
+    CMAP_FOR_EACH_WITH_HASH (field, cmap_node, mf_field_hash(id),
+                             &vl_mff_map->cmap) {
+        if (field->id == id) {
+            return field;
+        }
+    }
+
+    return NULL;
+}
+
+/* If 'mff' is a variable length field, looks up 'vl_mff_map', returns a
+ * pointer to the variable length meta-flow field corresponding to 'mff'.
+ * Returns NULL if no mapping is existed for 'mff'. */
+const struct mf_field *
+mf_get_vl_mff(const struct mf_field *mff,
+              const struct vl_mff_map *vl_mff_map)
+{
+    if (mff && mff->variable_len && vl_mff_map) {
+        return mf_get_vl_mff__(mff->id, vl_mff_map);
+    }
+
+    return NULL;
+}
+
+/* Updates the tun_metadata mf_field in 'vl_mff_map' according to 'ttm'.
+ * This function is supposed to be invoked after tun_metadata_table_mod(). */
+enum ofperr
+mf_vl_mff_map_mod_from_tun_metadata(struct vl_mff_map *vl_mff_map,
+                                    const struct ofputil_tlv_table_mod *ttm)
+    OVS_REQUIRES(vl_mff_map->mutex)
+{
+    struct ofputil_tlv_map *tlv_map;
+
+    if (ttm->command == NXTTMC_CLEAR) {
+        mf_vl_mff_map_clear(vl_mff_map);
+        return 0;
+    }
+
+    LIST_FOR_EACH (tlv_map, list_node, &ttm->mappings) {
+        unsigned int idx = MFF_TUN_METADATA0 + tlv_map->index;
+        struct mf_field *mf;
+
+        if (idx >= MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS) {
+            return OFPERR_NXTTMFC_BAD_FIELD_IDX;
+        }
+
+        switch (ttm->command) {
+        case NXTTMC_ADD:
+            mf = xmalloc(sizeof *mf);
+            *mf = mf_fields[idx];
+            mf->n_bytes = tlv_map->option_len;
+            mf->n_bits = tlv_map->option_len * 8;
+            mf->mapped = true;
+
+            cmap_insert(&vl_mff_map->cmap, &mf->cmap_node, mf_field_hash(idx));
+            break;
+
+        case NXTTMC_DELETE:
+            mf = mf_get_vl_mff__(idx, vl_mff_map);
+            if (mf) {
+                cmap_remove(&vl_mff_map->cmap, &mf->cmap_node,
+                            mf_field_hash(idx));
+                ovsrcu_postpone(free, mf);
+            }
+            break;
+
+        case NXTTMC_CLEAR:
+        default:
+            OVS_NOT_REACHED();
+        }
+    }
+
+    return 0;
+}
+
+/* Returns true if a variable length meta-flow field 'mff' is not mapped in
+ * the 'vl_mff_map'. */
+bool
+mf_vl_mff_invalid(const struct mf_field *mff, const struct vl_mff_map *map)
+{
+    return map && mff && mff->variable_len && !mff->mapped;
+}