multipath: Validate multipath actions more thoroughly in multipath_parse().

The stricter validation requires updates to the calls to test-multipath
to supply a valid n_links value.  test-multipath doesn't actually use
that value (it runs over different values in an internal "for" loop), so
this doesn't change any behavior.

Also adds a test to exercise each possible multipath_parse() error message.

Reported-by: Reid Price <reid@nicira.com>
Bug #4462.

diff --git a/lib/multipath.c b/lib/multipath.c
index 19e7b3610083f82f0e61bfdd638abc1c4256d4a0..07e0ada209cfca86f59aa1acb07df50c510cb2f0 100644 (file)
--- a/lib/multipath.c
+++ b/lib/multipath.c
@@ -183,18 +183,19 @@ multipath_parse(struct nx_action_multipath *mp, const char *s_)
 {
     char *s = xstrdup(s_);
     char *save_ptr = NULL;
-    char *fields, *basis, *algorithm, *n_links, *arg, *dst;
+    char *fields, *basis, *algorithm, *n_links_str, *arg, *dst;
     uint32_t header;
     int ofs, n_bits;
+    int n_links;
 
     fields = strtok_r(s, ", ", &save_ptr);
     basis = strtok_r(NULL, ", ", &save_ptr);
     algorithm = strtok_r(NULL, ", ", &save_ptr);
-    n_links = strtok_r(NULL, ", ", &save_ptr);
+    n_links_str = strtok_r(NULL, ", ", &save_ptr);
     arg = strtok_r(NULL, ", ", &save_ptr);
     dst = strtok_r(NULL, ", ", &save_ptr);
     if (!dst) {
-        ovs_fatal(0, "%s: not enough arguments to multipath action", s);
+        ovs_fatal(0, "%s: not enough arguments to multipath action", s_);
     }
 
     memset(mp, 0, sizeof *mp);
@@ -207,7 +208,7 @@ multipath_parse(struct nx_action_multipath *mp, const char *s_)
     } else if (!strcasecmp(fields, "symmetric_l4")) {
         mp->fields = htons(NX_MP_FIELDS_SYMMETRIC_L4);
     } else {
-        ovs_fatal(0, "%s: unknown fields `%s'", s, fields);
+        ovs_fatal(0, "%s: unknown fields `%s'", s_, fields);
     }
     mp->basis = htons(atoi(basis));
     if (!strcasecmp(algorithm, "modulo_n")) {
@@ -219,12 +220,25 @@ multipath_parse(struct nx_action_multipath *mp, const char *s_)
     } else if (!strcasecmp(algorithm, "iter_hash")) {
         mp->algorithm = htons(NX_MP_ALG_ITER_HASH);
     } else {
-        ovs_fatal(0, "%s: unknown algorithm `%s'", s, algorithm);
+        ovs_fatal(0, "%s: unknown algorithm `%s'", s_, algorithm);
     }
-    mp->max_link = htons(atoi(n_links) - 1);
+    n_links = atoi(n_links_str);
+    if (n_links < 1 || n_links > 65536) {
+        ovs_fatal(0, "%s: n_links %d is not in valid range 1 to 65536",
+                  s_, n_links);
+    }
+    mp->max_link = htons(n_links - 1);
     mp->arg = htonl(atoi(arg));
 
     nxm_parse_field_bits(dst, &header, &ofs, &n_bits);
+    if (!NXM_IS_NX_REG(header) || NXM_NX_REG_IDX(header) >= FLOW_N_REGS) {
+        ovs_fatal(0, "%s: destination field must be register", s_);
+    }
+    if (n_bits < 16 && n_links > (1u << n_bits)) {
+        ovs_fatal(0, "%s: %d-bit destination field has %u possible values, "
+                  "less than specified n_links %d",
+                  s_, n_bits, 1u << n_bits, n_links);
+    }
     mp->ofs_nbits = nxm_encode_ofs_nbits(ofs, n_bits);
     mp->dst = htonl(header);
 

diff --git a/tests/multipath.at b/tests/multipath.at
index a5a1a7be9cbc70b152047f499a0b8d1716230a8d..6eafa9a3f750aa7eacb65265e9091922c4c8c400 100644 (file)
--- a/tests/multipath.at
+++ b/tests/multipath.at
@@ -8,7 +8,7 @@ AT_BANNER([multipath link selection])
 # if the test does fail.
 
 AT_SETUP([modulo_n multipath link selection])
-AT_CHECK([[test-multipath 'eth_src,50,modulo_n,0,0,NXM_NX_REG0[]']],
+AT_CHECK([[test-multipath 'eth_src,50,modulo_n,1,0,NXM_NX_REG0[]']],
   [0], [ignore])
 # 1 ->  2: disruption=0.50 (perfect=0.50); stddev/expected=0.0000
 # 2 ->  3: disruption=0.66 (perfect=0.33); stddev/expected=0.0023
@@ -76,7 +76,7 @@ AT_CHECK([[test-multipath 'eth_src,50,modulo_n,0,0,NXM_NX_REG0[]']],
 AT_CLEANUP
 
 AT_SETUP([hash_threshold multipath link selection])
-AT_CHECK([[test-multipath 'eth_src,50,hash_threshold,0,0,NXM_NX_REG0[]']],
+AT_CHECK([[test-multipath 'eth_src,50,hash_threshold,1,0,NXM_NX_REG0[]']],
   [0], [ignore])
 # 1 ->  2: disruption=0.50 (perfect=0.50); stddev/expected=0.0000
 # 2 ->  3: disruption=0.50 (perfect=0.33); stddev/expected=0.0056
@@ -144,7 +144,7 @@ AT_CHECK([[test-multipath 'eth_src,50,hash_threshold,0,0,NXM_NX_REG0[]']],
 AT_CLEANUP
 
 AT_SETUP([hrw multipath link selection])
-AT_CHECK([[test-multipath 'eth_src,50,hrw,0,0,NXM_NX_REG0[]']],
+AT_CHECK([[test-multipath 'eth_src,50,hrw,1,0,NXM_NX_REG0[]']],
   [0], [ignore])
 # 1 ->  2: disruption=0.50 (perfect=0.50); stddev/expected=0.0000
 # 2 ->  3: disruption=0.33 (perfect=0.33); stddev/expected=0.0033
@@ -212,7 +212,7 @@ AT_CHECK([[test-multipath 'eth_src,50,hrw,0,0,NXM_NX_REG0[]']],
 AT_CLEANUP
 
 AT_SETUP([iter_hash multipath link selection])
-AT_CHECK([[test-multipath 'eth_src,50,iter_hash,0,0,NXM_NX_REG0[]']],
+AT_CHECK([[test-multipath 'eth_src,50,iter_hash,1,0,NXM_NX_REG0[]']],
   [0], [ignore])
 # 1 ->  2: disruption=0.50 (perfect=0.50); stddev/expected=0.0000
 # 2 ->  3: disruption=0.42 (perfect=0.33); stddev/expected=0.0034
@@ -278,3 +278,40 @@ AT_CHECK([[test-multipath 'eth_src,50,iter_hash,0,0,NXM_NX_REG0[]']],
 #62 -> 63: disruption=0.02 (perfect=0.02); stddev/expected=0.0292
 #63 -> 64: disruption=0.02 (perfect=0.02); stddev/expected=0.0307
 AT_CLEANUP
+
+AT_SETUP([multipath action missing argument])
+AT_CHECK([ovs-ofctl parse-flow actions=multipath], [1], [],
+  [ovs-ofctl: : not enough arguments to multipath action
+])
+AT_CLEANUP
+
+AT_SETUP([multipath action bad fields])
+AT_CHECK([ovs-ofctl parse-flow 'actions=multipath(xyzzy,50,modulo_n,1,0,NXM_NX_REG0[[]])'], [1], [],
+  [ovs-ofctl: xyzzy,50,modulo_n,1,0,NXM_NX_REG0[[]]: unknown fields `xyzzy'
+])
+AT_CLEANUP
+
+AT_SETUP([multipath action bad algorithm])
+AT_CHECK([ovs-ofctl parse-flow 'actions=multipath(eth_src,50,fubar,1,0,NXM_NX_REG0[[]])'], [1], [],
+  [ovs-ofctl: eth_src,50,fubar,1,0,NXM_NX_REG0[[]]: unknown algorithm `fubar'
+])
+AT_CLEANUP
+
+AT_SETUP([multipath action bad n_links])
+AT_CHECK([ovs-ofctl parse-flow 'actions=multipath(eth_src,50,modulo_n,0,0,NXM_NX_REG0[[]])'], [1], [],
+  [ovs-ofctl: eth_src,50,modulo_n,0,0,NXM_NX_REG0[[]]: n_links 0 is not in valid range 1 to 65536
+])
+AT_CLEANUP
+
+AT_SETUP([multipath action bad destination])
+AT_CHECK([ovs-ofctl parse-flow 'actions=multipath(eth_src,50,modulo_n,1,0,NXM_OF_VLAN_TCI[[]])'], [1], [],
+  [ovs-ofctl: eth_src,50,modulo_n,1,0,NXM_OF_VLAN_TCI[[]]: destination field must be register
+])
+AT_CLEANUP
+
+AT_SETUP([multipath action destination too narrow])
+AT_CHECK([ovs-ofctl parse-flow 'actions=multipath(eth_src,50,modulo_n,1024,0,NXM_NX_REG0[[0..7]])'], [1], [],
+  [ovs-ofctl: eth_src,50,modulo_n,1024,0,NXM_NX_REG0[[0..7]]: 8-bit destination field has 256 possible values, less than specified n_links 1024
+])
+AT_CLEANUP
+

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 68f2edafff5ee13123aed60129c353386867a523..9f5a0a524f12640a06d930f74b7f3522a210d86c 100644 (file)
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -884,6 +884,36 @@ do_help(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
 \f
 /* Undocumented commands for unit testing. */
 
+static void
+print_packet_list(struct list *packets)
+{
+    struct ofpbuf *packet, *next;
+
+    LIST_FOR_EACH_SAFE (packet, next, list_node, packets) {
+        ofp_print(stdout, packet->data, packet->size, verbosity);
+        list_remove(&packet->list_node);
+        ofpbuf_delete(packet);
+    }
+}
+
+/* "parse-flow FLOW": parses the argument as a flow (like add-flow) and prints
+ * it back to stdout.  */
+static void
+do_parse_flow(int argc OVS_UNUSED, char *argv[])
+{
+    enum nx_flow_format flow_format;
+    struct list packets;
+
+    flow_format = NXFF_OPENFLOW10;
+    if (preferred_flow_format > 0) {
+        flow_format = preferred_flow_format;
+    }
+
+    list_init(&packets);
+    parse_ofp_flow_mod_str(&packets, &flow_format, argv[1], OFPFC_ADD);
+    print_packet_list(&packets);
+}
+
 /* "parse-flows FILENAME": reads the named file as a sequence of flows (like
  * add-flows) and prints each of the flows back to stdout.  */
 static void
@@ -898,20 +928,14 @@ do_parse_flows(int argc OVS_UNUSED, char *argv[])
         ovs_fatal(errno, "%s: open", argv[2]);
     }
 
-    list_init(&packets);
     flow_format = NXFF_OPENFLOW10;
     if (preferred_flow_format > 0) {
         flow_format = preferred_flow_format;
     }
 
+    list_init(&packets);
     while (parse_ofp_add_flow_file(&packets, &flow_format, file)) {
-        struct ofpbuf *packet, *next;
-
-        LIST_FOR_EACH_SAFE (packet, next, list_node, &packets) {
-            ofp_print(stdout, packet->data, packet->size, verbosity);
-            list_remove(&packet->list_node);
-            ofpbuf_delete(packet);
-        }
+        print_packet_list(&packets);
     }
     fclose(file);
 }
@@ -1011,6 +1035,7 @@ static const struct command all_commands[] = {
     { "help", 0, INT_MAX, do_help },
 
     /* Undocumented commands for testing. */
+    { "parse-flow", 1, 1, do_parse_flow },
     { "parse-flows", 1, 1, do_parse_flows },
     { "parse-nx-match", 0, 0, do_parse_nx_match },
     { "ofp-print", 1, 2, do_ofp_print },