From b82ef59d9e299aa72c0ce8256bd98ba10cd441cb Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Tue, 28 Jul 2015 22:01:24 -0700 Subject: [PATCH] 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: Ben Pfaff Acked-by: Flavio Leitner --- lib/ofp-parse.c | 78 ++++++++++++++++++++++++------------------------- lib/ofp-print.c | 2 +- lib/ofp-util.c | 13 ++++++--- 3 files changed, 49 insertions(+), 44 deletions(-) diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 210feed1e..43b210046 100644 --- a/lib/ofp-parse.c +++ b/lib/ofp-parse.c @@ -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."); diff --git a/lib/ofp-print.c b/lib/ofp-print.c index 2ac11b1ea..04fc7e447 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -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) { diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 3722bf16d..7ddf88aca 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -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: -- 2.20.1