Skip to content

Commit

Permalink
lib/ofp-actions: Enforce action consistency.
Browse files Browse the repository at this point in the history 
OpenFlow 1.1+ specs encourage switches to verify action consistency
at flow setup time.  Implement this for OpenFlow 1.1+ only to not
break any current OF 1.0 based use.

Signed-off-by: Jarno Rajahalme <[email protected]>
Signed-off-by: Ben Pfaff <[email protected]>
  • Loading branch information
@blp
Jarno Rajahalme authored and blp committed Oct 23, 2013
1 parent dba70df commit 8621547
Show file tree
Hide file tree
Showing 10 changed files with 138 additions and 41 deletions.
63 changes: 57 additions & 6 deletions lib/ofp-actions.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1541,7 +1541,7 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
/* May modify flow->dl_type, caller must restore it. */
static enum ofperr
ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
uint8_t table_id)
uint8_t table_id, bool enforce_consistency)
{
const struct ofpact_enqueue *enqueue;

Expand Down Expand Up @@ -1570,17 +1570,48 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,

case OFPACT_SET_VLAN_VID:
case OFPACT_SET_VLAN_PCP:
return 0;

case OFPACT_STRIP_VLAN:
if (!(flow->vlan_tci & htons(VLAN_CFI))) {
goto inconsistent;
}
return 0;

case OFPACT_PUSH_VLAN:
if (flow->vlan_tci & htons(VLAN_CFI)) {
/* Multiple VLAN headers not supported. */
return OFPERR_OFPBAC_BAD_TAG;
}
return 0;

case OFPACT_SET_ETH_SRC:
case OFPACT_SET_ETH_DST:
return 0;

case OFPACT_SET_IPV4_SRC:
case OFPACT_SET_IPV4_DST:
if (flow->dl_type != htons(ETH_TYPE_IP)) {
goto inconsistent;
}
return 0;

case OFPACT_SET_IP_DSCP:
case OFPACT_SET_IP_ECN:
case OFPACT_SET_IP_TTL:
case OFPACT_DEC_TTL:
if (!is_ip_any(flow)) {
goto inconsistent;
}
return 0;

case OFPACT_SET_L4_SRC_PORT:
case OFPACT_SET_L4_DST_PORT:
if (!is_ip_any(flow) ||
(flow->nw_proto != IPPROTO_TCP && flow->nw_proto != IPPROTO_UDP
&& flow->nw_proto != IPPROTO_SCTP)) {
goto inconsistent;
}
return 0;

case OFPACT_REG_MOVE:
Expand All @@ -1595,16 +1626,25 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
case OFPACT_STACK_POP:
return nxm_stack_pop_check(ofpact_get_STACK_POP(a), flow);

case OFPACT_DEC_TTL:
case OFPACT_SET_MPLS_TTL:
case OFPACT_DEC_MPLS_TTL:
if (!eth_type_mpls(flow->dl_type)) {
goto inconsistent;
}
return 0;

case OFPACT_SET_TUNNEL:
case OFPACT_SET_QUEUE:
case OFPACT_POP_QUEUE:
case OFPACT_FIN_TIMEOUT:
case OFPACT_RESUBMIT:
return 0;

case OFPACT_FIN_TIMEOUT:
if (flow->nw_proto != IPPROTO_TCP) {
goto inconsistent;
}
return 0;

case OFPACT_LEARN:
return learn_check(ofpact_get_LEARN(a), flow);

Expand All @@ -1621,6 +1661,9 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,

case OFPACT_POP_MPLS:
flow->dl_type = ofpact_get_POP_MPLS(a)->ethertype;
if (!eth_type_mpls(flow->dl_type)) {
goto inconsistent;
}
return 0;

case OFPACT_SAMPLE:
Expand All @@ -1632,7 +1675,7 @@ ofpact_check__(const 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);
flow, max_ports, table_id, false);
}

case OFPACT_WRITE_METADATA:
Expand All @@ -1658,6 +1701,12 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
default:
NOT_REACHED();
}

inconsistent:
if (enforce_consistency) {
return OFPERR_OFPBAC_MATCH_INCONSISTENT;
}
return 0;
}

/* Checks that the 'ofpacts_len' bytes of actions in 'ofpacts' are
Expand All @@ -1667,14 +1716,16 @@ ofpact_check__(const 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(const struct ofpact ofpacts[], size_t ofpacts_len,
struct flow *flow, ofp_port_t max_ports, uint8_t table_id)
struct flow *flow, ofp_port_t max_ports, uint8_t table_id,
bool enforce_consistency)
{
const struct ofpact *a;
ovs_be16 dl_type = flow->dl_type;
enum ofperr error = 0;

OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
error = ofpact_check__(a, flow, max_ports, table_id);
error = ofpact_check__(a, flow, max_ports, table_id,
enforce_consistency);
if (error) {
break;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/ofp-actions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,7 @@ enum ofperr ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
struct ofpbuf *ofpacts);
enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
struct flow *, ofp_port_t max_ports,
uint8_t table_id);
uint8_t table_id, bool enforce_consistency);
enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len);

/* Converting ofpacts to OpenFlow. */
Expand Down
43 changes: 31 additions & 12 deletions lib/ofp-parse.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1152,7 +1152,8 @@ parse_field(const struct mf_field *mf, const char *s, struct match *match,

static char * WARN_UNUSED_RESULT
parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
enum ofputil_protocol *usable_protocols)
enum ofputil_protocol *usable_protocols,
bool enforce_consistency)
{
enum {
F_OUT_PORT = 1 << 0,
Expand Down Expand Up @@ -1360,10 +1361,21 @@ 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);
OFPP_MAX, 0, true);
if (err) {
error = xasprintf("actions are invalid with specified match "
"(%s)", ofperr_to_string(err));
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, OFPP_MAX, 0, false);
}
if (err) {
error = xasprintf("actions are invalid with specified match "
"(%s)", ofperr_to_string(err));
}
}
}
if (error) {
Expand Down Expand Up @@ -1393,12 +1405,14 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
* error. The caller is responsible for freeing the returned string. */
char * WARN_UNUSED_RESULT
parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
enum ofputil_protocol *usable_protocols)
enum ofputil_protocol *usable_protocols,
bool enforce_consistency)
{
char *string = xstrdup(str_);
char *error;

error = parse_ofp_str__(fm, command, string, usable_protocols);
error = parse_ofp_str__(fm, command, string, usable_protocols,
enforce_consistency);
if (error) {
fm->ofpacts = NULL;
fm->ofpacts_len = 0;
Expand Down Expand Up @@ -1745,9 +1759,11 @@ parse_ofpacts(const char *s_, struct ofpbuf *ofpacts,
char * WARN_UNUSED_RESULT
parse_ofp_flow_mod_str(struct ofputil_flow_mod *fm, const char *string,
uint16_t command,
enum ofputil_protocol *usable_protocols)
enum ofputil_protocol *usable_protocols,
bool enforce_consistency)
{
char *error = parse_ofp_str(fm, command, string, usable_protocols);
char *error = parse_ofp_str(fm, command, string, usable_protocols,
enforce_consistency);
if (!error) {
/* Normalize a copy of the match. This ensures that non-normalized
* flows get logged but doesn't affect what gets sent to the switch, so
Expand Down Expand Up @@ -1809,7 +1825,8 @@ parse_ofp_table_mod(struct ofputil_table_mod *tm, const char *table_id,
char * WARN_UNUSED_RESULT
parse_ofp_flow_mod_file(const char *file_name, uint16_t command,
struct ofputil_flow_mod **fms, size_t *n_fms,
enum ofputil_protocol *usable_protocols)
enum ofputil_protocol *usable_protocols,
bool enforce_consistency)
{
size_t allocated_fms;
int line_number;
Expand Down Expand Up @@ -1838,7 +1855,7 @@ parse_ofp_flow_mod_file(const char *file_name, uint16_t command,
*fms = x2nrealloc(*fms, &allocated_fms, sizeof **fms);
}
error = parse_ofp_flow_mod_str(&(*fms)[*n_fms], ds_cstr(&s), command,
&usable);
&usable, enforce_consistency);
if (error) {
size_t i;

Expand Down Expand Up @@ -1870,12 +1887,14 @@ parse_ofp_flow_mod_file(const char *file_name, uint16_t command,
char * WARN_UNUSED_RESULT
parse_ofp_flow_stats_request_str(struct ofputil_flow_stats_request *fsr,
bool aggregate, const char *string,
enum ofputil_protocol *usable_protocols)
enum ofputil_protocol *usable_protocols,
bool enforce_consistency)
{
struct ofputil_flow_mod fm;
char *error;

error = parse_ofp_str(&fm, -1, string, usable_protocols);
error = parse_ofp_str(&fm, -1, string, usable_protocols,
enforce_consistency);
if (error) {
return error;
}
Expand Down
12 changes: 8 additions & 4 deletions lib/ofp-parse.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,14 @@ struct simap;
enum ofputil_protocol;

char *parse_ofp_str(struct ofputil_flow_mod *, int command, const char *str_,
enum ofputil_protocol *usable_protocols)
enum ofputil_protocol *usable_protocols,
bool enforce_consistency)
WARN_UNUSED_RESULT;

char *parse_ofp_flow_mod_str(struct ofputil_flow_mod *, const char *string,
uint16_t command,
enum ofputil_protocol *usable_protocols)
enum ofputil_protocol *usable_protocols,
bool enforce_consistency)
WARN_UNUSED_RESULT;

char *parse_ofp_table_mod(struct ofputil_table_mod *,
Expand All @@ -51,12 +53,14 @@ char *parse_ofp_table_mod(struct ofputil_table_mod *,

char *parse_ofp_flow_mod_file(const char *file_name, uint16_t command,
struct ofputil_flow_mod **fms, size_t *n_fms,
enum ofputil_protocol *usable_protocols)
enum ofputil_protocol *usable_protocols,
bool enforce_consistency)
WARN_UNUSED_RESULT;

char *parse_ofp_flow_stats_request_str(struct ofputil_flow_stats_request *,
bool aggregate, const char *string,
enum ofputil_protocol *usable_protocols)
enum ofputil_protocol *usable_protocols,
bool enforce_consistency)
WARN_UNUSED_RESULT;

char *parse_ofpacts(const char *, struct ofpbuf *ofpacts,
Expand Down
1 change: 1 addition & 0 deletions lib/ofp-util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ enum ofputil_protocol {
OFPUTIL_P_OF10_NXM_TID = 1 << 3,
#define OFPUTIL_P_OF10_STD_ANY (OFPUTIL_P_OF10_STD | OFPUTIL_P_OF10_STD_TID)
#define OFPUTIL_P_OF10_NXM_ANY (OFPUTIL_P_OF10_NXM | OFPUTIL_P_OF10_NXM_TID)
#define OFPUTIL_P_OF10_ANY (OFPUTIL_P_OF10_STD_ANY | OFPUTIL_P_OF10_NXM_ANY)

/* OpenFlow 1.1 protocol.
*
Expand Down
17 changes: 11 additions & 6 deletions ofproto/ofproto.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2823,13 +2823,15 @@ reject_slave_controller(struct ofconn *ofconn)
static enum ofperr
ofproto_check_ofpacts(struct ofproto *ofproto,
const struct ofpact ofpacts[], size_t ofpacts_len,
struct flow *flow, uint8_t table_id)
struct flow *flow, uint8_t table_id,
bool enforce_consistency)
{
enum ofperr error;
uint32_t mid;

error = ofpacts_check(ofpacts, ofpacts_len, flow,
u16_to_ofp(ofproto->max_ports), table_id);
u16_to_ofp(ofproto->max_ports), table_id,
enforce_consistency);
if (error) {
return error;
}
Expand Down Expand Up @@ -2887,7 +2889,8 @@ 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);
error = ofproto_check_ofpacts(p, po.ofpacts, po.ofpacts_len, &flow, 0,
oh->version > OFP10_VERSION);
if (!error) {
error = p->ofproto_class->packet_out(p, payload, &flow,
po.ofpacts, po.ofpacts_len);
Expand Down Expand Up @@ -3937,7 +3940,8 @@ 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);
&fm->match.flow, table_id,
request && request->version > OFP10_VERSION);
if (error) {
cls_rule_destroy(&cr);
return error;
Expand Down Expand Up @@ -4059,9 +4063,10 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
continue;
}

/* Verify actions. */
/* Verify actions, enforce consistency check on OF1.1+. */
error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
u16_to_ofp(ofproto->max_ports), rule->table_id);
u16_to_ofp(ofproto->max_ports), rule->table_id,
request && request->version > OFP10_VERSION);
if (error) {
return error;
}
Expand Down
2 changes: 2 additions & 0 deletions tests/learn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,14 @@ AT_CHECK([[ovs-ofctl parse-flow 'actions=learn(load:5->NXM_OF_IP_DST[])']],
[1], [], [stderr])
AT_CHECK([sed -e 's/.*|meta_flow|WARN|//' < stderr], [0],
[[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[])']],
[1], [], [stderr])
AT_CHECK([sed -e 's/.*|meta_flow|WARN|//' < stderr], [0],
[[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
Expand Down
10 changes: 8 additions & 2 deletions tests/ovs-ofctl.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,12 @@ OFPT_FLOW_MOD (OF1.2): ADD table:255 actions=sample(probability=12345,collector_
]])
AT_CLEANUP

AT_SETUP([ovs-ofctl action inconsistency (OpenFlow 1.1)])
AT_CHECK([ovs-ofctl --protocols OpenFlow11 add-flow br0 'ip actions=mod_tp_dst:1234'
], [1], [stdout], [ovs-ofctl: actions are invalid with specified match (OFPBAC_MATCH_INCONSISTENT)
])
AT_CLEANUP

AT_SETUP([ovs-ofctl parse-flows (With Tunnel-Parameters)])
AT_DATA([flows.txt], [[
tun_id=0x1234000056780000/0xffff0000ffff0000,tun_src=1.1.1.1,tun_dst=2.2.2.2,tun_tos=0x3,tun_ttl=20,tun_flags=key|csum actions=drop
Expand Down Expand Up @@ -266,7 +272,7 @@ actions=output:1,bundle_load(eth_src,0,hrw,ofport,NXM_NX_REG0[16..31],slaves:1),
actions=resubmit:1,resubmit(2),resubmit(,3),resubmit(2,3)
send_flow_rem,actions=output:1,output:NXM_NX_REG0[],output:2,output:NXM_NX_REG1[16..31],output:3
check_overlap,actions=output:1,exit,output:2
actions=fin_timeout(idle_timeout=5,hard_timeout=15)
tcp,actions=fin_timeout(idle_timeout=5,hard_timeout=15)
actions=controller(max_len=123,reason=invalid_ttl,id=555)
actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678)
]])
Expand Down Expand Up @@ -302,7 +308,7 @@ NXT_FLOW_MOD: ADD table:255 actions=output:1,bundle_load(eth_src,0,hrw,ofport,NX
NXT_FLOW_MOD: ADD table:255 actions=resubmit:1,resubmit:2,resubmit(,3),resubmit(2,3)
NXT_FLOW_MOD: ADD table:255 send_flow_rem actions=output:1,output:NXM_NX_REG0[],output:2,output:NXM_NX_REG1[16..31],output:3
NXT_FLOW_MOD: ADD table:255 check_overlap actions=output:1,exit,output:2
NXT_FLOW_MOD: ADD table:255 actions=fin_timeout(idle_timeout=5,hard_timeout=15)
NXT_FLOW_MOD: ADD table:255 tcp actions=fin_timeout(idle_timeout=5,hard_timeout=15)
NXT_FLOW_MOD: ADD table:255 actions=controller(reason=invalid_ttl,max_len=123,id=555)
NXT_FLOW_MOD: ADD table:255 actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678)
]])
Expand Down
2 changes: 1 addition & 1 deletion utilities/ovs-controller.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ parse_options(int argc, char *argv[])
case OPT_WITH_FLOWS:
error = parse_ofp_flow_mod_file(optarg, OFPFC_ADD, &default_flows,
&n_default_flows,
&usable_protocols);
&usable_protocols, false);
if (error) {
ovs_fatal(0, "%s", error);
}
Expand Down
Loading

0 comments on commit 8621547

Please sign in to comment.