Skip to content

Commit

Permalink
multipath: Validate multipath actions more thoroughly in multipath_pa…
Browse files Browse the repository at this point in the history 
…rse().

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 <[email protected]>
Bug #4462.
  • Loading branch information
@blp
blp committed Feb 23, 2011
1 parent c4894ed commit 770f1f6
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 18 deletions.
26 changes: 20 additions & 6 deletions lib/multipath.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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")) {
Expand All @@ -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);

Expand Down
45 changes: 41 additions & 4 deletions tests/multipath.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

41 changes: 33 additions & 8 deletions utilities/ovs-ofctl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
Expand Down Expand Up @@ -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 },
Expand Down

0 comments on commit 770f1f6

Please sign in to comment.