ofproto-dpif-xlate: Limit memory and time that translation can consume.

The resubmit depth has been limited to MAX_RESUBMIT_RECURSION, currently
64, for a long time.  But the flow "actions=resubmit:1, resubmit:2,
output:1" 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@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Justin Pettit <jpettit@nicira.com>

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index cced7cc..9090b05 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -56,6 +56,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 {
@@ -165,7 +169,10 @@ struct xlate_ctx {
                                  * prior to an mpls_push so that it may be
                                  * used for a subsequent mpls_pop. */
 
-    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. */
@@ -1685,6 +1692,7 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule)
         rule_dpif_credit_stats(rule, ctx->xin->resubmit_stats);
     }
 
+    ctx->resubmits++;
     ctx->recurse++;
     ctx->rule = rule;
     actions = rule_dpif_get_actions(rule);
@@ -1698,7 +1706,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 > UINT16_MAX) {
+        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;
@@ -1738,12 +1757,10 @@ xlate_table_action(struct xlate_ctx *ctx,
         }
 
         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
@@ -2702,6 +2719,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 ea36020..bfb4bb3 100644 (file)
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -2875,3 +2875,64 @@ unixctl_received           0.4/sec     0.033/sec        0.0047/sec   total: 17
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+\f
+AT_BANNER([ofproto-dpif - flow translation resource limits])
+
+AT_SETUP([ofproto-dpif - infinite resubmit])
+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 - exponential resubmit chain])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], 1)
+(for i in `seq 1 64`; do
+     j=`expr $i + 1`
+     echo "in_port=$i, actions=resubmit:$j, resubmit:$j, local"
+ done
+ echo "in_port=65, actions=local") > flows
+ AT_CHECK([ovs-ofctl add-flows br0 flows])
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1'], [0], [stdout])
+AT_CHECK([grep -c 'over 4096 resubmit actions' ovs-vswitchd.log], [0], [1
+])
+OVS_VSWITCHD_STOP(["/over.*resubmit actions/d"])
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - too many output actions])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], 1)
+(for i in `seq 1 12`; do
+     j=`expr $i + 1`
+     echo "in_port=$i, actions=resubmit:$j, resubmit:$j, local"
+ done
+ echo "in_port=13, actions=local,local,local,local,local,local,local,local") > flows
+ AT_CHECK([ovs-ofctl add-flows br0 flows])
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1'], [0], [stdout])
+AT_CHECK([grep -c 'resubmits yielded over 64 kB of actions' ovs-vswitchd.log], [0], [1
+])
+OVS_VSWITCHD_STOP(["/resubmits yielded over 64 kB of actions/d"])
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - stack too deep])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], 1)
+(for i in `seq 1 12`; do
+     j=`expr $i + 1`
+     echo "in_port=$i, actions=resubmit:$j, resubmit:$j, local"
+ done
+ push="push:NXM_NX_REG0[[]]"
+ echo "in_port=13, actions=$push,$push,$push,$push,$push,$push,$push,$push") > flows
+ AT_CHECK([ovs-ofctl add-flows br0 flows])
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1'], [0], [stdout])
+AT_CHECK([grep -c 'resubmits yielded over 64 kB of stack' ovs-vswitchd.log], [0], [1
+])
+OVS_VSWITCHD_STOP(["/resubmits yielded over 64 kB of stack/d"])
+AT_CLEANUP

diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index 839d41f..3bcffc2 100644 (file)
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -89,6 +89,10 @@ m4_define([OVS_VSWITCHD_START],
 m4_divert_push([PREPARE_TESTS])
 check_logs () {
     sed -n "$1
+/timeval.*Unreasonably long [[0-9]]*ms poll interval/d
+/timeval.*faults: [[0-9]]* minor, [[0-9]]* major/d
+/timeval.*disk: [[0-9]]* reads, [[0-9]]* writes/d
+/timeval.*context switches: [[0-9]]* voluntary, [[0-9]]* involuntary/d
 /|WARN|/p
 /|ERR|/p
 /|EMER|/p" ovs-vswitchd.log ovsdb-server.log