[ovs-dev] [PATCH]: ofp-actions: check table_id in goto_table instruction is larger than the current table_id

[Jing Ai]

Found a bug that OVS does not return any error message when the goto_table_id is smaller than (or equals to) then current table where the flow is installed. As a result, I install a flow as follows (using "ovs-ofctl dump-flows br0")

table_id=4, duration=40s, priority=32768, n_packets=0, n_bytes=0, tcp,in_port=1,dl_src=00:06:07:08:09:0a,dl_dst=00:01:02:03:04:05,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,tp_src=1234,tp_dst=80,actions=goto_table:3
Best,Jing
Signed-off-by: Jing Ai <ai_jing2000 at hotmail.com>
 lib/ofp-actions.c     |  5 +++++ lib/ofp-actions.h     |  1 + lib/ofp-util.c        |  8 +++++--- tests/ofp-actions.at  |  4 ++++ utilities/ovs-ofctl.c | 17 ++++++++++++++--- 5 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.cindex 026a376..30f3f65 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.hindex 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.cindex 3c8d5a2..42021a8 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;         }@@ -1652,7 +1653,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,     case OFPUTIL_P_OF13_OXM: {         struct ofp11_flow_mod *ofm; -        msg = ofpraw_alloc(OFPRAW_OFPT11_FLOW_MOD, +        msg = ofpraw_alloc(OFPRAW_OFPT11_FLOW_MOD,                            ofputil_protocol_to_ofp_version(protocol),                            NXM_TYPICAL_LEN + fm->ofpacts_len);         ofm = ofpbuf_put_zeros(msg, sizeof *ofm);@@ -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.atindex 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 000000diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.cindex 24b9434..009768a 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 *instructions;+        const char *table_id;++        /* 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 ? (uint8_t)atoi(table_id) : 0,+            &ofpacts);         if (error) {             printf("bad OF1.1 instructions: %s\n\n", ofperr_get_name(error));             ofpbuf_uninit(&ofpacts);
 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130531/d8559e42/attachment-0003.html>