[ovs-dev] [PATCH 1/3] Fix table checking for goto table instruction.
Jarno Rajahalme
jarno.rajahalme at nsn.com
Fri Jun 28 16:44:03 UTC 2013
Usually the table id in flow mods is 255, which means that goto table
instruction cannot be checked before the table is picked (for flow add),
or the rules to be modified are found (flow mod).
Move goto table checking from decode (ofp-util) to actions checking
(ofp-actions), and postpone the action checking until the table in
which the actions are added is known.
This fixes OFPBRC_BAD_TABLE_ID errors for flow adds that specify the table
id as 255, and have a goto table instruction.
Signed-off-by: Jarno Rajahalme <jarno.rajahalme at nsn.com>
---
lib/ofp-actions.c | 18 +++++++++---------
lib/ofp-actions.h | 4 ++--
lib/ofp-parse.c | 2 +-
lib/ofp-util.c | 6 ++----
ofproto/ofproto-provider.h | 2 +-
ofproto/ofproto.c | 42 +++++++++++++++++++++++++-----------------
utilities/ovs-ofctl.c | 12 +++++++++---
7 files changed, 49 insertions(+), 37 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 34a700b..8f80758 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1051,7 +1051,6 @@ ofpacts_pull_openflow11_actions(struct ofpbuf *openflow,
enum ofperr
ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
unsigned int instructions_len,
- uint8_t table_id,
struct ofpbuf *ofpacts)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
@@ -1129,10 +1128,6 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
oigt = instruction_get_OFPIT11_GOTO_TABLE(
insts[OVSINST_OFPIT11_GOTO_TABLE]);
- if (table_id >= oigt->table_id) {
- error = OFPERR_OFPBRC_BAD_TABLE_ID;
- goto exit;
- }
ogt = ofpact_put_GOTO_TABLE(ofpacts);
ogt->table_id = oigt->table_id;
}
@@ -1152,7 +1147,8 @@ exit:
/* 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)
+ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
+ uint8_t table_id)
{
const struct ofpact_enqueue *enqueue;
@@ -1238,9 +1234,13 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports)
case OFPACT_CLEAR_ACTIONS:
case OFPACT_WRITE_METADATA:
case OFPACT_METER:
- case OFPACT_GOTO_TABLE:
return 0;
+ case OFPACT_GOTO_TABLE:
+ if (ofpact_get_GOTO_TABLE(a)->table_id <= table_id) {
+ return OFPERR_OFPBRC_BAD_TABLE_ID;
+ }
+ return 0;
default:
NOT_REACHED();
}
@@ -1253,14 +1253,14 @@ 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)
+ struct flow *flow, ofp_port_t max_ports, uint8_t table_id)
{
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);
+ error = ofpact_check__(a, flow, max_ports, table_id);
if (error) {
break;
}
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 799f64c..aacd30c 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -499,10 +499,10 @@ enum ofperr ofpacts_pull_openflow11_actions(struct ofpbuf *openflow,
struct ofpbuf *ofpacts);
enum ofperr ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
unsigned int instructions_len,
- uint8_t table_id,
struct ofpbuf *ofpacts);
enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
- struct flow *, ofp_port_t max_ports);
+ struct flow *, ofp_port_t max_ports,
+ uint8_t table_id);
enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len);
/* Converting ofpacts to OpenFlow. */
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index da36f88..089df6d 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1016,7 +1016,7 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
fm->ofpacts = ofpbuf_steal_data(&ofpacts);
err = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
- OFPP_MAX);
+ OFPP_MAX, 0);
if (err) {
exit(EXIT_FAILURE);
}
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 6c58415..bba41b9 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1502,8 +1502,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
return error;
}
- error = ofpacts_pull_openflow11_instructions(&b, b.size, ofm->table_id,
- ofpacts);
+ error = ofpacts_pull_openflow11_instructions(&b, b.size, ofpacts);
if (error) {
return error;
}
@@ -2377,8 +2376,7 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
}
if (ofpacts_pull_openflow11_instructions(msg, length - sizeof *ofs -
- padded_match_len,
- ofs->table_id, ofpacts)) {
+ padded_match_len, ofpacts)) {
VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad instructions");
return EINVAL;
}
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 1655c3a..8b07f76 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1344,7 +1344,7 @@ int ofproto_class_unregister(const struct ofproto_class *);
enum { OFPROTO_POSTPONE = 1 << 16 };
BUILD_ASSERT_DECL(OFPROTO_POSTPONE < OFPERR_OFS);
-int ofproto_flow_mod(struct ofproto *, const struct ofputil_flow_mod *);
+int ofproto_flow_mod(struct ofproto *, struct ofputil_flow_mod *);
void ofproto_add_flow(struct ofproto *, const struct match *,
unsigned int priority,
const struct ofpact *ofpacts, size_t ofpacts_len);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 7a844ba..aa360a8 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -196,13 +196,13 @@ static bool rule_is_modifiable(const struct rule *);
/* OpenFlow. */
static enum ofperr add_flow(struct ofproto *, struct ofconn *,
- const struct ofputil_flow_mod *,
+ struct ofputil_flow_mod *,
const struct ofp_header *);
static void delete_flow__(struct rule *, struct ofopgroup *,
enum ofp_flow_removed_reason);
static bool handle_openflow(struct ofconn *, const struct ofpbuf *);
static enum ofperr handle_flow_mod__(struct ofproto *, struct ofconn *,
- const struct ofputil_flow_mod *,
+ struct ofputil_flow_mod *,
const struct ofp_header *);
static void calc_duration(long long int start, long long int now,
uint32_t *sec, uint32_t *nsec);
@@ -1618,7 +1618,7 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match,
*
* This is a helper function for in-band control and fail-open. */
int
-ofproto_flow_mod(struct ofproto *ofproto, const struct ofputil_flow_mod *fm)
+ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm)
{
return handle_flow_mod__(ofproto, NULL, fm, NULL);
}
@@ -2415,7 +2415,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 = ofpacts_check(po.ofpacts, po.ofpacts_len, &flow, p->max_ports);
+ error = ofpacts_check(po.ofpacts, po.ofpacts_len, &flow, p->max_ports, 0);
if (!error) {
error = p->ofproto_class->packet_out(p, payload, &flow,
po.ofpacts, po.ofpacts_len);
@@ -3225,7 +3225,7 @@ is_flow_deletion_pending(const struct ofproto *ofproto,
* if any. */
static enum ofperr
add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
- const struct ofputil_flow_mod *fm, const struct ofp_header *request)
+ struct ofputil_flow_mod *fm, const struct ofp_header *request)
{
struct oftable *table;
struct ofopgroup *group;
@@ -3264,6 +3264,13 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
return OFPERR_OFPBRC_EPERM;
}
+ /* Verify actions. */
+ error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
+ ofproto->max_ports, table_id);
+ if (error) {
+ return error;
+ }
+
/* Allocate new rule and initialize classifier rule. */
rule = ofproto->ofproto_class->rule_alloc();
if (!rule) {
@@ -3372,8 +3379,8 @@ exit:
* Returns 0 on success, otherwise an OpenFlow error code. */
static enum ofperr
modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
- const struct ofputil_flow_mod *fm,
- const struct ofp_header *request, struct list *rules)
+ struct ofputil_flow_mod *fm, const struct ofp_header *request,
+ struct list *rules)
{
struct ofopgroup *group;
struct rule *rule;
@@ -3394,6 +3401,13 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
continue;
}
+ /* Verify actions. */
+ error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
+ ofproto->max_ports, rule->table_id);
+ if (error) {
+ return error;
+ }
+
actions_changed = !ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
rule->ofpacts, rule->ofpacts_len);
@@ -3419,8 +3433,7 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
static enum ofperr
modify_flows_add(struct ofproto *ofproto, struct ofconn *ofconn,
- const struct ofputil_flow_mod *fm,
- const struct ofp_header *request)
+ struct ofputil_flow_mod *fm, const struct ofp_header *request)
{
if (fm->cookie_mask != htonll(0) || fm->new_cookie == htonll(UINT64_MAX)) {
return 0;
@@ -3435,7 +3448,7 @@ modify_flows_add(struct ofproto *ofproto, struct ofconn *ofconn,
* if any. */
static enum ofperr
modify_flows_loose(struct ofproto *ofproto, struct ofconn *ofconn,
- const struct ofputil_flow_mod *fm,
+ struct ofputil_flow_mod *fm,
const struct ofp_header *request)
{
struct list rules;
@@ -3460,7 +3473,7 @@ modify_flows_loose(struct ofproto *ofproto, struct ofconn *ofconn,
* if any. */
static enum ofperr
modify_flow_strict(struct ofproto *ofproto, struct ofconn *ofconn,
- const struct ofputil_flow_mod *fm,
+ struct ofputil_flow_mod *fm,
const struct ofp_header *request)
{
struct list rules;
@@ -3636,10 +3649,6 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
error = ofputil_decode_flow_mod(&fm, oh, ofconn_get_protocol(ofconn),
&ofpacts);
if (!error) {
- error = ofpacts_check(fm.ofpacts, fm.ofpacts_len,
- &fm.match.flow, ofproto->max_ports);
- }
- if (!error) {
error = handle_flow_mod__(ofproto, ofconn, &fm, oh);
}
if (error) {
@@ -3680,8 +3689,7 @@ exit:
static enum ofperr
handle_flow_mod__(struct ofproto *ofproto, struct ofconn *ofconn,
- const struct ofputil_flow_mod *fm,
- const struct ofp_header *oh)
+ struct ofputil_flow_mod *fm, const struct ofp_header *oh)
{
if (ofproto->n_pending >= 50) {
ovs_assert(!list_is_empty(&ofproto->pending));
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index b2470e6..2d6872b 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2637,9 +2637,15 @@ ofctl_parse_ofp11_instructions(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
/* Convert to ofpacts. */
ofpbuf_init(&ofpacts, 0);
size = of11_in.size;
- error = ofpacts_pull_openflow11_instructions(
- &of11_in, of11_in.size, table_id ? atoi(table_id) : 0,
- &ofpacts);
+ error = ofpacts_pull_openflow11_instructions(&of11_in, of11_in.size,
+ &ofpacts);
+ if (!error) {
+ /* Verify actions. */
+ struct flow flow;
+ memset(&flow, 0, sizeof flow);
+ error = ofpacts_check(ofpacts.data, ofpacts.size, &flow, OFPP_MAX,
+ table_id ? atoi(table_id) : 0);
+ }
if (error) {
printf("bad OF1.1 instructions: %s\n\n", ofperr_get_name(error));
ofpbuf_uninit(&ofpacts);
--
1.7.10.4
More information about the dev
mailing list