By passing a flow to the action parser the pre-requisites
of set-feild actions will be checked. If the flow is NULL,
for instance in test code such as ofctl_parse_ofp11_instructions(),
then the check is skiped, as it always was before this change.
Unfortunately I don't think that this check is correct as
it does not take into account other actions actions that may
be applied before the load action, e.g. vlan push/pop, which may
affect the pre-requisites.
I believe that in its current form there is scope for both false positives
(not so bad, perhaps) and false negatives (pretty bad). I would welcome
some input on if these concerns are valid and if so how they may be
overcome.
Signed-off-by: Simon Horman <ho...@verge.net.au>
---
v5
* Initial post
---
lib/nx-match.c | 5 +++--
lib/nx-match.h | 3 ++-
lib/ofp-actions.c | 31 ++++++++++++++++++-------------
lib/ofp-actions.h | 3 ++-
lib/ofp-util.c | 6 ++++--
utilities/ovs-ofctl.c | 2 +-
6 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/lib/nx-match.c b/lib/nx-match.c
index b1eb9e2..c6fee74 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1107,7 +1107,8 @@ nxm_reg_load_from_openflow(const struct
nx_action_reg_load *narl,
enum ofperr
nxm_reg_load_from_openflow12_set_field(
- const struct ofp12_action_set_field * oasf, struct ofpbuf *ofpacts)
+ const struct ofp12_action_set_field *oasf, struct ofpbuf *ofpacts,
+ struct flow *flow)
{
uint16_t oasf_len = ntohs(oasf->len);
ovs_be32 *p = (ovs_be32*)oasf->field;
@@ -1143,7 +1144,7 @@ nxm_reg_load_from_openflow12_set_field(
&load->subvalue, sizeof load->subvalue, load->dst.ofs,
mf->n_bits);
- return nxm_reg_load_check(load, NULL);
+ return nxm_reg_load_check(load, flow);
}
enum ofperr
diff --git a/lib/nx-match.h b/lib/nx-match.h
index 6a57297..721bdeb 100644
--- a/lib/nx-match.h
+++ b/lib/nx-match.h
@@ -66,7 +66,8 @@ enum ofperr nxm_reg_move_from_openflow(const struct
nx_action_reg_move *,
enum ofperr nxm_reg_load_from_openflow(const struct nx_action_reg_load *,
struct ofpbuf *ofpacts);
enum ofperr nxm_reg_load_from_openflow12_set_field(
- const struct ofp12_action_set_field * oasf, struct ofpbuf *ofpacts);
+ const struct ofp12_action_set_field * oasf, struct ofpbuf *ofpacts,
+ struct flow *flow);
enum ofperr nxm_reg_move_check(const struct ofpact_reg_move *,
const struct flow *);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index df94974..6f748ba 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -385,7 +385,8 @@ ofpact_from_nxast(const union ofp_action *a, enum
ofputil_action_code code,
}
static enum ofperr
-ofpact_from_openflow10(const union ofp_action *a, struct ofpbuf *out)
+ofpact_from_openflow10(const union ofp_action *a, struct ofpbuf *out,
+ struct flow *flow OVS_UNUSED)
{
enum ofputil_action_code code;
enum ofperr error;
@@ -510,15 +511,16 @@ log_bad_action(const union ofp_action *actions, size_t
n_actions, size_t ofs,
static enum ofperr
ofpacts_from_openflow(const union ofp_action *in, size_t n_in,
- struct ofpbuf *out,
+ struct ofpbuf *out, struct flow *flow,
enum ofperr (*ofpact_from_openflow)(
- const union ofp_action *a, struct ofpbuf *out))
+ const union ofp_action *a, struct ofpbuf *out,
+ struct flow *flow))
{
const union ofp_action *a;
size_t left;
ACTION_FOR_EACH (a, left, in, n_in) {
- enum ofperr error = ofpact_from_openflow(a, out);
+ enum ofperr error = ofpact_from_openflow(a, out, flow);
if (error) {
log_bad_action(in, n_in, a - in, error);
return error;
@@ -538,7 +540,7 @@ static enum ofperr
ofpacts_from_openflow10(const union ofp_action *in, size_t n_in,
struct ofpbuf *out)
{
- return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow10);
+ return ofpacts_from_openflow(in, n_in, out, NULL, ofpact_from_openflow10);
}
static enum ofperr
@@ -648,7 +650,8 @@ output_from_openflow11(const struct ofp11_action_output
*oao,
}
static enum ofperr
-ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out)
+ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out,
+ struct flow *flow OVS_UNUSED)
{
enum ofputil_action_code code;
enum ofperr error;
@@ -728,7 +731,7 @@ static enum ofperr
ofpacts_from_openflow11(const union ofp_action *in, size_t n_in,
struct ofpbuf *out)
{
- return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow11);
+ return ofpacts_from_openflow(in, n_in, out, NULL, ofpact_from_openflow11);
}
static enum ofperr
@@ -768,7 +771,8 @@ decode_openflow12_action(const union ofp_action *a,
}
static enum ofperr
-ofpact_from_openflow12(const union ofp_action *a, struct ofpbuf *out)
+ofpact_from_openflow12(const union ofp_action *a, struct ofpbuf *out,
+ struct flow *flow)
{
/* XXX */
enum ofputil_action_code code;
@@ -792,7 +796,7 @@ ofpact_from_openflow12(const union ofp_action *a, struct
ofpbuf *out)
case OFPUTIL_OFPAT12_SET_FIELD:
return nxm_reg_load_from_openflow12_set_field(
- (const struct ofp12_action_set_field *)a, out);
+ (const struct ofp12_action_set_field *)a, out, flow);
#define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) case OFPUTIL_##ENUM:
#include "ofp-util.def"
@@ -804,9 +808,9 @@ ofpact_from_openflow12(const union ofp_action *a, struct
ofpbuf *out)
static enum ofperr
ofpacts_from_openflow12(const union ofp_action *in, size_t n_in,
- struct ofpbuf *out)
+ struct ofpbuf *out, struct flow *flow)
{
- return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow12);
+ return ofpacts_from_openflow(in, n_in, out, flow, ofpact_from_openflow12);
}
/* OpenFlow 1.1 instructions. */
@@ -987,7 +991,8 @@ enum ofperr
ofpacts_pull_openflow11_instructions(enum ofp_version ofp_version,
struct ofpbuf *openflow,
unsigned int instructions_len,
- struct ofpbuf *ofpacts)
+ struct ofpbuf *ofpacts,
+ struct flow *flow)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
const struct ofp11_instruction *instructions;
@@ -1027,7 +1032,7 @@ ofpacts_pull_openflow11_instructions(enum ofp_version
ofp_version,
get_actions_from_instruction(insts[OVSINST_OFPIT11_APPLY_ACTIONS],
&actions, &n_actions);
if (ofp_version == OFP12_VERSION) {
- error = ofpacts_from_openflow12(actions, n_actions, ofpacts);
+ error = ofpacts_from_openflow12(actions, n_actions, ofpacts, flow);
} else if (ofp_version == OFP11_VERSION){
error = ofpacts_from_openflow11(actions, n_actions, ofpacts);
} else {
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 540c91d..4d6ec42 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -402,7 +402,8 @@ enum ofperr ofpacts_pull_openflow11_actions(struct ofpbuf
*openflow,
enum ofperr ofpacts_pull_openflow11_instructions(enum ofp_version ofp_version,
struct ofpbuf *openflow,
unsigned int instructions_len,
- struct ofpbuf *ofpacts);
+ struct ofpbuf *ofpacts,
+ struct flow *flow);
enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
const struct flow *, int max_ports);
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index ac3a401..273eacb 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1145,7 +1145,8 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
}
error = ofpacts_pull_openflow11_instructions(oh->version,
- &b, b.size, ofpacts);
+ &b, b.size, ofpacts,
+ &fm->match.flow);
if (error) {
return error;
}
@@ -1636,7 +1637,8 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats
*fs,
if (ofpacts_pull_openflow11_instructions(oh->version, msg,
length - sizeof *ofs -
- padded_match_len, ofpacts)) {
+ padded_match_len, ofpacts,
+ &fs->match.flow)) {
VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad instructions");
return EINVAL;
}
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 125523d..c7beb1a 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2553,7 +2553,7 @@ ofctl_parse_ofp11_instructions(int argc OVS_UNUSED, char
*argv[] OVS_UNUSED)
size = of11_in.size;
error = ofpacts_pull_openflow11_instructions(OFP11_VERSION,
&of11_in, of11_in.size,
- &ofpacts);
+ &ofpacts, NULL);
if (error) {
printf("bad OF1.1 instructions: %s\n\n", ofperr_get_name(error));
ofpbuf_uninit(&ofpacts);
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
