All new accounts created on Gitlab now require administrator approval. If you invite any collaborators, please let Flux staff know so they can approve the accounts.

Commit b82ef59d authored by Ben Pfaff's avatar Ben Pfaff

Fix treatment of OpenFlow 1.1+ bucket weights.

Until now, OVS has parsed all OF1.1+ group buckets that lack a weight
as having weight 1.  Unfortunately, OpenFlow says that only "select"
groups may have a nonzero weight, and requires reporting an error for
other kinds of groups that have a nonzero weight.  This commit fixes
the problem by parsing only select groups with a default weight of 1
and other groups with a default weight of 0.  It also adds the
OpenFlow-required check for nonzero weights for other kinds of groups.

This complies with OpenFlow 1.1 and later.  OF1.1 says in section 5.8:

    If a specified group type is invalid (ie: includes fields such as
    weight that are undefined for the specified group type) then the
    switch must refuse to add the group entry and must send an
    ofp_error_msg with OFPET_GROUP_MOD_FAILED type and
    OFPGMFC_INVALID_GROUP code.

Found by OFTest.
Signed-off-by: default avatarBen Pfaff <blp@nicira.com>
Acked-by: default avatarFlavio Leitner <fbl@sysclose.org>
parent 277d3c45
......@@ -1123,7 +1123,7 @@ exit:
}
static char * OVS_WARN_UNUSED_RESULT
parse_bucket_str(struct ofputil_bucket *bucket, char *str_,
parse_bucket_str(struct ofputil_bucket *bucket, char *str_, uint8_t group_type,
enum ofputil_protocol *usable_protocols)
{
char *pos, *key, *value;
......@@ -1131,7 +1131,7 @@ parse_bucket_str(struct ofputil_bucket *bucket, char *str_,
struct ds actions;
char *error;
bucket->weight = 1;
bucket->weight = group_type == OFPGT11_SELECT ? 1 : 0;
bucket->bucket_id = OFPG15_BUCKET_ALL;
bucket->watch_port = OFPP_ANY;
bucket->watch_group = OFPG11_ANY;
......@@ -1313,40 +1313,19 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
*usable_protocols = OFPUTIL_P_OF11_UP;
if (fields & F_BUCKETS) {
char *bkt_str = strstr(string, "bucket=");
if (bkt_str) {
*bkt_str = '\0';
}
while (bkt_str) {
char *next_bkt_str;
bkt_str = strchr(bkt_str + 1, '=');
if (!bkt_str) {
error = xstrdup("must specify bucket content");
goto out;
}
bkt_str++;
next_bkt_str = strstr(bkt_str, "bucket=");
if (next_bkt_str) {
*next_bkt_str = '\0';
}
bucket = xzalloc(sizeof(struct ofputil_bucket));
error = parse_bucket_str(bucket, bkt_str, usable_protocols);
if (error) {
free(bucket);
goto out;
}
list_push_back(&gm->buckets, &bucket->list_node);
bkt_str = next_bkt_str;
/* Strip the buckets off the end of 'string', if there are any, saving a
* pointer for later. We want to parse the buckets last because the bucket
* type influences bucket defaults. */
char *bkt_str = strstr(string, "bucket=");
if (bkt_str) {
if (!(fields & F_BUCKETS)) {
error = xstrdup("bucket is not needed");
goto out;
}
*bkt_str = '\0';
}
/* Parse everything before the buckets. */
for (name = strtok_r(string, "=, \t\r\n", &save_ptr); name;
name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) {
char *value;
......@@ -1421,9 +1400,6 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
goto out;
}
had_type = true;
} else if (!strcmp(name, "bucket")) {
error = xstrdup("bucket is not needed");
goto out;
} else if (!strcmp(name, "selection_method")) {
if (!(fields & F_GROUP_TYPE)) {
error = xstrdup("selection method is not needed");
......@@ -1484,12 +1460,36 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
goto out;
}
/* Validate buckets. */
LIST_FOR_EACH (bucket, list_node, &gm->buckets) {
if (bucket->weight != 1 && gm->type != OFPGT11_SELECT) {
/* Now parse the buckets, if any. */
while (bkt_str) {
char *next_bkt_str;
bkt_str = strchr(bkt_str + 1, '=');
if (!bkt_str) {
error = xstrdup("must specify bucket content");
goto out;
}
bkt_str++;
next_bkt_str = strstr(bkt_str, "bucket=");
if (next_bkt_str) {
*next_bkt_str = '\0';
}
bucket = xzalloc(sizeof(struct ofputil_bucket));
error = parse_bucket_str(bucket, bkt_str, gm->type, usable_protocols);
if (error) {
free(bucket);
goto out;
}
list_push_back(&gm->buckets, &bucket->list_node);
if (gm->type != OFPGT11_SELECT && bucket->weight) {
error = xstrdup("Only select groups can have bucket weights.");
goto out;
}
bkt_str = next_bkt_str;
}
if (gm->type == OFPGT11_INDIRECT && !list_is_short(&gm->buckets)) {
error = xstrdup("Indirect groups can have at most one bucket.");
......
......@@ -2165,7 +2165,7 @@ ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type,
ds_put_cstr(s, "bucket=");
ofp_print_bucket_id(s, "bucket_id:", bucket->bucket_id, ofp_version);
if (bucket->weight != 1) {
if (bucket->weight != (type == OFPGT11_SELECT ? 1 : 0)) {
ds_put_format(s, "weight:%"PRIu16",", bucket->weight);
}
if (bucket->watch_port != OFPP_NONE) {
......
......@@ -7580,7 +7580,8 @@ parse_ofp15_group_bucket_prop_watch(const struct ofpbuf *payload,
static enum ofperr
ofputil_pull_ofp15_buckets(struct ofpbuf *msg, size_t buckets_length,
enum ofp_version version, struct ovs_list *buckets)
enum ofp_version version, uint8_t group_type,
struct ovs_list *buckets)
{
struct ofp15_bucket *ob;
......@@ -7593,7 +7594,7 @@ ofputil_pull_ofp15_buckets(struct ofpbuf *msg, size_t buckets_length,
size_t ob_len, actions_len, properties_len;
ovs_be32 watch_port = ofputil_port_to_ofp11(OFPP_ANY);
ovs_be32 watch_group = htonl(OFPG_ANY);
ovs_be16 weight = htons(1);
ovs_be16 weight = htons(group_type == OFPGT11_SELECT ? 1 : 0);
ofpbuf_init(&ofpacts, 0);
......@@ -7975,7 +7976,7 @@ ofputil_decode_ofp15_group_desc_reply(struct ofputil_group_desc *gd,
"bucket list length %u", bucket_list_len);
return OFPERR_OFPBRC_BAD_LEN;
}
error = ofputil_pull_ofp15_buckets(msg, bucket_list_len, version,
error = ofputil_pull_ofp15_buckets(msg, bucket_list_len, version, gd->type,
&gd->buckets);
if (error) {
return error;
......@@ -8273,7 +8274,7 @@ ofputil_pull_ofp15_group_mod(struct ofpbuf *msg, enum ofp_version ofp_version,
bucket_list_len = ntohs(ogm->bucket_array_len);
error = ofputil_pull_ofp15_buckets(msg, bucket_list_len, ofp_version,
&gm->buckets);
gm->type, &gm->buckets);
if (error) {
return error;
}
......@@ -8350,6 +8351,10 @@ ofputil_decode_group_mod(const struct ofp_header *oh,
}
LIST_FOR_EACH (bucket, list_node, &gm->buckets) {
if (bucket->weight && gm->type != OFPGT11_SELECT) {
return OFPERR_OFPGMFC_INVALID_GROUP;
}
switch (gm->type) {
case OFPGT11_ALL:
case OFPGT11_INDIRECT:
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment