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

Jing Ai jinga at google.com
Wed Jun 5 20:42:16 UTC 2013


Thanks, Ben!

Best,
Jing


On Wed, Jun 5, 2013 at 1:38 PM, Ben Pfaff <blp at nicira.com> wrote:

> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130605/7f9d51a7/attachment-0003.html>


More information about the dev mailing list