Some different versions of OpenFlow require different consistency checking. This patch allows for three different variants to be checked.
1. OpenFlow1.0 This variant implicitly pushes a VLAN tag if one doesn't exist and an action to modify a VLAN tag is encountered. MPLS push is supported as a Nicira extension whose behaviour is the same as OpenFlow1.1 - 1.2. In practice Open vSwtich allows inconsistent OpenFlow 1.0 actions so this portion of the change is just for completeness. I do not believe it has any run-time affect. And I would not be opposed to folding this case into the handling of OpenFlow1.1 - 1.2 other than that I believe it will make the logic in parse_ofp_str__() and ofpact_check__() slightly less intuitive in parts. 2. OpenFlow1.1 - 1.2 This does not have the implicit VLAN tag push behaviour of OpenFlow1.0. An MPLS push action pushes an MPLS LSE after any VLAN tags that are present. 3. OpenFlow1.3 This does not have the implicit VLAN tag push behaviour of OpenFlow1.0. An MPLS push action pushes an MPLS LSE before any VLAN tags that are present. ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed and similar devices can't be used in order to effect this logic when ofpact_check__() is called via parse_ofp_str() as they are not filled in for specific OF versions at that time. Again, for OpenFlow1.0 and thus push_vlan_if_needed I believe this is cosmetic. But that is not the case for the MPLS portion of this change. The net effect of this change on run-time is: * To increase logging under some circumstances as ofpacts_check() may now be called up to four times instead of up to twice for each invocation of parse_ofp_str__() * To disallow MPLS push actions on VLAN flows using OpenFlow1.3. These did not comply to the OpenFlow1.3 as they would push an MPLS LSE after any VLAN tags that were present. A subsequent patch will support them in a manner that complies with the OpenFlow1.3. Signed-off-by: Simon Horman <ho...@verge.net.au> --- This patch is targeted as a pre-requisite for v2.48 of the "MPLS actions and matches" series. I am posting it separately as I would like some feedback on the approach I have taken and I believe it can be reviewed independently of the MPLS series. --- lib/ofp-actions.c | 31 ++++++++++++++++++++----------- lib/ofp-actions.h | 3 ++- lib/ofp-parse.c | 51 +++++++++++++++++++++++++++++++++++++++------------ lib/ofp-util.c | 6 +++--- tests/learn.at | 4 ++++ utilities/ovs-ofctl.c | 4 ++-- 6 files changed, 70 insertions(+), 29 deletions(-) diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index bae479e..f292fd4 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -1874,9 +1874,9 @@ ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports) * Modifies some actions, filling in fields that could not be properly set * without context. */ static enum ofperr -ofpact_check__(struct ofpact *a, struct flow *flow, - bool enforce_consistency, ofp_port_t max_ports, - uint8_t table_id, uint8_t n_tables) +ofpact_check__(enum ofp_version ofp_version, struct ofpact *a, + struct flow *flow, bool enforce_consistency, + ofp_port_t max_ports, uint8_t table_id, uint8_t n_tables) { const struct ofpact_enqueue *enqueue; const struct mf_field *mf; @@ -1910,7 +1910,7 @@ ofpact_check__(struct ofpact *a, struct flow *flow, ofpact_get_SET_VLAN_VID(a)->flow_has_vlan = (flow->vlan_tci & htons(VLAN_CFI)) == htons(VLAN_CFI); if (!(flow->vlan_tci & htons(VLAN_CFI)) && - !ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) { + ofp_version >= OFP11_VERSION) { goto inconsistent; } /* Temporary mark that we have a vlan tag. */ @@ -1923,7 +1923,7 @@ ofpact_check__(struct ofpact *a, struct flow *flow, ofpact_get_SET_VLAN_PCP(a)->flow_has_vlan = (flow->vlan_tci & htons(VLAN_CFI)) == htons(VLAN_CFI); if (!(flow->vlan_tci & htons(VLAN_CFI)) && - !ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) { + ofp_version >= OFP11_VERSION) { goto inconsistent; } /* Temporary mark that we have a vlan tag. */ @@ -2054,9 +2054,16 @@ ofpact_check__(struct ofpact *a, struct flow *flow, case OFPACT_EXIT: return 0; - case OFPACT_PUSH_MPLS: - flow->dl_type = ofpact_get_PUSH_MPLS(a)->ethertype; + case OFPACT_PUSH_MPLS: { + struct ofpact_push_mpls *oam = ofpact_get_PUSH_MPLS(a); + + flow->dl_type = oam->ethertype; + if (ofp_version >= OFP13_VERSION) { + /* Temporary mark that we have no vlan tag. */ + flow->vlan_tci = htons(0); + } return 0; + } case OFPACT_POP_MPLS: flow->dl_type = ofpact_get_POP_MPLS(a)->ethertype; @@ -2073,8 +2080,9 @@ ofpact_check__(struct ofpact *a, struct flow *flow, case OFPACT_WRITE_ACTIONS: { struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a); - return ofpacts_check(on->actions, ofpact_nest_get_action_len(on), - flow, false, max_ports, table_id, n_tables); + return ofpacts_check(ofp_version, on->actions, + ofpact_nest_get_action_len(on), flow, false, + max_ports, table_id, n_tables); } case OFPACT_WRITE_METADATA: @@ -2119,7 +2127,8 @@ ofpact_check__(struct ofpact *a, struct flow *flow, * * May temporarily modify 'flow', but restores the changes before returning. */ enum ofperr -ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len, +ofpacts_check(enum ofp_version ofp_version, + struct ofpact ofpacts[], size_t ofpacts_len, struct flow *flow, bool enforce_consistency, ofp_port_t max_ports, uint8_t table_id, uint8_t n_tables) @@ -2130,7 +2139,7 @@ ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len, enum ofperr error = 0; OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { - error = ofpact_check__(a, flow, enforce_consistency, + error = ofpact_check__(ofp_version, a, flow, enforce_consistency, max_ports, table_id, n_tables); if (error) { break; diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h index 70ad4b6..307b359 100644 --- a/lib/ofp-actions.h +++ b/lib/ofp-actions.h @@ -606,7 +606,8 @@ enum ofperr ofpacts_pull_openflow_instructions(struct ofpbuf *openflow, unsigned int instructions_len, enum ofp_version version, struct ofpbuf *ofpacts); -enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len, +enum ofperr ofpacts_check(enum ofp_version ofp_version, + struct ofpact[], size_t ofpacts_len, struct flow *, bool enforce_consistency, ofp_port_t max_ports, uint8_t table_id, uint8_t n_tables); diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 915dc90..7a92c64 100644 --- a/lib/ofp-parse.c +++ b/lib/ofp-parse.c @@ -1412,25 +1412,52 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string, error = str_to_inst_ofpacts(act_str, &ofpacts, usable_protocols); if (!error) { enum ofperr err; - - err = ofpacts_check(ofpacts.data, ofpacts.size, &fm->match.flow, - true, OFPP_MAX, fm->table_id, 255); - if (err) { + enum ofperr last_err = 0; + + /* + * XXX: As a side effect of multiple calls to ofpacts_check + * logging may be duplicated. + */ + if (*usable_protocols & OFPUTIL_P_OF10_ANY) { + err = ofpacts_check(OFP10_VERSION, ofpacts.data, ofpacts.size, + &fm->match.flow, true, OFPP_MAX, + fm->table_id, 255); if (!enforce_consistency && err == OFPERR_OFPBAC_MATCH_INCONSISTENT) { - /* Allow inconsistent actions to be used with OF 1.0. */ - *usable_protocols &= OFPUTIL_P_OF10_ANY; - /* Try again, allowing for inconsistency. - * XXX: As a side effect, logging may be duplicated. */ - err = ofpacts_check(ofpacts.data, ofpacts.size, - &fm->match.flow, false, + /* Try again, allowing for inconsistency. */ + err = ofpacts_check(OFP10_VERSION, ofpacts.data, + ofpacts.size, &fm->match.flow, false, OFPP_MAX, fm->table_id, 255); } if (err) { - error = xasprintf("actions are invalid with specified match " - "(%s)", ofperr_to_string(err)); + *usable_protocols &= ~OFPUTIL_P_OF10_ANY; + last_err = err; } } + if (*usable_protocols & (OFPUTIL_P_OF11_STD|OFPUTIL_P_OF12_OXM)) { + err = ofpacts_check(OFP11_VERSION, ofpacts.data, ofpacts.size, + &fm->match.flow, true, OFPP_MAX, + fm->table_id, 255); + if (err) { + *usable_protocols &= ~(OFPUTIL_P_OF11_STD| + OFPUTIL_P_OF12_OXM); + last_err = err; + } + } + if (*usable_protocols & OFPUTIL_P_OF13_UP) { + err = ofpacts_check(OFP13_VERSION, ofpacts.data, ofpacts.size, + &fm->match.flow, true, OFPP_MAX, + fm->table_id, 255); + if (err) { + *usable_protocols &= ~OFPUTIL_P_OF13_UP; + last_err = err; + } + } + if (!*usable_protocols && last_err) { + error = xasprintf("actions are invalid with specified match " + "(%s)", ofperr_to_string(err)); + } + } if (error) { ofpbuf_uninit(&ofpacts); diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 9645e04..40cd150 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -1672,9 +1672,9 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, : OFPERR_OFPFMFC_TABLE_FULL); } - return ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow, - oh->version > OFP10_VERSION, max_port, - fm->table_id, max_table); + return ofpacts_check(oh->version, fm->ofpacts, fm->ofpacts_len, + &fm->match.flow, oh->version > OFP10_VERSION, + max_port, fm->table_id, max_table); } static enum ofperr diff --git a/tests/learn.at b/tests/learn.at index 491cd5e..20fdc17 100644 --- a/tests/learn.at +++ b/tests/learn.at @@ -76,6 +76,8 @@ AT_CHECK([[ovs-ofctl parse-flow 'actions=learn(load:5->NXM_OF_IP_DST[])']], AT_CHECK([sed -e 's/.*|meta_flow|WARN|//' < stderr], [0], [[destination field ip_dst lacks correct prerequisites destination field ip_dst lacks correct prerequisites +destination field ip_dst lacks correct prerequisites +destination field ip_dst lacks correct prerequisites ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT) ]], [[]]) AT_CHECK([[ovs-ofctl parse-flow 'actions=learn(load:NXM_OF_IP_DST[]->NXM_NX_REG1[])']], @@ -83,6 +85,8 @@ AT_CHECK([[ovs-ofctl parse-flow 'actions=learn(load:NXM_OF_IP_DST[]->NXM_NX_REG1 AT_CHECK([sed -e 's/.*|meta_flow|WARN|//' < stderr], [0], [[source field ip_dst lacks correct prerequisites source field ip_dst lacks correct prerequisites +source field ip_dst lacks correct prerequisites +source field ip_dst lacks correct prerequisites ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT) ]]) AT_CLEANUP diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index a0dc5c8..f49093d 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -3077,8 +3077,8 @@ ofctl_parse_ofp11_instructions(int argc OVS_UNUSED, char *argv[] OVS_UNUSED) /* Verify actions, enforce consistency. */ struct flow flow; memset(&flow, 0, sizeof flow); - error = ofpacts_check(ofpacts.data, ofpacts.size, &flow, - true, OFPP_MAX, + error = ofpacts_check(OFP11_VERSION, ofpacts.data, ofpacts.size, + &flow, true, OFPP_MAX, table_id ? atoi(table_id) : 0, 255); } if (error) { -- 1.8.4 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev