[ovs-dev] [PATCH] ofp-actions: make sure table_id in goto_table instruction is larger than the current table_id
Ben Pfaff
blp at nicira.com
Wed Jun 5 20:38:31 UTC 2013
On Wed, Jun 05, 2013 at 01:18:09PM -0700, Jing Ai wrote:
> Found a bug that OVS allows goto_table_id to be smaller than (or equal to) the current table id where the flow resides. It potentially creates an infinite loop when composing actions for a packet. To fix it, we just let OVS returns an error message to prevent such flow to be programmed.
>
> Signed-off-by: Jing Ai <jinga at google.com>
GCC reported:
../utilities/ovs-ofctl.c: In function ‘ofctl_parse_ofp11_instructions’:
../utilities/ovs-ofctl.c:2618: error: passing argument 1 of ‘__strsep_1c’ from incompatible pointer type
/usr/include/bits/string2.h:1224: note: expected ‘char **’ but argument is of type ‘const char **’
I changed 'instructions' from const char * to char * to fix the problem.
I removed the cast here. It is unnecessary:
+ error = ofpacts_pull_openflow11_instructions(
+ &of11_in, of11_in.size, table_id ? (uint8_t)atoi(table_id) : 0,
+ &ofpacts);
I changed AUTHORS so that your name appears only in the "committers"
list. We normally list each person only in one place or the other.
I reverted one whitespace-only change in ofp-util.c. It is correct to
remove trailing whitespace but please do not mix it with other
changes.
I reflowed the commit message to fit in 75 columns.
And I applied it to master as follows:
--8<--------------------------cut here-------------------------->8--
From: Jing Ai <jinga at google.com>
Date: Wed, 5 Jun 2013 13:18:09 -0700
Subject: [PATCH] ofp-actions: enforce valid range for table_id in goto_table instruction
Found a bug that OVS allows goto_table_id to be smaller than (or equal to)
the current table id where the flow resides. It potentially creates an
infinite loop when composing actions for a packet. To fix it, we just let
OVS returns an error message to prevent such flow to be programmed.
Signed-off-by: Jing Ai <jinga at google.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
AUTHORS | 2 +-
lib/ofp-actions.c | 5 +++++
lib/ofp-actions.h | 1 +
lib/ofp-util.c | 6 ++++--
tests/ofp-actions.at | 4 ++++
utilities/ovs-ofctl.c | 17 ++++++++++++++---
6 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index c887599..91d4dfd 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -46,6 +46,7 @@ Jarno Rajahalme jarno.rajahalme at nsn.com
Jean Tourrilhes jt at hpl.hp.com
Jeremy Stribling strib at nicira.com
Jesse Gross jesse at nicira.com
+Jing Ai jinga at google.com
Joe Perches joe at perches.com
Joe Stringer joe at wand.net.nz
Jun Nakajima jun.nakajima at intel.com
@@ -154,7 +155,6 @@ Jari Sundell sundell.software at gmail.com
Jed Daniels openvswitch at jeddaniels.com
Jeff Merrick jmerrick at vmware.com
Jeongkeun Lee jklee at hp.com
-Jing Ai ai_jing2000 at hotmail.com
Joan Cirer joan at ev0.net
John Galgay john at galgay.net
Kevin Mancuso kevin.mancuso at rackspace.com
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index c98e29a..58d0098 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1052,6 +1052,7 @@ 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);
@@ -1119,6 +1120,10 @@ 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;
}
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index ffceb05..9a74bcc 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -490,6 +490,7 @@ 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,
const struct flow *, int max_ports);
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 3c8d5a2..cc2dc36 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1502,7 +1502,8 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
return error;
}
- error = ofpacts_pull_openflow11_instructions(&b, b.size, ofpacts);
+ error = ofpacts_pull_openflow11_instructions(&b, b.size, ofm->table_id,
+ ofpacts);
if (error) {
return error;
}
@@ -2014,7 +2015,8 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
}
if (ofpacts_pull_openflow11_instructions(msg, length - sizeof *ofs -
- padded_match_len, ofpacts)) {
+ padded_match_len,
+ ofs->table_id, ofpacts)) {
VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad instructions");
return EINVAL;
}
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 2ecbdb5..b455bb9 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -356,6 +356,10 @@ dnl Goto-Table 1 instruction non-zero padding
# 7: 01 -> 00
0001 0008 01 000001
+dnl Goto-Table 1 instruction go back to the previous table.
+# bad OF1.1 instructions: OFPBRC_BAD_TABLE_ID
+2,0001 0008 01 000000
+
dnl Goto-Table 1
# actions=goto_table:1
0001 0008 01 000000
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 24b9434..48f0fbf 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2607,18 +2607,29 @@ ofctl_parse_ofp11_instructions(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
enum ofperr error;
size_t size;
struct ds s;
+ const char *table_id;
+ char *instructions;
+
+ /* Parse table_id separated with the follow-up instructions by ",", if
+ * any. */
+ instructions = ds_cstr(&in);
+ table_id = NULL;
+ if (strstr(instructions, ",")) {
+ table_id = strsep(&instructions, ",");
+ }
/* Parse hex bytes. */
ofpbuf_init(&of11_in, 0);
- if (ofpbuf_put_hex(&of11_in, ds_cstr(&in), NULL)[0] != '\0') {
+ if (ofpbuf_put_hex(&of11_in, instructions, NULL)[0] != '\0') {
ovs_fatal(0, "Trailing garbage in hex data");
}
/* Convert to ofpacts. */
ofpbuf_init(&ofpacts, 0);
size = of11_in.size;
- error = ofpacts_pull_openflow11_instructions(&of11_in, of11_in.size,
- &ofpacts);
+ error = ofpacts_pull_openflow11_instructions(
+ &of11_in, of11_in.size, table_id ? atoi(table_id) : 0,
+ &ofpacts);
if (error) {
printf("bad OF1.1 instructions: %s\n\n", ofperr_get_name(error));
ofpbuf_uninit(&ofpacts);
--
1.7.2.5
More information about the dev
mailing list