[ovs-dev] [bug4462 3/3] multipath: Validate multipath actions more thoroughly in multipath_parse().
Justin Pettit
jpettit at nicira.com
Wed Feb 23 07:21:20 UTC 2011
Looks good.
--Justin
On Feb 22, 2011, at 4:28 PM, Ben Pfaff wrote:
> 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 at nicira.com>
> 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 19e7b36..07e0ada 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 a5a1a7b..6eafa9a 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 68f2eda..9f5a0a5 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 },
> --
> 1.7.1
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
More information about the dev
mailing list