From 7e9f8266a4b6a951d3b8c0892d0874f4c9d44b3f Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 1 Nov 2013 21:45:28 -0700 Subject: [PATCH] ofproto: Centralize action checking, doing it at decode time. Jarno pointed out that modify_flows__() didn't really need to check every instance of the flow separately. After some further investigation I decided that this was even more of an improvement. CC: Jarno Rajahalme Signed-off-by: Ben Pfaff Acked-by: Jarno Rajahalme --- lib/ofp-actions.c | 23 +++++++---- lib/ofp-actions.h | 5 ++- lib/ofp-parse.c | 5 ++- lib/ofp-print.c | 3 +- lib/ofp-util.c | 7 +++- lib/ofp-util.h | 4 +- ofproto/ofproto.c | 95 +++++++++++++++++++------------------------ utilities/ovs-ofctl.c | 5 ++- 8 files changed, 74 insertions(+), 73 deletions(-) diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 216bc7a78..2b4b45c75 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -1823,8 +1823,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, ofp_port_t max_ports, - uint8_t table_id, bool enforce_consistency) +ofpact_check__(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; @@ -2020,7 +2021,7 @@ ofpact_check__(struct ofpact *a, struct flow *flow, ofp_port_t max_ports, 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, max_ports, table_id, false); + flow, false, max_ports, table_id, n_tables); } case OFPACT_WRITE_METADATA: @@ -2034,11 +2035,14 @@ ofpact_check__(struct ofpact *a, struct flow *flow, ofp_port_t max_ports, return 0; } - case OFPACT_GOTO_TABLE: - if (ofpact_get_GOTO_TABLE(a)->table_id <= table_id) { + case OFPACT_GOTO_TABLE: { + uint8_t goto_table = ofpact_get_GOTO_TABLE(a)->table_id; + if ((table_id != 255 && goto_table <= table_id) + || (n_tables != 255 && goto_table >= n_tables)) { return OFPERR_OFPBRC_BAD_TABLE_ID; } return 0; + } case OFPACT_GROUP: return 0; @@ -2063,8 +2067,9 @@ ofpact_check__(struct ofpact *a, struct flow *flow, ofp_port_t max_ports, * May temporarily modify 'flow', but restores the changes before returning. */ enum ofperr ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len, - struct flow *flow, ofp_port_t max_ports, uint8_t table_id, - bool enforce_consistency) + struct flow *flow, bool enforce_consistency, + ofp_port_t max_ports, + uint8_t table_id, uint8_t n_tables) { struct ofpact *a; ovs_be16 dl_type = flow->dl_type; @@ -2072,8 +2077,8 @@ 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, max_ports, table_id, - enforce_consistency); + error = ofpact_check__(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 ed40a9d9d..c7ce2767f 100644 --- a/lib/ofp-actions.h +++ b/lib/ofp-actions.h @@ -587,8 +587,9 @@ enum ofperr ofpacts_pull_openflow_instructions(struct ofpbuf *openflow, enum ofp_version version, struct ofpbuf *ofpacts); enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len, - struct flow *, ofp_port_t max_ports, - uint8_t table_id, bool enforce_consistency); + struct flow *, bool enforce_consistency, + ofp_port_t max_ports, + uint8_t table_id, uint8_t n_tables); enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len); enum ofperr ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports); diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 83413c23e..e7fdd3ba1 100644 --- a/lib/ofp-parse.c +++ b/lib/ofp-parse.c @@ -1368,7 +1368,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string, enum ofperr err; err = ofpacts_check(ofpacts.data, ofpacts.size, &fm->match.flow, - OFPP_MAX, 0, true); + true, OFPP_MAX, fm->table_id, 255); if (err) { if (!enforce_consistency && err == OFPERR_OFPBAC_MATCH_INCONSISTENT) { @@ -1377,7 +1377,8 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string, /* Try again, allowing for inconsistency. * XXX: As a side effect, logging may be duplicated. */ err = ofpacts_check(ofpacts.data, ofpacts.size, - &fm->match.flow, OFPP_MAX, 0, false); + &fm->match.flow, false, + OFPP_MAX, fm->table_id, 255); } if (err) { error = xasprintf("actions are invalid with specified match " diff --git a/lib/ofp-print.c b/lib/ofp-print.c index 1dc459888..37e1f4f88 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -757,7 +757,8 @@ ofp_print_flow_mod(struct ds *s, const struct ofp_header *oh, int verbosity) protocol = ofputil_protocol_set_tid(protocol, true); ofpbuf_init(&ofpacts, 64); - error = ofputil_decode_flow_mod(&fm, oh, protocol, &ofpacts); + error = ofputil_decode_flow_mod(&fm, oh, protocol, &ofpacts, + OFPP_MAX, 255); if (error) { ofpbuf_uninit(&ofpacts); ofp_print_error(s, error); diff --git a/lib/ofp-util.c b/lib/ofp-util.c index f0cd7516c..017268f7b 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -1484,7 +1484,8 @@ enum ofperr ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, const struct ofp_header *oh, enum ofputil_protocol protocol, - struct ofpbuf *ofpacts) + struct ofpbuf *ofpacts, + ofp_port_t max_port, uint8_t max_table) { ovs_be16 raw_flags; enum ofperr error; @@ -1663,7 +1664,9 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, : OFPERR_OFPFMFC_TABLE_FULL); } - return 0; + return ofpacts_check(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/lib/ofp-util.h b/lib/ofp-util.h index d72e0a882..f62aa0206 100644 --- a/lib/ofp-util.h +++ b/lib/ofp-util.h @@ -294,7 +294,9 @@ struct ofputil_flow_mod { enum ofperr ofputil_decode_flow_mod(struct ofputil_flow_mod *, const struct ofp_header *, enum ofputil_protocol, - struct ofpbuf *ofpacts); + struct ofpbuf *ofpacts, + ofp_port_t max_port, + uint8_t max_table); struct ofpbuf *ofputil_encode_flow_mod(const struct ofputil_flow_mod *, enum ofputil_protocol); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 052c8cd2e..4fb7e4fb7 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -271,9 +271,12 @@ static enum ofperr modify_flows__(struct ofproto *, struct ofconn *, static void delete_flow__(struct rule *rule, struct ofopgroup *, enum ofp_flow_removed_reason) OVS_REQUIRES(ofproto_mutex); +static bool ofproto_group_exists__(const struct ofproto *ofproto, + uint32_t group_id) + OVS_REQ_RDLOCK(ofproto->groups_rwlock); static bool ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id) - OVS_REQ_RDLOCK(ofproto->groups_rwlock); + OVS_EXCLUDED(ofproto->groups_rwlock); static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *); static bool handle_openflow(struct ofconn *, const struct ofpbuf *); static enum ofperr handle_flow_mod__(struct ofproto *, struct ofconn *, @@ -2829,46 +2832,33 @@ reject_slave_controller(struct ofconn *ofconn) } } -/* Checks that the 'ofpacts_len' bytes of actions in 'ofpacts' are appropriate - * for a packet with the prerequisites satisfied by 'flow' in table 'table_id'. - * 'flow' may be temporarily modified, but is restored at return. - */ +/* Checks that the 'ofpacts_len' bytes of action in 'ofpacts' are appropriate + * for 'ofproto': + * + * - If they use a meter, then 'ofproto' has that meter configured. + * + * - If they use any groups, then 'ofproto' has that group configured. + * + * Returns 0 if successful, otherwise an OpenFlow error. */ static enum ofperr ofproto_check_ofpacts(struct ofproto *ofproto, - struct ofpact ofpacts[], size_t ofpacts_len, - struct flow *flow, uint8_t table_id, - const struct ofp_header *oh) + const struct ofpact ofpacts[], size_t ofpacts_len) { - enum ofperr error; const struct ofpact *a; uint32_t mid; - error = ofpacts_check(ofpacts, ofpacts_len, flow, - u16_to_ofp(ofproto->max_ports), table_id, - oh && oh->version > OFP10_VERSION); - if (error) { - return error; + mid = ofpacts_get_meter(ofpacts, ofpacts_len); + if (mid && get_provider_meter_id(ofproto, mid) == UINT32_MAX) { + return OFPERR_OFPMMFC_INVALID_METER; } OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { - if (a->type == OFPACT_GROUP) { - bool exists; - - ovs_rwlock_rdlock(&ofproto->groups_rwlock); - exists = ofproto_group_exists(ofproto, - ofpact_get_GROUP(a)->group_id); - ovs_rwlock_unlock(&ofproto->groups_rwlock); - - if (!exists) { - return OFPERR_OFPBAC_BAD_OUT_GROUP; - } + if (a->type == OFPACT_GROUP + && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) { + return OFPERR_OFPBAC_BAD_OUT_GROUP; } } - mid = ofpacts_get_meter(ofpacts, ofpacts_len); - if (mid && get_provider_meter_id(ofproto, mid) == UINT32_MAX) { - return OFPERR_OFPMMFC_INVALID_METER; - } return 0; } @@ -2918,7 +2908,7 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh) /* Verify actions against packet, then send packet if successful. */ in_port_.ofp_port = po.in_port; flow_extract(payload, 0, 0, NULL, &in_port_, &flow); - error = ofproto_check_ofpacts(p, po.ofpacts, po.ofpacts_len, &flow, 0, oh); + error = ofproto_check_ofpacts(p, po.ofpacts, po.ofpacts_len); if (!error) { error = p->ofproto_class->packet_out(p, payload, &flow, po.ofpacts, po.ofpacts_len); @@ -3966,14 +3956,6 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn, } } - /* Verify actions. */ - error = ofproto_check_ofpacts(ofproto, fm->ofpacts, fm->ofpacts_len, - &fm->match.flow, table_id, request); - if (error) { - cls_rule_destroy(&cr); - return error; - } - /* Serialize against pending deletion. */ if (is_flow_deletion_pending(ofproto, &cr, table_id)) { cls_rule_destroy(&cr); @@ -4072,19 +4054,6 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, enum ofperr error; size_t i; - /* Verify actions before we start to modify any rules, to avoid partial - * flow table modifications. */ - for (i = 0; i < rules->n; i++) { - struct rule *rule = rules->rules[i]; - - error = ofproto_check_ofpacts(ofproto, fm->ofpacts, fm->ofpacts_len, - &fm->match.flow, rule->table_id, - request); - if (error) { - return error; - } - } - type = fm->command == OFPFC_ADD ? OFOPERATION_REPLACE : OFOPERATION_MODIFY; group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id); error = OFPERR_OFPBRC_EPERM; @@ -4417,7 +4386,12 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh) ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub); error = ofputil_decode_flow_mod(&fm, oh, ofconn_get_protocol(ofconn), - &ofpacts); + &ofpacts, + u16_to_ofp(ofproto->max_ports), + ofproto->n_tables); + if (!error) { + error = ofproto_check_ofpacts(ofproto, fm.ofpacts, fm.ofpacts_len); + } if (!error) { error = handle_flow_mod__(ofproto, ofconn, &fm, oh); } @@ -5321,7 +5295,7 @@ ofproto_group_write_lookup(const struct ofproto *ofproto, uint32_t group_id, } static bool -ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id) +ofproto_group_exists__(const struct ofproto *ofproto, uint32_t group_id) OVS_REQ_RDLOCK(ofproto->groups_rwlock) { struct ofgroup *grp; @@ -5335,6 +5309,19 @@ ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id) return false; } +static bool +ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id) + OVS_EXCLUDED(ofproto->groups_rwlock) +{ + bool exists; + + ovs_rwlock_rdlock(&ofproto->groups_rwlock); + exists = ofproto_group_exists__(ofproto, group_id); + ovs_rwlock_unlock(&ofproto->groups_rwlock); + + return exists; +} + static uint32_t group_get_ref_count(struct ofgroup *group) OVS_EXCLUDED(ofproto_mutex) @@ -5569,7 +5556,7 @@ add_group(struct ofproto *ofproto, struct ofputil_group_mod *gm) goto unlock_out; } - if (ofproto_group_exists(ofproto, gm->group_id)) { + if (ofproto_group_exists__(ofproto, gm->group_id)) { error = OFPERR_OFPGMFC_GROUP_EXISTS; goto unlock_out; } diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index a5c69c7bb..a0dc5c8ba 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -3077,8 +3077,9 @@ 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, OFPP_MAX, - table_id ? atoi(table_id) : 0, true); + error = ofpacts_check(ofpacts.data, ofpacts.size, &flow, + true, OFPP_MAX, + table_id ? atoi(table_id) : 0, 255); } if (error) { printf("bad OF1.1 instructions: %s\n\n", ofperr_get_name(error)); -- 2.47.0