| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1384501858-30535-3-git-send-email-horms@verge.net.au>
Date: Fri, 15 Nov 2013 16:50:54 +0900
From: Simon Horman <horms@...ge.net.au>
To: dev@...nvswitch.org, netdev@...r.kernel.org,
Jesse Gross <jesse@...ira.com>, Ben Pfaff <blp@...ira.com>
Cc: Pravin B Shelar <pshelar@...ira.com>, Ravi K <rkerur@...il.com>,
Joe Stringer <joe@...d.net.nz>
Subject: [PATCH v2.49 2/6] ofp-actions: Account for OF version when enforcing action consistency
The aim of this patch is to provide infrastructure to differentiate
between OpenFlow1.1 - 1.2 and OpenFlow1.3+ consistency checking. It
also provides enhanced OpenFlow1.0 consistency checking.
Some different versions of OpenFlow require different consistency
checking.
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 vSwitch 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.
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. This is pre-OpenFlow1.3 tag ordering.
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. This is OpenFlow1.3+ tag ordering. Its implication
is that after an MPLS push action a packet is no longer a VLAN
packet as there are no longer any VLAN tags immediately after
the ethernet header: there is now an MPLS LSE there.
Currently Open vSwitch does not implement OpenFlow1.3+ tag ordering
so this patch uses the same consistency checking for OpenFlow1.3+
as for OpenFlow1.1 - 1.2.
A subsequent patch, which implements OpenFlow1.3+ tag ordering,
makes use of the infrastructure provided by this patch to check
actions accordingly.
Unfortunately ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed and similar
devices can't be used in order to implement logic as they are not set when
when ofpact_check__() is called via parse_ofp_str().
Instead this patch takes the approach of adding an OpenFlow version
parameter to ofpact_check__().
A new function ofpact_check_usable_protocols() allows trimming the
usable_protocols based on action checking.
The net effect of this change on run-time is only 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__()
Signed-off-by: Simon Horman <horms@...ge.net.au>
---
v2.49
* Temporarily set the CFI bit of flow->vlan_tci in ofpact_check__()
on mpls_pop as in this case a VLAN is now present. The value
of the other bits of the VLAN are unimportant in this context
* Add ofpact_check_usable_protocols using logic that appeared in
parse_ofp_str__() in v1 of this patch. This is to allow it to
be re-used in ofproto_unixctl_trace_actions().
* Move OpenFlow1.3+ logic into a subsequent patch.
* Enhance changelog
v1
* First post posted separately to the MPLS patchset that
it is now included in.
---
lib/ofp-actions.c | 61 ++++++++++++++++++++++++++++++++++++++++++--------
lib/ofp-actions.h | 11 ++++++++-
lib/ofp-parse.c | 24 +++++++-------------
lib/ofp-util.c | 6 ++---
ofproto/ofproto-dpif.c | 8 ++++---
tests/learn.at | 2 ++
utilities/ovs-ofctl.c | 4 ++--
7 files changed, 82 insertions(+), 34 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index e4e05dd..64333b3 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1875,9 +1875,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;
@@ -1911,7 +1911,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. */
@@ -1924,7 +1924,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. */
@@ -2078,8 +2078,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:
@@ -2124,7 +2125,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)
@@ -2136,7 +2138,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;
@@ -2149,6 +2151,47 @@ ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len,
return error;
}
+enum ofperr
+ofpacts_check_usable_protocols(enum ofputil_protocol *usable_protocols,
+ 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)
+{
+ enum ofperr 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, ofpacts_len, flow,
+ true, max_ports, table_id, n_tables);
+ if (!enforce_consistency &&
+ err == OFPERR_OFPBAC_MATCH_INCONSISTENT) {
+ /* Try again, allowing for inconsistency. */
+ err = ofpacts_check(OFP10_VERSION, ofpacts, ofpacts_len, flow,
+ false, max_ports, table_id, n_tables);
+ }
+ if (err) {
+ *usable_protocols &= ~OFPUTIL_P_OF10_ANY;
+ last_err = err;
+ }
+ }
+ if (*usable_protocols & (OFPUTIL_P_OF11_UP)) {
+ err = ofpacts_check(OFP11_VERSION, ofpacts, ofpacts_len, flow,
+ true, max_ports, table_id, n_tables);
+ if (err) {
+ *usable_protocols &= ~(OFPUTIL_P_OF11_UP);
+ last_err = err;
+ }
+ }
+ if (!*usable_protocols && last_err) {
+ return last_err;
+ }
+
+ return 0;
+}
+
/* Verifies that the 'ofpacts_len' bytes of actions in 'ofpacts' are
* in the appropriate order as defined by the OpenFlow spec. */
enum ofperr
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 70ad4b6..56c84d0 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -606,10 +606,19 @@ 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);
+enum ofperr ofpacts_check_usable_protocols(enum ofputil_protocol *usable_protocols,
+ 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);
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 f2debb3..85e7228 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1414,24 +1414,16 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
if (!error) {
enum ofperr err;
- err = ofpacts_check(ofpacts.data, ofpacts.size, &fm->match.flow,
- true, OFPP_MAX, fm->table_id, 255);
+ err = ofpacts_check_usable_protocols(usable_protocols,
+ ofpacts.data, ofpacts.size,
+ &fm->match.flow,
+ enforce_consistency, OFPP_MAX,
+ fm->table_id, 255);
if (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, false,
- OFPP_MAX, fm->table_id, 255);
- }
- if (err) {
- error = xasprintf("actions are invalid with specified match "
- "(%s)", ofperr_to_string(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 ede37b0..6678318 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/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 3391594..cef6e38 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5490,9 +5490,11 @@ ofproto_unixctl_trace_actions(struct unixctl_conn *conn, int argc,
unixctl_command_reply_error(conn, "invalid in_port");
goto exit;
}
- retval = ofpacts_check(ofpacts.data, ofpacts.size, &flow,
- enforce_consistency,
- u16_to_ofp(ofproto->up.max_ports), 0, 0);
+ retval = ofpacts_check_usable_protocols(&usable_protocols, ofpacts.data,
+ ofpacts.size, &flow,
+ enforce_consistency,
+ u16_to_ofp(ofproto->up.max_ports),
+ 0, 0);
if (retval) {
ds_clear(&result);
ds_put_format(&result, "Bad actions: %s", ofperr_to_string(retval));
diff --git a/tests/learn.at b/tests/learn.at
index 491cd5e..19e4449 100644
--- a/tests/learn.at
+++ b/tests/learn.at
@@ -76,6 +76,7 @@ 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
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 +84,7 @@ 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
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists