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