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

Ben Pfaff blp at nicira.com
Tue Jun 4 19:51:33 UTC 2013


I was out Friday through Monday, sorry.

On Tue, Jun 04, 2013 at 10:33:53AM -0700, Jing Ai wrote:
> Anyone reviewing it? Thanks!
> 
> Best,
> Jing
> 
> 
> On Fri, May 31, 2013 at 5:46 PM, Jing Ai <jinga at google.com> wrote:
> 
> > --- Resent the patch since the last one was messed up in format
> > --- Added and changed AUTHOR information
> >
> > 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 <jinga at google.com>
> > ---
> >  AUTHORS               |  3 ++-
> >  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 ++++++++++++++---
> >  6 files changed, 31 insertions(+), 7 deletions(-)
> >
> > diff --git a/AUTHORS b/AUTHORS
> > index d15173e..a2b0c4d 100644
> > --- a/AUTHORS
> > +++ b/AUTHORS
> > @@ -45,6 +45,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
> > @@ -153,7 +154,7 @@ 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
> > +Jing Ai                 jinga at google.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 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.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..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.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..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);
> > --
> > 1.8.2.1
> >
> >

> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list