From 770f1f66bb9dc4e784e6c2c5351089c62b9a5919 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Tue, 22 Feb 2011 16:24:19 -0800 Subject: [PATCH] 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 Bug #4462. --- lib/multipath.c | 26 +++++++++++++++++++------ tests/multipath.at | 45 +++++++++++++++++++++++++++++++++++++++---- utilities/ovs-ofctl.c | 41 +++++++++++++++++++++++++++++++-------- 3 files changed, 94 insertions(+), 18 deletions(-) diff --git a/lib/multipath.c b/lib/multipath.c index 19e7b361..07e0ada2 100644 --- 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 a5a1a7be..6eafa9a3 100644 --- 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 68f2edaf..9f5a0a52 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -884,6 +884,36 @@ do_help(int argc OVS_UNUSED, char *argv[] OVS_UNUSED) /* 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 }, -- 2.30.2