On Fri, Aug 23, 2013 at 12:05:44AM -0700, Ethan Jackson wrote:
> "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?
Yes. Before this patch, you'd then pop up one level and hit
MAX_RESUBMIT again, then pop up two levels and visit the other branch,
hitting MAX_RESUBMIT two more times, then pop up three levels and
visit the other branch, hitting MAX_RESUBMIT four more times, then pop
up four levels and visit the other branch, hitting MAX_RESUBMIT eight
more times, ..., eventually visiting 2**64 nodes and using 2**64
memory.
With this patch, you hit MAX_RESUBMIT and set "exit", terminating
translation.
But it's not enough to terminate when you hit MAX_RESUBMIT because you
can easily construct flow tables that use about 2**64 memory and time
without ever hitting MAX_RESUBMIT. For example:
in_port=1, actions=resubmit:2, resubmit:2, local
in_port=2, actions=resubmit:3, resubmit:3, local
in_port=3, actions=resubmit:4, resubmit:4, local
in_port=4, actions=resubmit:5, resubmit:5, local
...
in_port=63, actions=local
(That's probably enough but you can do better: for 3**64, just
increase the number of resubmits per flow to 3.)
That's a nice example so I'll add it as a test.
> When you say it exhausts memory, are you talking about odp_actions?
Yes.
> 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?
That would work too. I chose to put the limit in the resubmit code
because it's cheaper to check only for a subset of actions rather than
for every action and because it seems somewhat nice to have all the
resource limits checking in one place.
Here's a revised version that has a test to trigger each resource
limit individually.
--8<--------------------------cut here-------------------------->8--
From: Ben Pfaff <b...@nicira.com>
Date: Fri, 23 Aug 2013 10:03:32 -0700
Subject: [PATCH] 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 <p...@nicira.com>
Signed-off-by: Ben Pfaff <b...@nicira.com>
---
ofproto/ofproto-dpif-xlate.c | 32 +++++++++++++++++-----
tests/ofproto-dpif.at | 61 ++++++++++++++++++++++++++++++++++++++++++
tests/ofproto-macros.at | 4 +++
3 files changed, 90 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..97a2615 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -2808,3 +2808,64 @@ AT_CHECK([ovs-appctl bond/show | sed -n
'/^.*may_enable:.*/p'], [0], [dnl
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+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
--- 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
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
