[ovs-dev] [PATCH] ofproto-dpif-xlate: Limit actions, stack use to 64 kB at resubmit time.

[Ethan Jackson]

 "actions=resubmit:1, resubmit:2, output:1"

I don't totally get this.  If this is your only flow, won't you fall
into an infinite loop and hit the MAX_RESUBMIT depth?

When you say it exhausts memory, are you talking about odp_actions?
If so, why not simply limit the size of the odp_actions directly by
bailing out in do_xlate_actions() when it's beyond some limit?

I haven't read the patch carefully yet.

Ethan

 generates about 2**MAX_RESUBMIT_RECURSION output actions,
> exhausting memory.  This commit fixes the problem.
>
> Such a flow also requires 2**MAX_RESUBMIT_RECURSION time for translation.
> This commit fixes that problem too.
>
> Bug #19277.
> Reported-by: Paul Ingram <paul at nicira.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  ofproto/ofproto-dpif-xlate.c |   32 +++++++++++++++++++++++++-------
>  tests/ofproto-dpif.at        |   14 ++++++++++++++
>  2 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 4578675..80a4579 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -55,6 +55,10 @@ VLOG_DEFINE_THIS_MODULE(ofproto_dpif_xlate);
>   * flow translation. */
>  #define MAX_RESUBMIT_RECURSION 64
>
> +/* Maximum number of resubmit actions in a flow translation, whether they are
> + * recursive or not. */
> +#define MAX_RESUBMITS (MAX_RESUBMIT_RECURSION * MAX_RESUBMIT_RECURSION)
> +
>  struct ovs_rwlock xlate_rwlock = OVS_RWLOCK_INITIALIZER;
>
>  struct xbridge {
> @@ -157,7 +161,10 @@ struct xlate_ctx {
>      /* The rule that we are currently translating, or NULL. */
>      struct rule_dpif *rule;
>
> -    int recurse;                /* Recursion level, via xlate_table_action. */
> +    /* Resubmit statistics, via xlate_table_action(). */
> +    int recurse;                /* Current resubmit nesting depth. */
> +    int resubmits;              /* Total number of resubmits. */
> +
>      uint32_t orig_skb_priority; /* Priority when packet arrived. */
>      uint8_t table_id;           /* OpenFlow table ID where flow was found. */
>      uint32_t sflow_n_outputs;   /* Number of output ports. */
> @@ -1667,7 +1674,18 @@ static void
>  xlate_table_action(struct xlate_ctx *ctx,
>                     ofp_port_t in_port, uint8_t table_id, bool may_packet_in)
>  {
> -    if (ctx->recurse < MAX_RESUBMIT_RECURSION) {
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +
> +    if (ctx->recurse >= MAX_RESUBMIT_RECURSION) {
> +        VLOG_ERR_RL(&rl, "resubmit actions recursed over %d times",
> +                    MAX_RESUBMIT_RECURSION);
> +    } else if (ctx->resubmits >= MAX_RESUBMITS) {
> +        VLOG_ERR_RL(&rl, "over %d resubmit actions", MAX_RESUBMITS);
> +    } else if (ctx->xout->odp_actions.size >= 65536) {
> +        VLOG_ERR_RL(&rl, "resubmits yielded over 64 kB of actions");
> +    } else if (ctx->stack.size >= 65536) {
> +        VLOG_ERR_RL(&rl, "resubmits yielded over 64 kB of stack");
> +    } else {
>          struct rule_dpif *rule;
>          ofp_port_t old_in_port = ctx->xin->flow.in_port.ofp_port;
>          uint8_t old_table_id = ctx->table_id;
> @@ -1713,6 +1731,7 @@ xlate_table_action(struct xlate_ctx *ctx,
>          if (rule) {
>              struct rule_dpif *old_rule = ctx->rule;
>
> +            ctx->resubmits++;
>              ctx->recurse++;
>              ctx->rule = rule;
>              do_xlate_actions(rule->up.ofpacts, rule->up.ofpacts_len, ctx);
> @@ -1722,12 +1741,10 @@ xlate_table_action(struct xlate_ctx *ctx,
>          rule_release(rule);
>
>          ctx->table_id = old_table_id;
> -    } else {
> -        static struct vlog_rate_limit recurse_rl = VLOG_RATE_LIMIT_INIT(1, 1);
> -
> -        VLOG_ERR_RL(&recurse_rl, "resubmit actions recursed over %d times",
> -                    MAX_RESUBMIT_RECURSION);
> +        return;
>      }
> +
> +    ctx->exit = true;
>  }
>
>  static void
> @@ -2602,6 +2619,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>      }
>
>      ctx.recurse = 0;
> +    ctx.resubmits = 0;
>      ctx.orig_skb_priority = flow->skb_priority;
>      ctx.table_id = 0;
>      ctx.exit = false;
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index af19672..af2a19e 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -20,6 +20,20 @@ AT_CHECK([tail -1 stdout], [0],
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> +# Tests that a
> +AT_SETUP([ofproto-dpif - resubmit bounds memory and time])
> +OVS_VSWITCHD_START
> +AT_CHECK([ovs-ofctl add-flow br0 actions=resubmit:1,resubmit:2,output:3])
> +AT_CHECK([ovs-appctl ofproto/trace br0 'eth_dst(ff:ff:ff:ff:ff:ff)'],
> +  [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0], [Datapath actions: drop
> +])
> +AT_CHECK([grep -c 'resubmit actions recursed over 64 times' ovs-vswitchd.log],
> +  [0], [1
> +])
> +OVS_VSWITCHD_STOP(["/resubmit actions recursed/d"])
> +AT_CLEANUP
> +
>  AT_SETUP([ofproto-dpif - goto table])
>  OVS_VSWITCHD_START
>  ADD_OF_PORTS([br0], [1], [10], [11])
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev